From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Guy Briggs Subject: Re: [PATCH] capabilities: do not audit log BPRM_FCAPS on set*id Date: Tue, 7 Mar 2017 16:10:49 -0500 Message-ID: <20170307211048.GE10258@madcap2.tricolour.ca> References: <515427654218b7ce22441f635115e93cf74d6302.1488491988.git.rgb@redhat.com> <20170303020757.GA9920@mail.hallyn.com> <20170303025014.GG18258@madcap2.tricolour.ca> <20170307172227.GM3818@madcap2.tricolour.ca> <20170307181049.GA31834@mail.hallyn.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170307181049.GA31834@mail.hallyn.com> Sender: owner-linux-security-module@vger.kernel.org To: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org, linux-audit@redhat.com, Andy Lutomirski , "Serge E. Hallyn" , Kees Cook , James Morris , Eric Paris , Paul Moore , Steve Grubb List-Id: linux-audit@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. 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 > > > > > --- > > > > > 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 > > Kernel Security Engineering, Base Operating Systems, Red Hat > > Remote, Ottawa, Canada > > Voice: +1.647.777.2635, Internal: (81) 32635 - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635