Re: [RFC PATCH v3] security, capability: pass object information to security_capable

From: Paul Moore <paul@paul-moore.com>
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: James Morris <jmorris@namei.org>,
	Aaron Goidel <acgoide@tycho.nsa.gov>,
	rgb@redhat.com, mortonm@chromium.org,
	john.johansen@canonical.com, selinux@vger.kernel.org,
	luto@amacapital.net, linux-security-module@vger.kernel.org,
	linux-audit@redhat.com, nhfran2@tycho.nsa.gov,
	Serge Hallyn <serge@hallyn.com>
Subject: Re: [RFC PATCH v3] security, capability: pass object information to security_capable
Date: Fri, 16 Aug 2019 12:36:13 -0400	[thread overview]
Message-ID: <CAHC9VhRLnUO_iiz31z=7wiHf2sNihC7mmi3FhaPCqmW=xt+tRw@mail.gmail.com> (raw)
In-Reply-To: <cebacde0-5c53-c414-8f27-8d81ed928dfd@tycho.nsa.gov>

On Fri, Aug 16, 2019 at 10:57 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 8/15/19 6:32 PM, James Morris wrote:
> > On Thu, 15 Aug 2019, Aaron Goidel wrote:
> >
> >> In SELinux this new information is leveraged here to perform an
> >> additional inode based check for capabilities relevant to inodes. Since
> >> the inode provided to capable_wrt_inode_uidgid() is a const argument,
> >> this also required propagating const down to dump_common_audit_data() and
> >> dropping the use of d_find_alias() to find an alias for the inode. This
> >> was sketchy to begin with and should be obsoleted by a separate change
> >> that will allow LSMs to trigger audit collection for all file-related
> >> information.
> >
> > Will the audit logs look the same once the 2nd patch is applied? We need
> > to be careful about breaking existing userland.
>
> It was already the case that the name= field in the AVC audit record was
> not guaranteed to be emitted in this case, since d_find_alias could
> return NULL.  And it was only a hint, since that name might have nothing
> to do with the name used to look up the inode in the first place. So I
> don't believe userland could have ever relied upon it being present
> here.  Removing it also fixes a problem with AVC audit generation under
> RCU walk; we should be able to drop the code that skips audit generation
> in that case with this d_find_alias call gone IIUC.
>
> With the ability for an LSM to enable collection and generation of
> AUDIT_PATH and other AUDIT_* records (which is made possible via the
> other patch), we will get more complete and relevant information in the
> audit log.  It won't look exactly the same (there will be separate AVC,
> PATH, ... records that can be correlated based on timestamp/serial and
> ausearch does this automatically for you).

Regardless of if it is The Right Thing, changes like this should
probably be put into a separate, unrelated patch.

I think there are a few things in dump_common_audit_data() that should
have been done differently, but unfortunately the audit records (and
IMHO the many stupid design decisions that went into them) are
effectively part of the kernel API and need to be treated with care.

-- 
paul moore
www.paul-moore.com