diff mbox series

[2/3] Teach SELinux about anonymous inodes

Message ID 20200214032635.75434-3-dancol@google.com (mailing list archive)
State Superseded
Headers show
Series [1/3] Add a new LSM-supporting anonymous inode interface | expand

Commit Message

Daniel Colascione Feb. 14, 2020, 3:26 a.m. UTC
This change uses the anon_inodes and LSM infrastructure introduced in
the previous patch to give SELinux the ability to control
anonymous-inode files that are created using the new _secure()
anon_inodes functions.

A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".

Example:

type uffd_t;
type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:file { create };

(The next patch in this series is necessary for making userfaultfd
support this new interface.  The example above is just
for exposition.)

Signed-off-by: Daniel Colascione <dancol@google.com>
---
 security/selinux/hooks.c | 57 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

Comments

Stephen Smalley Feb. 14, 2020, 4:39 p.m. UTC | #1
On 2/13/20 10:26 PM, Daniel Colascione wrote:
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1659b59fb5d7..6de0892620b3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2915,6 +2915,62 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>   	return 0;
>   }
>   
> +static int selinux_inode_init_security_anon(struct inode *inode,
> +					    const struct qstr *name,
> +					    const struct file_operations *fops,
> +					    const struct inode *context_inode)
> +{
> +	const struct task_security_struct *tsec = selinux_cred(current_cred());
> +	struct common_audit_data ad;
> +	struct inode_security_struct *isec;
> +	int rc;
> +
> +	if (unlikely(IS_PRIVATE(inode)))
> +		return 0;

This is not possible since the caller clears S_PRIVATE before calling 
and it would be a bug to call the hook on an inode that was intended to 
be private, so we shouldn't check it here.

> +
> +	if (unlikely(!selinux_state.initialized))
> +		return 0;

Are we doing this to defer initialization until selinux_complete_init() 
- that's normally why we bail in the !initialized case?  Not entirely 
sure what will happen in such a situation since we won't have the 
context_inode or the allocating task information at that time, so we 
certainly won't get the same result - probably they would all be labeled 
with whatever anon_inodefs is assigned via genfscon or 
SECINITSID_UNLABELED by default.  If we instead just drop this test and 
proceed, we'll inherit the context inode SID if specified or we'll call 
security_transition_sid(), which in the !initialized case will just 
return the tsid i.e. tsec->sid, so it will be labeled with the creating 
task SID (SECINITSID_KERNEL prior to initialization).  Then the 
avc_has_perm() call will pass because everything gets allowed until 
initialization. So you could drop this check and userfaultfds created 
before policy load would get the kernel SID, or you can keep it and they 
will get the unlabeled SID.  Preference?

> +
> +	isec = selinux_inode(inode);
> +
> +	/*
> +	 * We only get here once per ephemeral inode.  The inode has
> +	 * been initialized via inode_alloc_security but is otherwise
> +	 * untouched.
> +	 */
> +
> +	if (context_inode) {
> +		struct inode_security_struct *context_isec =
> +			selinux_inode(context_inode);
> +		if (IS_ERR(context_isec))
> +			return PTR_ERR(context_isec);

This isn't possible AFAICT so you don't need to test for it or handle 
it.  In fact, even the test for NULL in selinux_inode() is bogus and 
should get dropped AFAICT; we always allocate an inode security blob 
even before policy load so it would be a bug if we ever had a NULL there.

> +		isec->sid = context_isec->sid;
> +	} else {
> +		rc = security_transition_sid(
> +			&selinux_state, tsec->sid, tsec->sid,
> +			SECCLASS_FILE, name, &isec->sid);
> +		if (rc)
> +			return rc;
> +	}

Since you switched to using security_transition_sid(), you are not using 
the fops parameter anymore nor comparing with userfaultfd_fops, so you 
could drop the parameter from the hook and leave the latter static in 
the first patch.
That's assuming you are ok with having to define these type_transition 
rules for the userfaultfd case instead of having your own separate 
security class.  Wondering how many different anon inode names/classes 
there are in the kernel today and how much they change over time; for a 
small, relatively stable set, separate classes might be ok; for a large, 
dynamic set, type transitions should scale better.  We might still need 
to create a mapping table in SELinux from the names to some stable 
identifier for the policy lookup if we can't rely on the names being stable.
Daniel Colascione Feb. 14, 2020, 5:21 p.m. UTC | #2
On Fri, Feb 14, 2020 at 8:38 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 2/13/20 10:26 PM, Daniel Colascione wrote:
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1659b59fb5d7..6de0892620b3 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2915,6 +2915,62 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> >       return 0;
> >   }
> >
> > +static int selinux_inode_init_security_anon(struct inode *inode,
> > +                                         const struct qstr *name,
> > +                                         const struct file_operations *fops,
> > +                                         const struct inode *context_inode)
> > +{
> > +     const struct task_security_struct *tsec = selinux_cred(current_cred());
> > +     struct common_audit_data ad;
> > +     struct inode_security_struct *isec;
> > +     int rc;
> > +
> > +     if (unlikely(IS_PRIVATE(inode)))
> > +             return 0;
>
> This is not possible since the caller clears S_PRIVATE before calling
> and it would be a bug to call the hook on an inode that was intended to
> be private, so we shouldn't check it here.
>
> > +
> > +     if (unlikely(!selinux_state.initialized))
> > +             return 0;
>
> Are we doing this to defer initialization until selinux_complete_init()
> - that's normally why we bail in the !initialized case?  Not entirely
> sure what will happen in such a situation since we won't have the
> context_inode or the allocating task information at that time, so we
> certainly won't get the same result - probably they would all be labeled
> with whatever anon_inodefs is assigned via genfscon or
> SECINITSID_UNLABELED by default.
> If we instead just drop this test and
> proceed, we'll inherit the context inode SID if specified or we'll call
> security_transition_sid(), which in the !initialized case will just
> return the tsid i.e. tsec->sid, so it will be labeled with the creating
> task SID (SECINITSID_KERNEL prior to initialization).  Then the
> avc_has_perm() call will pass because everything gets allowed until
> initialization. So you could drop this check and userfaultfds created
> before policy load would get the kernel SID, or you can keep it and they
> will get the unlabeled SID.  Preference?

The kernel SID seems safer. Thanks for the explanation!

> > +
> > +     isec = selinux_inode(inode);
> > +
> > +     /*
> > +      * We only get here once per ephemeral inode.  The inode has
> > +      * been initialized via inode_alloc_security but is otherwise
> > +      * untouched.
> > +      */
> > +
> > +     if (context_inode) {
> > +             struct inode_security_struct *context_isec =
> > +                     selinux_inode(context_inode);
> > +             if (IS_ERR(context_isec))
> > +                     return PTR_ERR(context_isec);
>
> This isn't possible AFAICT so you don't need to test for it or handle
> it.  In fact, even the test for NULL in selinux_inode() is bogus and
> should get dropped AFAICT; we always allocate an inode security blob
> even before policy load so it would be a bug if we ever had a NULL there.

Thanks. Will fix.

> > +             isec->sid = context_isec->sid;
> > +     } else {
> > +             rc = security_transition_sid(
> > +                     &selinux_state, tsec->sid, tsec->sid,
> > +                     SECCLASS_FILE, name, &isec->sid);
> > +             if (rc)
> > +                     return rc;
> > +     }
>
> Since you switched to using security_transition_sid(), you are not using
> the fops parameter anymore nor comparing with userfaultfd_fops, so you
> could drop the parameter from the hook and leave the latter static in
> the first patch.

That's true, but I figured different LSMs might want different rules
that depend on the fops. I'm also okay with removing this parameter
for now, since we're not using it.

> That's assuming you are ok with having to define these type_transition
> rules for the userfaultfd case instead of having your own separate
> security class.  Wondering how many different anon inode names/classes
> there are in the kernel today and how much they change over time; for a
> small, relatively stable set, separate classes might be ok; for a large,
> dynamic set, type transitions should scale better.

I think we can get away without a class per anonymous-inode-type. I do
wonder whether we need a class for all anonymous inodes, though: if we
just give them the file class and use the anon inode type name for the
type_transition rule, isn't it possible that the type_transition rule
might also fire for plain files with the same names in the last path
component and the same originating sid? (Maybe I'm not understanding
type_transition rules properly.) Using a class to encompass all
anonymous inodes would address this problem (assuming the problem
exists in the first place).

> We might still need
> to create a mapping table in SELinux from the names to some stable
> identifier for the policy lookup if we can't rely on the names being stable.

Sure. The anonymous inode type names have historically been stable,
though, so maybe we can just use the names from anon_inodes directly
for now and then add some kind of remapping if we want to change those
names in the core, remaping to the old name for SELinux
type_transition purposes.
Stephen Smalley Feb. 14, 2020, 6:02 p.m. UTC | #3
On 2/14/20 12:21 PM, Daniel Colascione wrote:
> On Fri, Feb 14, 2020 at 8:38 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> That's assuming you are ok with having to define these type_transition
>> rules for the userfaultfd case instead of having your own separate
>> security class.  Wondering how many different anon inode names/classes
>> there are in the kernel today and how much they change over time; for a
>> small, relatively stable set, separate classes might be ok; for a large,
>> dynamic set, type transitions should scale better.
> 
> I think we can get away without a class per anonymous-inode-type. I do
> wonder whether we need a class for all anonymous inodes, though: if we
> just give them the file class and use the anon inode type name for the
> type_transition rule, isn't it possible that the type_transition rule
> might also fire for plain files with the same names in the last path
> component and the same originating sid? (Maybe I'm not understanding
> type_transition rules properly.) Using a class to encompass all
> anonymous inodes would address this problem (assuming the problem
> exists in the first place).

It shouldn't fire for non-anon inodes because on a (non-anon) file 
creation, security_transition_sid() is passed the parent directory SID 
as the second argument and we only assign task SIDs to /proc/pid 
directories, which don't support (userspace) file creation anyway.

However, in the absence of a matching type_transition rule, we'll end up 
defaulting to the task SID on the anon inode, and without a separate 
class we won't be able to distinguish it from a /proc/pid inode.  So 
that might justify a separate anoninode or similar class.

This however reminded me that for the context_inode case, we not only 
want to inherit the SID but also the sclass from the context_inode. 
That is so that anon inodes created via device node ioctls inherit the 
same SID/class pair as the device node and a single allowx rule can 
govern all ioctl commands on that device.
Stephen Smalley Feb. 14, 2020, 6:08 p.m. UTC | #4
On 2/14/20 1:02 PM, Stephen Smalley wrote:
> It shouldn't fire for non-anon inodes because on a (non-anon) file 
> creation, security_transition_sid() is passed the parent directory SID 
> as the second argument and we only assign task SIDs to /proc/pid 
> directories, which don't support (userspace) file creation anyway.
> 
> However, in the absence of a matching type_transition rule, we'll end up 
> defaulting to the task SID on the anon inode, and without a separate 
> class we won't be able to distinguish it from a /proc/pid inode.  So 
> that might justify a separate anoninode or similar class.
> 
> This however reminded me that for the context_inode case, we not only 
> want to inherit the SID but also the sclass from the context_inode. That 
> is so that anon inodes created via device node ioctls inherit the same 
> SID/class pair as the device node and a single allowx rule can govern 
> all ioctl commands on that device.

At least that's the way our patch worked with the /dev/kvm example. 
However, if we are introducing a separate anoninode class for the 
type_transition case, maybe we should apply that to all anon inodes 
regardless of how they are labeled (based on context_inode or 
transition) and then we'd need to write two allowx rules, one for ioctls 
on the original device node and one for those on anon inodes created 
from it.  Not sure how Android wants to handle that as the original 
developer and primary user of SELinux ioctl whitelisting.
Stephen Smalley Feb. 14, 2020, 8:24 p.m. UTC | #5
On 2/14/20 1:08 PM, Stephen Smalley wrote:
> On 2/14/20 1:02 PM, Stephen Smalley wrote:
>> It shouldn't fire for non-anon inodes because on a (non-anon) file 
>> creation, security_transition_sid() is passed the parent directory SID 
>> as the second argument and we only assign task SIDs to /proc/pid 
>> directories, which don't support (userspace) file creation anyway.
>>
>> However, in the absence of a matching type_transition rule, we'll end 
>> up defaulting to the task SID on the anon inode, and without a 
>> separate class we won't be able to distinguish it from a /proc/pid 
>> inode.  So that might justify a separate anoninode or similar class.
>>
>> This however reminded me that for the context_inode case, we not only 
>> want to inherit the SID but also the sclass from the context_inode. 
>> That is so that anon inodes created via device node ioctls inherit the 
>> same SID/class pair as the device node and a single allowx rule can 
>> govern all ioctl commands on that device.
> 
> At least that's the way our patch worked with the /dev/kvm example. 
> However, if we are introducing a separate anoninode class for the 
> type_transition case, maybe we should apply that to all anon inodes 
> regardless of how they are labeled (based on context_inode or 
> transition) and then we'd need to write two allowx rules, one for ioctls 
> on the original device node and one for those on anon inodes created 
> from it.  Not sure how Android wants to handle that as the original 
> developer and primary user of SELinux ioctl whitelisting.

I would tentatively argue for inheriting both sclass and SID from the 
context_inode for the sake of sane policy writing.  In the userfaultfd 
case, that will still end up using the new SECCLASS_ANONINODE or 
whatever since the sclass will be initially set to that value for the 
transition SID case and then inherited on fork.  But for /dev/kvm, it 
would be the class from the /dev/kvm inode, i.e. SECCLASS_CHR_FILE.
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1659b59fb5d7..6de0892620b3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2915,6 +2915,62 @@  static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	return 0;
 }
 
+static int selinux_inode_init_security_anon(struct inode *inode,
+					    const struct qstr *name,
+					    const struct file_operations *fops,
+					    const struct inode *context_inode)
+{
+	const struct task_security_struct *tsec = selinux_cred(current_cred());
+	struct common_audit_data ad;
+	struct inode_security_struct *isec;
+	int rc;
+
+	if (unlikely(IS_PRIVATE(inode)))
+		return 0;
+
+	if (unlikely(!selinux_state.initialized))
+		return 0;
+
+	isec = selinux_inode(inode);
+
+	/*
+	 * We only get here once per ephemeral inode.  The inode has
+	 * been initialized via inode_alloc_security but is otherwise
+	 * untouched.
+	 */
+
+	if (context_inode) {
+		struct inode_security_struct *context_isec =
+			selinux_inode(context_inode);
+		if (IS_ERR(context_isec))
+			return PTR_ERR(context_isec);
+		isec->sid = context_isec->sid;
+	} else {
+		rc = security_transition_sid(
+			&selinux_state, tsec->sid, tsec->sid,
+			SECCLASS_FILE, name, &isec->sid);
+		if (rc)
+			return rc;
+	}
+
+	isec->initialized = LABEL_INITIALIZED;
+
+	/*
+	 * Now that we've initialized security, check whether we're
+	 * allowed to actually create this type of anonymous inode.
+	 */
+
+	ad.type = LSM_AUDIT_DATA_INODE;
+	ad.u.inode = inode;
+
+	return avc_has_perm(&selinux_state,
+			    tsec->sid,
+			    isec->sid,
+			    isec->sclass,
+			    FILE__CREATE,
+			    &ad);
+}
+
 static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
 	return may_create(dir, dentry, SECCLASS_FILE);
@@ -6923,6 +6979,7 @@  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 
 	LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
 	LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
+	LSM_HOOK_INIT(inode_init_security_anon, selinux_inode_init_security_anon),
 	LSM_HOOK_INIT(inode_create, selinux_inode_create),
 	LSM_HOOK_INIT(inode_link, selinux_inode_link),
 	LSM_HOOK_INIT(inode_unlink, selinux_inode_unlink),