| Message ID | 515427654218b7ce22441f635115e93cf74d6302.1488491988.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 02, 2017 at 08:10:29PM -0500, Richard Guy Briggs wrote: > The audit subsystem is adding a BPRM_FCAPS record when auditing setuid > application execution (SYSCALL execve). This is not expected as it was > supposed to be limited to when the file system actually had capabilities > in an extended attribute. It lists all capabilities making the event > really ugly to parse what is happening. The PATH record correctly > records the setuid bit and owner. Suppress the BPRM_FCAPS record on > set*id. > > See: https://github.com/linux-audit/audit-kernel/issues/16 Hey Richard, one possibly audit-worth case which (if I read correctly) this will skip is where a setuid-root binary has filecaps which *limit* its privs. Does that matter? > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/commoncap.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 14540bd..8f6bedf 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -594,16 +594,17 @@ skip: > /* > * Audit candidate if current->cap_effective is set > * > - * We do not bother to audit if 3 things are true: > + * We do not bother to audit if 4 things are true: > * 1) cap_effective has all caps > * 2) we are root > * 3) root is supposed to have all caps (SECURE_NOROOT) > + * 4) we are running a set*id binary > * Since this is just a normal root execing a process. > * > * Number 1 above might fail if you don't have a full bset, but I think > * that is interesting information to audit. > */ > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > + if (!is_setid && !cap_issubset(new->cap_effective, new->cap_ambient)) { > if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > issecure(SECURE_NOROOT)) { > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-03-02 20:07, Serge E. Hallyn wrote: > On Thu, Mar 02, 2017 at 08:10:29PM -0500, Richard Guy Briggs wrote: > > The audit subsystem is adding a BPRM_FCAPS record when auditing setuid > > application execution (SYSCALL execve). This is not expected as it was > > supposed to be limited to when the file system actually had capabilities > > in an extended attribute. It lists all capabilities making the event > > really ugly to parse what is happening. The PATH record correctly > > records the setuid bit and owner. Suppress the BPRM_FCAPS record on > > set*id. > > > > See: https://github.com/linux-audit/audit-kernel/issues/16 > > Hey Richard, Hi Serge, > one possibly audit-worth case which (if I read correctly) this will > skip is where a setuid-root binary has filecaps which *limit* its privs. > Does that matter? I hadn't thought of that case, but I did consider in the setuid case comparing before and after without setuid forcing the drop of all capabilities via "ambient". Mind you, this bug has been around before Luto's patch that adds the ambient capabilities set. Paul? > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > security/commoncap.c | 5 +++-- > > 1 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index 14540bd..8f6bedf 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -594,16 +594,17 @@ skip: > > /* > > * Audit candidate if current->cap_effective is set > > * > > - * We do not bother to audit if 3 things are true: > > + * We do not bother to audit if 4 things are true: > > * 1) cap_effective has all caps > > * 2) we are root > > * 3) root is supposed to have all caps (SECURE_NOROOT) > > + * 4) we are running a set*id binary > > * Since this is just a normal root execing a process. > > * > > * Number 1 above might fail if you don't have a full bset, but I think > > * that is interesting information to audit. > > */ > > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > > + if (!is_setid && !cap_issubset(new->cap_effective, new->cap_ambient)) { > > if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > > issecure(SECURE_NOROOT)) { > > -- > > 1.7.1 - RGB -- Richard Guy Briggs <rgb@redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-03-02 21:50, Richard Guy Briggs wrote: > On 2017-03-02 20:07, Serge E. Hallyn wrote: > > On Thu, Mar 02, 2017 at 08:10:29PM -0500, Richard Guy Briggs wrote: > > > The audit subsystem is adding a BPRM_FCAPS record when auditing setuid > > > application execution (SYSCALL execve). This is not expected as it was > > > supposed to be limited to when the file system actually had capabilities > > > in an extended attribute. It lists all capabilities making the event > > > really ugly to parse what is happening. The PATH record correctly > > > records the setuid bit and owner. Suppress the BPRM_FCAPS record on > > > set*id. > > > > > > See: https://github.com/linux-audit/audit-kernel/issues/16 > > > > Hey Richard, > > Hi Serge, > > > one possibly audit-worth case which (if I read correctly) this will > > skip is where a setuid-root binary has filecaps which *limit* its privs. > > Does that matter? > > I hadn't thought of that case, but I did consider in the setuid case > comparing before and after without setuid forcing the drop of all > capabilities via "ambient". Mind you, this bug has been around before > Luto's patch that adds the ambient capabilities set. Can you suggest a scenario where that might happen? Can you come up with an idea for a test case? At first I figured I could simply go from root and su to an unprivileged user, but that doesn't trigger it and then naively thought I could strace both directions to find out the difference and su or sudo to root really doesn't like being straced. > Paul? > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > security/commoncap.c | 5 +++-- > > > 1 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > index 14540bd..8f6bedf 100644 > > > --- a/security/commoncap.c > > > +++ b/security/commoncap.c > > > @@ -594,16 +594,17 @@ skip: > > > /* > > > * Audit candidate if current->cap_effective is set > > > * > > > - * We do not bother to audit if 3 things are true: > > > + * We do not bother to audit if 4 things are true: > > > * 1) cap_effective has all caps > > > * 2) we are root > > > * 3) root is supposed to have all caps (SECURE_NOROOT) > > > + * 4) we are running a set*id binary > > > * Since this is just a normal root execing a process. > > > * > > > * Number 1 above might fail if you don't have a full bset, but I think > > > * that is interesting information to audit. > > > */ > > > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > > > + if (!is_setid && !cap_issubset(new->cap_effective, new->cap_ambient)) { > > > if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > > > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > > > issecure(SECURE_NOROOT)) { > > > -- > > > 1.7.1 > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Richard Guy Briggs (rgb@redhat.com): > On 2017-03-02 21:50, Richard Guy Briggs wrote: > > On 2017-03-02 20:07, Serge E. Hallyn wrote: > > > On Thu, Mar 02, 2017 at 08:10:29PM -0500, Richard Guy Briggs wrote: > > > > The audit subsystem is adding a BPRM_FCAPS record when auditing setuid > > > > application execution (SYSCALL execve). This is not expected as it was > > > > supposed to be limited to when the file system actually had capabilities > > > > in an extended attribute. It lists all capabilities making the event > > > > really ugly to parse what is happening. The PATH record correctly > > > > records the setuid bit and owner. Suppress the BPRM_FCAPS record on > > > > set*id. > > > > > > > > See: https://github.com/linux-audit/audit-kernel/issues/16 > > > > > > Hey Richard, > > > > Hi Serge, > > > > > one possibly audit-worth case which (if I read correctly) this will > > > skip is where a setuid-root binary has filecaps which *limit* its privs. > > > Does that matter? > > > > I hadn't thought of that case, but I did consider in the setuid case > > comparing before and after without setuid forcing the drop of all > > capabilities via "ambient". Mind you, this bug has been around before > > Luto's patch that adds the ambient capabilities set. > > Can you suggest a scenario where that might happen? Sorry, do you mean the case I brought up, or the one you mentioned? I don't quite understnad the one you brought up. For mine it's pretty simple to reproduce, just # as root cp `which sleep` /tmp/sleep chown root: /tmp/sleep chmod u+s /tmp/sleep setcap cap_sys_admin+pe /tmp/sleep # as non-root /tmp/sleep 200 & cat /proc/$!/status | egrep -e '(^[UG]id|^Cap)' > Can you come up with an idea for a test case? At first I figured I > could simply go from root and su to an unprivileged user, but that Ok - that sounds like you're talking about the case you brought up then. Certainly setuid to nonroot should clear ambient, but what's the problem? Is that broken, or are you wondering whether that should be logged? > doesn't trigger it and then naively thought I could strace both > directions to find out the difference and su or sudo to root really > doesn't like being straced. > > > Paul? > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > --- > > > > security/commoncap.c | 5 +++-- > > > > 1 files changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > index 14540bd..8f6bedf 100644 > > > > --- a/security/commoncap.c > > > > +++ b/security/commoncap.c > > > > @@ -594,16 +594,17 @@ skip: > > > > /* > > > > * Audit candidate if current->cap_effective is set > > > > * > > > > - * We do not bother to audit if 3 things are true: > > > > + * We do not bother to audit if 4 things are true: > > > > * 1) cap_effective has all caps > > > > * 2) we are root > > > > * 3) root is supposed to have all caps (SECURE_NOROOT) > > > > + * 4) we are running a set*id binary > > > > * Since this is just a normal root execing a process. > > > > * > > > > * Number 1 above might fail if you don't have a full bset, but I think > > > > * that is interesting information to audit. > > > > */ > > > > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > > > > + if (!is_setid && !cap_issubset(new->cap_effective, new->cap_ambient)) { > > > > if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > > > > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > > > > issecure(SECURE_NOROOT)) { > > > > -- > > > > 1.7.1 > > > > - RGB > > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Kernel Security Engineering, Base Operating Systems, Red Hat > Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-03-07 12:10, Serge E. Hallyn wrote: > Quoting Richard Guy Briggs (rgb@redhat.com): > > On 2017-03-02 21:50, Richard Guy Briggs wrote: > > > On 2017-03-02 20:07, Serge E. Hallyn wrote: > > > > On Thu, Mar 02, 2017 at 08:10:29PM -0500, Richard Guy Briggs wrote: > > > > > The audit subsystem is adding a BPRM_FCAPS record when auditing setuid > > > > > application execution (SYSCALL execve). This is not expected as it was > > > > > supposed to be limited to when the file system actually had capabilities > > > > > in an extended attribute. It lists all capabilities making the event > > > > > really ugly to parse what is happening. The PATH record correctly > > > > > records the setuid bit and owner. Suppress the BPRM_FCAPS record on > > > > > set*id. > > > > > > > > > > See: https://github.com/linux-audit/audit-kernel/issues/16 > > > > > > > > Hey Richard, > > > > > > Hi Serge, > > > > > > > one possibly audit-worth case which (if I read correctly) this will > > > > skip is where a setuid-root binary has filecaps which *limit* its privs. > > > > Does that matter? > > > > > > I hadn't thought of that case, but I did consider in the setuid case > > > comparing before and after without setuid forcing the drop of all > > > capabilities via "ambient". Mind you, this bug has been around before > > > Luto's patch that adds the ambient capabilities set. > > > > Can you suggest a scenario where that might happen? > > Sorry, do you mean the case I brought up, or the one you mentioned? I > don't quite understnad the one you brought up. For mine it's pretty > simple to reproduce, just I was talking about the case you brought up, but they could be the same case. I was thinking of a case where the caps actually change, but are overridden by the blanket full permissions of setuid. > # as root > cp `which sleep` /tmp/sleep > chown root: /tmp/sleep > chmod u+s /tmp/sleep > setcap cap_sys_admin+pe /tmp/sleep > # as non-root > /tmp/sleep 200 & > cat /proc/$!/status | egrep -e '(^[UG]id|^Cap)' I don't see this setuid sleep behave differently than the original one. Was this intended to trigger that audit rule? I don't see it doing that. > > Can you come up with an idea for a test case? At first I figured I > > could simply go from root and su to an unprivileged user, but that > > Ok - that sounds like you're talking about the case you brought up then. > Certainly setuid to nonroot should clear ambient, but what's the problem? > Is that broken, or are you wondering whether that should be logged? I wonder if it should be logged. > > doesn't trigger it and then naively thought I could strace both > > directions to find out the difference and su or sudo to root really > > doesn't like being straced. (I solved this testing problem by setting strace setuid temporarily. Not sure what I learned from that... Output was larger than I expected.) > > > Paul? > > > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > > --- > > > > > security/commoncap.c | 5 +++-- > > > > > 1 files changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > index 14540bd..8f6bedf 100644 > > > > > --- a/security/commoncap.c > > > > > +++ b/security/commoncap.c > > > > > @@ -594,16 +594,17 @@ skip: > > > > > /* > > > > > * Audit candidate if current->cap_effective is set > > > > > * > > > > > - * We do not bother to audit if 3 things are true: > > > > > + * We do not bother to audit if 4 things are true: > > > > > * 1) cap_effective has all caps > > > > > * 2) we are root > > > > > * 3) root is supposed to have all caps (SECURE_NOROOT) > > > > > + * 4) we are running a set*id binary > > > > > * Since this is just a normal root execing a process. > > > > > * > > > > > * Number 1 above might fail if you don't have a full bset, but I think > > > > > * that is interesting information to audit. > > > > > */ > > > > > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > > > > > + if (!is_setid && !cap_issubset(new->cap_effective, new->cap_ambient)) { > > > > > if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > > > > > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > > > > > issecure(SECURE_NOROOT)) { > > > > > -- > > > > > 1.7.1 > > > > > > - RGB > > > > - RGB > > > > -- > > Richard Guy Briggs <rgb@redhat.com> > > Kernel Security Engineering, Base Operating Systems, Red Hat > > Remote, Ottawa, Canada > > Voice: +1.647.777.2635, Internal: (81) 32635 - RGB -- Richard Guy Briggs <rgb@redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Richard Guy Briggs (rgb@redhat.com): > On 2017-03-07 12:10, Serge E. Hallyn wrote: > > Quoting Richard Guy Briggs (rgb@redhat.com): > > > On 2017-03-02 21:50, Richard Guy Briggs wrote: > > > > On 2017-03-02 20:07, Serge E. Hallyn wrote: > > > > > On Thu, Mar 02, 2017 at 08:10:29PM -0500, Richard Guy Briggs wrote: > > > > > > The audit subsystem is adding a BPRM_FCAPS record when auditing setuid > > > > > > application execution (SYSCALL execve). This is not expected as it was > > > > > > supposed to be limited to when the file system actually had capabilities > > > > > > in an extended attribute. It lists all capabilities making the event > > > > > > really ugly to parse what is happening. The PATH record correctly > > > > > > records the setuid bit and owner. Suppress the BPRM_FCAPS record on > > > > > > set*id. > > > > > > > > > > > > See: https://github.com/linux-audit/audit-kernel/issues/16 > > > > > > > > > > Hey Richard, > > > > > > > > Hi Serge, > > > > > > > > > one possibly audit-worth case which (if I read correctly) this will > > > > > skip is where a setuid-root binary has filecaps which *limit* its privs. > > > > > Does that matter? > > > > > > > > I hadn't thought of that case, but I did consider in the setuid case > > > > comparing before and after without setuid forcing the drop of all > > > > capabilities via "ambient". Mind you, this bug has been around before > > > > Luto's patch that adds the ambient capabilities set. > > > > > > Can you suggest a scenario where that might happen? > > > > Sorry, do you mean the case I brought up, or the one you mentioned? I > > don't quite understnad the one you brought up. For mine it's pretty > > simple to reproduce, just > > I was talking about the case you brought up, but they could be the same case. > > I was thinking of a case where the caps actually change, but are > overridden by the blanket full permissions of setuid. > > > # as root > > cp `which sleep` /tmp/sleep > > chown root: /tmp/sleep > > chmod u+s /tmp/sleep > > setcap cap_sys_admin+pe /tmp/sleep > > # as non-root > > /tmp/sleep 200 & > > cat /proc/$!/status | egrep -e '(^[UG]id|^Cap)' > > I don't see this setuid sleep behave differently than the original one. Oh, my /tmp is nosuid so actually I have to do it in $HOME. There I get: CapPrm: 0000003fffffffff CapEff: 0000003fffffffff for simple setuid-root, and CapPrm: 0000000000200000 CapEff: 0000000000200000 for setuid-root plus file-caps. > Was this intended to trigger that audit rule? I don't see it doing that. I was suggesting that it might be worth auditing, yes. > > > Can you come up with an idea for a test case? At first I figured I > > > could simply go from root and su to an unprivileged user, but that > > > > Ok - that sounds like you're talking about the case you brought up then. > > Certainly setuid to nonroot should clear ambient, but what's the problem? > > Is that broken, or are you wondering whether that should be logged? > > I wonder if it should be logged. Yeah I could see it being worth logging, but would be nice for audit folks to decide. -serge -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: > > > > > one possibly audit-worth case which (if I read correctly) this will > > > > > skip is where a setuid-root binary has filecaps which *limit* its > > > > > privs. > > > > > Does that matter? > > > > > > > > I hadn't thought of that case, but I did consider in the setuid case > > > > comparing before and after without setuid forcing the drop of all > > > > capabilities via "ambient". Mind you, this bug has been around before > > > > Luto's patch that adds the ambient capabilities set. > > > > > > Can you suggest a scenario where that might happen? > > > > Sorry, do you mean the case I brought up, or the one you mentioned? I > > don't quite understnad the one you brought up. For mine it's pretty > > simple to reproduce, just > > I was talking about the case you brought up, but they could be the same > case. > > I was thinking of a case where the caps actually change, but are > overridden by the blanket full permissions of setuid. If there actually is a change in capability bits besides the implied change of capabilities based on the change of the uid alone, then it should be logged. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-03-09 09:34, Steve Grubb wrote: > On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: > > > > > > one possibly audit-worth case which (if I read correctly) this will > > > > > > skip is where a setuid-root binary has filecaps which *limit* its > > > > > > privs. > > > > > > Does that matter? > > > > > > > > > > I hadn't thought of that case, but I did consider in the setuid case > > > > > comparing before and after without setuid forcing the drop of all > > > > > capabilities via "ambient". Mind you, this bug has been around before > > > > > Luto's patch that adds the ambient capabilities set. > > > > > > > > Can you suggest a scenario where that might happen? > > > > > > Sorry, do you mean the case I brought up, or the one you mentioned? I > > > don't quite understnad the one you brought up. For mine it's pretty > > > simple to reproduce, just > > > > I was talking about the case you brought up, but they could be the same > > case. > > > > I was thinking of a case where the caps actually change, but are > > overridden by the blanket full permissions of setuid. > > If there actually is a change in capability bits besides the implied change of > capabilities based on the change of the uid alone, then it should be logged. Are you speaking of a change in pP' only from pI, or also pI', pE' and pA'? Something like ( pP' xor pI ) not empty? The previous patch I'd sent was reasonably easy to understand, but I'm having trouble adding this new twist to the logic expression in question due to the inverted combination of pre-existing items. I'm having trouble visualizing a 5 or more-dimensional Karnaugh map... While I am at it, I notice pA is missing from the audit record. The record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", "new_pe" so in keeping with the previous record normalizations, I'd like to change the "new_*" variants to simply drop the "new_" prefix. https://github.com/linux-audit/audit-kernel/issues/40 > -Steve - RGB -- Richard Guy Briggs <rgb@redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 29, 2017 at 6:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > On 2017-03-09 09:34, Steve Grubb wrote: >> On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: >> > > > > > one possibly audit-worth case which (if I read correctly) this will >> > > > > > skip is where a setuid-root binary has filecaps which *limit* its >> > > > > > privs. >> > > > > > Does that matter? >> > > > > >> > > > > I hadn't thought of that case, but I did consider in the setuid case >> > > > > comparing before and after without setuid forcing the drop of all >> > > > > capabilities via "ambient". Mind you, this bug has been around before >> > > > > Luto's patch that adds the ambient capabilities set. >> > > > >> > > > Can you suggest a scenario where that might happen? >> > > >> > > Sorry, do you mean the case I brought up, or the one you mentioned? I >> > > don't quite understnad the one you brought up. For mine it's pretty >> > > simple to reproduce, just >> > >> > I was talking about the case you brought up, but they could be the same >> > case. >> > >> > I was thinking of a case where the caps actually change, but are >> > overridden by the blanket full permissions of setuid. >> >> If there actually is a change in capability bits besides the implied change of >> capabilities based on the change of the uid alone, then it should be logged. > > Are you speaking of a change in pP' only from pI, or also pI', pE' and pA'? > > Something like ( pP' xor pI ) not empty? > > The previous patch I'd sent was reasonably easy to understand, but I'm > having trouble adding this new twist to the logic expression in question > due to the inverted combination of pre-existing items. I'm having > trouble visualizing a 5 or more-dimensional Karnaugh map... > > While I am at it, I notice pA is missing from the audit record. The > record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", > "new_pe" so in keeping with the previous record normalizations, I'd like > to change the "new_*" variants to simply drop the "new_" prefix. > > https://github.com/linux-audit/audit-kernel/issues/40 Yes, there is the separate ambient capabilities record patch, but where do we stand with this patch? From what I gather there is still some uncertainty here?
On 2017-04-11 15:36, Paul Moore wrote: > On Wed, Mar 29, 2017 at 6:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2017-03-09 09:34, Steve Grubb wrote: > >> On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: > >> > > > > > one possibly audit-worth case which (if I read correctly) this will > >> > > > > > skip is where a setuid-root binary has filecaps which *limit* its > >> > > > > > privs. > >> > > > > > Does that matter? > >> > > > > > >> > > > > I hadn't thought of that case, but I did consider in the setuid case > >> > > > > comparing before and after without setuid forcing the drop of all > >> > > > > capabilities via "ambient". Mind you, this bug has been around before > >> > > > > Luto's patch that adds the ambient capabilities set. > >> > > > > >> > > > Can you suggest a scenario where that might happen? > >> > > > >> > > Sorry, do you mean the case I brought up, or the one you mentioned? I > >> > > don't quite understnad the one you brought up. For mine it's pretty > >> > > simple to reproduce, just > >> > > >> > I was talking about the case you brought up, but they could be the same > >> > case. > >> > > >> > I was thinking of a case where the caps actually change, but are > >> > overridden by the blanket full permissions of setuid. > >> > >> If there actually is a change in capability bits besides the implied change of > >> capabilities based on the change of the uid alone, then it should be logged. > > > > Are you speaking of a change in pP' only from pI, or also pI', pE' and pA'? > > > > Something like ( pP' xor pI ) not empty? > > > > The previous patch I'd sent was reasonably easy to understand, but I'm > > having trouble adding this new twist to the logic expression in question > > due to the inverted combination of pre-existing items. I'm having > > trouble visualizing a 5 or more-dimensional Karnaugh map... > > > > While I am at it, I notice pA is missing from the audit record. The > > record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", > > "new_pe" so in keeping with the previous record normalizations, I'd like > > to change the "new_*" variants to simply drop the "new_" prefix. > > > > https://github.com/linux-audit/audit-kernel/issues/40 > > Yes, there is the separate ambient capabilities record patch, but > where do we stand with this patch? From what I gather there is still > some uncertainty here? Yes, I put this on my back burner thinking about how best to re-approach this, hoping others would offer some insight or advice how to attack this, otherwise I'm going to end up with a horrendous conditional expression, I fear. Steve, I was hoping to get a clarification from you about which capability bits had changed. Serge, do you have any suggestions on how to approach the conditional logic? > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Richard Guy Briggs (rgb@redhat.com): > On 2017-04-11 15:36, Paul Moore wrote: > > On Wed, Mar 29, 2017 at 6:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2017-03-09 09:34, Steve Grubb wrote: > > >> On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: > > >> > > > > > one possibly audit-worth case which (if I read correctly) this will > > >> > > > > > skip is where a setuid-root binary has filecaps which *limit* its > > >> > > > > > privs. > > >> > > > > > Does that matter? > > >> > > > > > > >> > > > > I hadn't thought of that case, but I did consider in the setuid case > > >> > > > > comparing before and after without setuid forcing the drop of all > > >> > > > > capabilities via "ambient". Mind you, this bug has been around before > > >> > > > > Luto's patch that adds the ambient capabilities set. > > >> > > > > > >> > > > Can you suggest a scenario where that might happen? > > >> > > > > >> > > Sorry, do you mean the case I brought up, or the one you mentioned? I > > >> > > don't quite understnad the one you brought up. For mine it's pretty > > >> > > simple to reproduce, just > > >> > > > >> > I was talking about the case you brought up, but they could be the same > > >> > case. > > >> > > > >> > I was thinking of a case where the caps actually change, but are > > >> > overridden by the blanket full permissions of setuid. > > >> > > >> If there actually is a change in capability bits besides the implied change of > > >> capabilities based on the change of the uid alone, then it should be logged. > > > > > > Are you speaking of a change in pP' only from pI, or also pI', pE' and pA'? > > > > > > Something like ( pP' xor pI ) not empty? > > > > > > The previous patch I'd sent was reasonably easy to understand, but I'm > > > having trouble adding this new twist to the logic expression in question > > > due to the inverted combination of pre-existing items. I'm having > > > trouble visualizing a 5 or more-dimensional Karnaugh map... > > > > > > While I am at it, I notice pA is missing from the audit record. The > > > record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", > > > "new_pe" so in keeping with the previous record normalizations, I'd like > > > to change the "new_*" variants to simply drop the "new_" prefix. > > > > > > https://github.com/linux-audit/audit-kernel/issues/40 > > > > Yes, there is the separate ambient capabilities record patch, but > > where do we stand with this patch? From what I gather there is still > > some uncertainty here? > > Yes, I put this on my back burner thinking about how best to re-approach > this, hoping others would offer some insight or advice how to attack > this, otherwise I'm going to end up with a horrendous conditional > expression, I fear. > > Steve, I was hoping to get a clarification from you about which > capability bits had changed. > > Serge, do you have any suggestions on how to approach the conditional > logic? I've been looking through the thread and the github issue, but haven't seen an example. Can you post a patch the way you'd do it off the top of your head, as a strawman? In general I'm a fan of putting complicated conditionals behind clearly named static inline helper functions, i.e. if (should_audit_caps(current)) // do the auditing then when reading should_audit_caps() your mind can process more complicated logic because it's not distracted by other things the calling function is doing... That's probably nothing you wouldn't do anyway so I don't think I'm being helpful. > > paul moore > > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, April 12, 2017 2:43:21 AM EDT Richard Guy Briggs wrote: > On 2017-04-11 15:36, Paul Moore wrote: > > On Wed, Mar 29, 2017 at 6:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2017-03-09 09:34, Steve Grubb wrote: > > >> On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: > > >> > > > > > one possibly audit-worth case which (if I read correctly) > > >> > > > > > this will > > >> > > > > > skip is where a setuid-root binary has filecaps which *limit* > > >> > > > > > its > > >> > > > > > privs. > > >> > > > > > Does that matter? > > >> > > > > > > >> > > > > I hadn't thought of that case, but I did consider in the setuid > > >> > > > > case > > >> > > > > comparing before and after without setuid forcing the drop of > > >> > > > > all > > >> > > > > capabilities via "ambient". Mind you, this bug has been around > > >> > > > > before > > >> > > > > Luto's patch that adds the ambient capabilities set. > > >> > > > > > >> > > > Can you suggest a scenario where that might happen? > > >> > > > > >> > > Sorry, do you mean the case I brought up, or the one you mentioned? > > >> > > I > > >> > > don't quite understnad the one you brought up. For mine it's > > >> > > pretty > > >> > > simple to reproduce, just > > >> > > > >> > I was talking about the case you brought up, but they could be the > > >> > same > > >> > case. > > >> > > > >> > I was thinking of a case where the caps actually change, but are > > >> > overridden by the blanket full permissions of setuid. > > >> > > >> If there actually is a change in capability bits besides the implied > > >> change of capabilities based on the change of the uid alone, then it > > >> should be logged.> > > > > Are you speaking of a change in pP' only from pI, or also pI', pE' and > > > pA'? > > > > > > Something like ( pP' xor pI ) not empty? > > > > > > The previous patch I'd sent was reasonably easy to understand, but I'm > > > having trouble adding this new twist to the logic expression in question > > > due to the inverted combination of pre-existing items. I'm having > > > trouble visualizing a 5 or more-dimensional Karnaugh map... > > > > > > While I am at it, I notice pA is missing from the audit record. The > > > record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", > > > "new_pe" so in keeping with the previous record normalizations, I'd like > > > to change the "new_*" variants to simply drop the "new_" prefix. > > > > > > https://github.com/linux-audit/audit-kernel/issues/40 > > > > Yes, there is the separate ambient capabilities record patch, but > > where do we stand with this patch? From what I gather there is still > > some uncertainty here? > > Yes, I put this on my back burner thinking about how best to re-approach > this, hoping others would offer some insight or advice how to attack > this, otherwise I'm going to end up with a horrendous conditional > expression, I fear. > > Steve, I was hoping to get a clarification from you about which > capability bits had changed. I am not sure what you are asking of me. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-04-12 14:49, Steve Grubb wrote: > On Wednesday, April 12, 2017 2:43:21 AM EDT Richard Guy Briggs wrote: > > On 2017-04-11 15:36, Paul Moore wrote: > > > On Wed, Mar 29, 2017 at 6:29 AM, Richard Guy Briggs <rgb@redhat.com> > wrote: > > > > On 2017-03-09 09:34, Steve Grubb wrote: > > > >> On Tuesday, March 7, 2017 4:10:49 PM EST Richard Guy Briggs wrote: > > > >> > > > > > one possibly audit-worth case which (if I read correctly) > > > >> > > > > > this will > > > >> > > > > > skip is where a setuid-root binary has filecaps which *limit* > > > >> > > > > > its > > > >> > > > > > privs. > > > >> > > > > > Does that matter? > > > >> > > > > > > > >> > > > > I hadn't thought of that case, but I did consider in the setuid > > > >> > > > > case > > > >> > > > > comparing before and after without setuid forcing the drop of > > > >> > > > > all > > > >> > > > > capabilities via "ambient". Mind you, this bug has been around > > > >> > > > > before > > > >> > > > > Luto's patch that adds the ambient capabilities set. > > > >> > > > > > > >> > > > Can you suggest a scenario where that might happen? > > > >> > > > > > >> > > Sorry, do you mean the case I brought up, or the one you mentioned? > > > >> > > I > > > >> > > don't quite understnad the one you brought up. For mine it's > > > >> > > pretty > > > >> > > simple to reproduce, just > > > >> > > > > >> > I was talking about the case you brought up, but they could be the > > > >> > same > > > >> > case. > > > >> > > > > >> > I was thinking of a case where the caps actually change, but are > > > >> > overridden by the blanket full permissions of setuid. > > > >> If there actually is a change in capability bits besides the implied > > > >> change of capabilities based on the change of the uid alone, then it > > > >> should be logged.> > > > > > Are you speaking of a change in pP' only from pI, or also pI', pE' and > > > > pA'? > > > > > > > > Something like ( pP' xor pI ) not empty? This is what I'm trying to clarify. > > > > The previous patch I'd sent was reasonably easy to understand, but I'm > > > > having trouble adding this new twist to the logic expression in question > > > > due to the inverted combination of pre-existing items. I'm having > > > > trouble visualizing a 5 or more-dimensional Karnaugh map... > > > > > > > > While I am at it, I notice pA is missing from the audit record. The > > > > record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", > > > > "new_pe" so in keeping with the previous record normalizations, I'd like > > > > to change the "new_*" variants to simply drop the "new_" prefix. > > > > > > > > https://github.com/linux-audit/audit-kernel/issues/40 > > > > > > Yes, there is the separate ambient capabilities record patch, but > > > where do we stand with this patch? From what I gather there is still > > > some uncertainty here? > > > > Yes, I put this on my back burner thinking about how best to re-approach > > this, hoping others would offer some insight or advice how to attack > > this, otherwise I'm going to end up with a horrendous conditional > > expression, I fear. > > > > Steve, I was hoping to get a clarification from you about which > > capability bits had changed. > > I am not sure what you are asking of me. I'm trying to understand exactly which capability sets should be compared to determine if there was a material change, but perhaps that question is better asked of Serge. What does the certification care about in terms that helps me code this? > -Steve - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, April 13, 2017 4:50:56 AM EDT Richard Guy Briggs wrote: > > > > >> > I was thinking of a case where the caps actually change, but are > > > > >> > overridden by the blanket full permissions of setuid. > > > > >> > > > > >> If there actually is a change in capability bits besides the > > > > >> implied change of capabilities based on the change of the uid > > > > >> alone, then it should be logged. > > > > > > > > > > Are you speaking of a change in pP' only from pI, or also pI', pE' > > > > > and pA'? > > > > > > > > > > Something like ( pP' xor pI ) not empty? > > This is what I'm trying to clarify. Any change other than what is expected. When you execute a setuid root application, its no surprise and expected that it got all capabilities. So, we do not want capabilities recorded. If, however, something prevented it and it wound up with one capability only, then it is a surprise and should be logged assuming the file is being watched for execution. Similarly if we have a watch on a non-setuid program and it does pick up capabilities due to file system based capabilities, then we want to know what capabilities were picked up. > > > > > The previous patch I'd sent was reasonably easy to understand, but > > > > > I'm having trouble adding this new twist to the logic expression in > > > > > question due to the inverted combination of pre-existing items. I'm > > > > > having trouble visualizing a 5 or more-dimensional Karnaugh map... > > > > > > > > > > While I am at it, I notice pA is missing from the audit record. The > > > > > record contains fields "old_pp", "old_pi", "old_pe", "new_pp", > > > > > "new_pi", "new_pe" so in keeping with the previous record > > > > > normalizations, I'd like to change the "new_*" variants to simply > > > > > drop the "new_" prefix. > > > > > > > > > > https://github.com/linux-audit/audit-kernel/issues/40 > > > > > > > > Yes, there is the separate ambient capabilities record patch, but > > > > where do we stand with this patch? From what I gather there is still > > > > some uncertainty here? > > > > > > Yes, I put this on my back burner thinking about how best to re-approach > > > this, hoping others would offer some insight or advice how to attack > > > this, otherwise I'm going to end up with a horrendous conditional > > > expression, I fear. > > > > > > Steve, I was hoping to get a clarification from you about which > > > capability bits had changed. > > > > I am not sure what you are asking of me. > > I'm trying to understand exactly which capability sets should be > compared to determine if there was a material change, but perhaps that > question is better asked of Serge. What does the certification care > about in terms that helps me code this? When its a file system based capabilities and the file has a watch for execute, we want the capabilities. If there is a watch for execute on non- setuid file and it gets ambient capabilities, we want the capabilities. When there's a watch on a setuid root for execute, we only want capabilities when the process does not get full capabilities.. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/commoncap.c b/security/commoncap.c index 14540bd..8f6bedf 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -594,16 +594,17 @@ skip: /* * Audit candidate if current->cap_effective is set * - * We do not bother to audit if 3 things are true: + * We do not bother to audit if 4 things are true: * 1) cap_effective has all caps * 2) we are root * 3) root is supposed to have all caps (SECURE_NOROOT) + * 4) we are running a set*id binary * Since this is just a normal root execing a process. * * Number 1 above might fail if you don't have a full bset, but I think * that is interesting information to audit. */ - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { + if (!is_setid && !cap_issubset(new->cap_effective, new->cap_ambient)) { if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || issecure(SECURE_NOROOT)) {
The audit subsystem is adding a BPRM_FCAPS record when auditing setuid application execution (SYSCALL execve). This is not expected as it was supposed to be limited to when the file system actually had capabilities in an extended attribute. It lists all capabilities making the event really ugly to parse what is happening. The PATH record correctly records the setuid bit and owner. Suppress the BPRM_FCAPS record on set*id. See: https://github.com/linux-audit/audit-kernel/issues/16 Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- security/commoncap.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)