| Message ID | 20201112015359.1103333-4-lokeshgidra@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | SELinux support for anonymous inodes and UFFD | expand |
On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > From: Daniel Colascione <dancol@google.com> > > This change uses the anon_inodes and LSM infrastructure introduced in > the previous patches to give SELinux the ability to control > anonymous-inode files that are created using the new > anon_inode_getfd_secure() function. > > 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 : anon_inode uffd_t "[userfaultfd]"; > allow sysadm_t uffd_t:anon_inode { 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> > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > --- > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > security/selinux/include/classmap.h | 2 ++ > 2 files changed, 58 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 6b1826fc3658..d092aa512868 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > + 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 (context_isec->initialized != LABEL_INITIALIZED) > + return -EACCES; > + > + isec->sclass = context_isec->sclass; Taking the object class directly from the context_inode is interesting, and I suspect problematic. In the case below where no context_inode is supplied the object class is set to SECCLASS_ANON_INODE, which is correct, but when a context_inode is supplied there is no guarantee that the object class will be set to SECCLASS_ANON_INODE. This could both pose a problem for policy writers (how do you distinguish the anon inode from other normal file inodes in this case?) as well as an outright fault later in this function when we try to check the ANON_INODE__CREATE on an object other than a SECCLASS_ANON_INODE object. It works in the userfaultfd case because the context_inode is originally created with this function so the object class is correctly set to SECCLASS_ANON_INODE, but can we always guarantee that to be the case? Do we ever need or want to support using a context_inode that is not SECCLASS_ANON_INODE? > + isec->sid = context_isec->sid; > + } else { > + isec->sclass = SECCLASS_ANON_INODE; > + rc = security_transition_sid( > + &selinux_state, tsec->sid, tsec->sid, > + isec->sclass, 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, > + ANON_INODE__CREATE, > + &ad); > +}
On Wed, Jan 6, 2021 at 7:03 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > From: Daniel Colascione <dancol@google.com> > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > the previous patches to give SELinux the ability to control > > anonymous-inode files that are created using the new > > anon_inode_getfd_secure() function. > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > allow sysadm_t uffd_t:anon_inode { 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> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > --- > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > security/selinux/include/classmap.h | 2 ++ > > 2 files changed, 58 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 6b1826fc3658..d092aa512868 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > + return -EACCES; > > + > > + isec->sclass = context_isec->sclass; > > Taking the object class directly from the context_inode is > interesting, and I suspect problematic. In the case below where no > context_inode is supplied the object class is set to > SECCLASS_ANON_INODE, which is correct, but when a context_inode is > supplied there is no guarantee that the object class will be set to > SECCLASS_ANON_INODE. This could both pose a problem for policy > writers (how do you distinguish the anon inode from other normal file > inodes in this case?) as well as an outright fault later in this > function when we try to check the ANON_INODE__CREATE on an object > other than a SECCLASS_ANON_INODE object. > Thanks for catching this. I'll initialize 'sclass' unconditionally to SECCLASS_ANON_INODE in the next version. Also, do you think I should add a check that context_inode's sclass must be SECCLASS_ANON_INODE to confirm that we never receive a regular inode as context_inode? > It works in the userfaultfd case because the context_inode is > originally created with this function so the object class is correctly > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the > case? Do we ever need or want to support using a context_inode that > is not SECCLASS_ANON_INODE? > I don't think there is any requirement of supporting context_inode which isn't anon-inode. And even if there is, as you described earlier, for ANON_INODE__CREATE to work the sclass has to be SECCLASS_ANON_INODE. I'll appreciate comments on this from others, particularly Daniel and Stephen who originally discussed and implemented this patch. > > + isec->sid = context_isec->sid; > > + } else { > > + isec->sclass = SECCLASS_ANON_INODE; > > + rc = security_transition_sid( > > + &selinux_state, tsec->sid, tsec->sid, > > + isec->sclass, 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, > > + ANON_INODE__CREATE, > > + &ad); > > +} > > -- > paul moore > www.paul-moore.com
On Wed, Jan 6, 2021 at 10:55 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > On Wed, Jan 6, 2021 at 7:03 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > From: Daniel Colascione <dancol@google.com> > > > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > > the previous patches to give SELinux the ability to control > > > anonymous-inode files that are created using the new > > > anon_inode_getfd_secure() function. > > > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > > allow sysadm_t uffd_t:anon_inode { 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> > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > > --- > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > > security/selinux/include/classmap.h | 2 ++ > > > 2 files changed, 58 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 6b1826fc3658..d092aa512868 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > > + return -EACCES; > > > + > > > + isec->sclass = context_isec->sclass; > > > > Taking the object class directly from the context_inode is > > interesting, and I suspect problematic. In the case below where no > > context_inode is supplied the object class is set to > > SECCLASS_ANON_INODE, which is correct, but when a context_inode is > > supplied there is no guarantee that the object class will be set to > > SECCLASS_ANON_INODE. This could both pose a problem for policy > > writers (how do you distinguish the anon inode from other normal file > > inodes in this case?) as well as an outright fault later in this > > function when we try to check the ANON_INODE__CREATE on an object > > other than a SECCLASS_ANON_INODE object. > > > Thanks for catching this. I'll initialize 'sclass' unconditionally to > SECCLASS_ANON_INODE in the next version. Also, do you think I should > add a check that context_inode's sclass must be SECCLASS_ANON_INODE to > confirm that we never receive a regular inode as context_inode? This is one of the reasons why I was asking if you ever saw the need to use a regular inode here. It seems much safer to me to add a check to ensure that context_inode is SECCLASS_ANON_INODE and return an error otherwise; I would also suggest emitting an error using pr_err() with something along the lines of "SELinux: initializing anonymous inode with inappropriate inode" (or something similar). If something changes in the future we can always reconsider this restriction. > > It works in the userfaultfd case because the context_inode is > > originally created with this function so the object class is correctly > > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the > > case? Do we ever need or want to support using a context_inode that > > is not SECCLASS_ANON_INODE? > > I don't think there is any requirement of supporting context_inode > which isn't anon-inode. And even if there is, as you described > earlier, for ANON_INODE__CREATE to work the sclass has to be > SECCLASS_ANON_INODE. I'll appreciate comments on this from others, > particularly Daniel and Stephen who originally discussed and > implemented this patch. I would encourage you not to wait too long for additional feedback before sending the next revision.
On Thu, Jan 7, 2021 at 2:30 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jan 6, 2021 at 10:55 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > On Wed, Jan 6, 2021 at 7:03 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > > From: Daniel Colascione <dancol@google.com> > > > > > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > > > the previous patches to give SELinux the ability to control > > > > anonymous-inode files that are created using the new > > > > anon_inode_getfd_secure() function. > > > > > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > > > allow sysadm_t uffd_t:anon_inode { 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> > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > > > --- > > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > > > security/selinux/include/classmap.h | 2 ++ > > > > 2 files changed, 58 insertions(+) > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index 6b1826fc3658..d092aa512868 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > > > + return -EACCES; > > > > + > > > > + isec->sclass = context_isec->sclass; > > > > > > Taking the object class directly from the context_inode is > > > interesting, and I suspect problematic. In the case below where no > > > context_inode is supplied the object class is set to > > > SECCLASS_ANON_INODE, which is correct, but when a context_inode is > > > supplied there is no guarantee that the object class will be set to > > > SECCLASS_ANON_INODE. This could both pose a problem for policy > > > writers (how do you distinguish the anon inode from other normal file > > > inodes in this case?) as well as an outright fault later in this > > > function when we try to check the ANON_INODE__CREATE on an object > > > other than a SECCLASS_ANON_INODE object. > > > > > Thanks for catching this. I'll initialize 'sclass' unconditionally to > > SECCLASS_ANON_INODE in the next version. Also, do you think I should > > add a check that context_inode's sclass must be SECCLASS_ANON_INODE to > > confirm that we never receive a regular inode as context_inode? > > This is one of the reasons why I was asking if you ever saw the need > to use a regular inode here. It seems much safer to me to add a check > to ensure that context_inode is SECCLASS_ANON_INODE and return an > error otherwise; I would also suggest emitting an error using pr_err() > with something along the lines of "SELinux: initializing anonymous > inode with inappropriate inode" (or something similar). > Thanks. I'll do that. > If something changes in the future we can always reconsider this restriction. > > > > It works in the userfaultfd case because the context_inode is > > > originally created with this function so the object class is correctly > > > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the > > > case? Do we ever need or want to support using a context_inode that > > > is not SECCLASS_ANON_INODE? > > > > I don't think there is any requirement of supporting context_inode > > which isn't anon-inode. And even if there is, as you described > > earlier, for ANON_INODE__CREATE to work the sclass has to be > > SECCLASS_ANON_INODE. I'll appreciate comments on this from others, > > particularly Daniel and Stephen who originally discussed and > > implemented this patch. > > I would encourage you not to wait too long for additional feedback > before sending the next revision. Certainly. I'll send next version in a day or two. > > -- > paul moore > www.paul-moore.com
On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > From: Daniel Colascione <dancol@google.com> > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > the previous patches to give SELinux the ability to control > > anonymous-inode files that are created using the new > > anon_inode_getfd_secure() function. > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > allow sysadm_t uffd_t:anon_inode { 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> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > --- > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > security/selinux/include/classmap.h | 2 ++ > > 2 files changed, 58 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 6b1826fc3658..d092aa512868 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > + return -EACCES; > > + > > + isec->sclass = context_isec->sclass; > > Taking the object class directly from the context_inode is > interesting, and I suspect problematic. In the case below where no > context_inode is supplied the object class is set to > SECCLASS_ANON_INODE, which is correct, but when a context_inode is > supplied there is no guarantee that the object class will be set to > SECCLASS_ANON_INODE. This could both pose a problem for policy > writers (how do you distinguish the anon inode from other normal file > inodes in this case?) as well as an outright fault later in this > function when we try to check the ANON_INODE__CREATE on an object > other than a SECCLASS_ANON_INODE object. > > It works in the userfaultfd case because the context_inode is > originally created with this function so the object class is correctly > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the > case? Do we ever need or want to support using a context_inode that > is not SECCLASS_ANON_INODE? Sorry, I haven't been following this. IIRC, the original reason for passing a context_inode was to support the /dev/kvm or similar use cases where the driver is creating anonymous inodes to represent specific objects/interfaces derived from the device node and we want to be able to control subsequent ioctl operations on those anonymous inodes in the same manner as for the device node. For example, ioctl operations on /dev/kvm can end up returning file descriptors for anonymous inodes representing a specific VM or VCPU or similar. If we propagate the security class and SID from the /dev/kvm inode (the context inode) to the new anonymous inode, we can write a single policy rule over all ioctl operations related to /dev/kvm. That's also why we used the FILE__CREATE permission here originally; that was also intentional. All the file-related classes including anon_inode inherit a common set of file permissions including create and thus we often use the FILE__<permission> in common code when checking permission against any potentially derived class.
On Fri, Jan 8, 2021 at 11:35 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > From: Daniel Colascione <dancol@google.com> > > > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > > the previous patches to give SELinux the ability to control > > > anonymous-inode files that are created using the new > > > anon_inode_getfd_secure() function. > > > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > > allow sysadm_t uffd_t:anon_inode { 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> > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > > --- > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > > security/selinux/include/classmap.h | 2 ++ > > > 2 files changed, 58 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 6b1826fc3658..d092aa512868 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > > + return -EACCES; Stephen, as per your explanation below, is this check also problematic? I mean is it possible that /dev/kvm context_inode may not have its label initialized? If so, then v12 of the patch series can be used as is. Otherwise, I will send the next version which rollbacks v14 and v13, except for this check. Kindly confirm. > > > + > > > + isec->sclass = context_isec->sclass; > > > > Taking the object class directly from the context_inode is > > interesting, and I suspect problematic. In the case below where no > > context_inode is supplied the object class is set to > > SECCLASS_ANON_INODE, which is correct, but when a context_inode is > > supplied there is no guarantee that the object class will be set to > > SECCLASS_ANON_INODE. This could both pose a problem for policy > > writers (how do you distinguish the anon inode from other normal file > > inodes in this case?) as well as an outright fault later in this > > function when we try to check the ANON_INODE__CREATE on an object > > other than a SECCLASS_ANON_INODE object. > > > > It works in the userfaultfd case because the context_inode is > > originally created with this function so the object class is correctly > > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the > > case? Do we ever need or want to support using a context_inode that > > is not SECCLASS_ANON_INODE? > > Sorry, I haven't been following this. IIRC, the original reason for > passing a context_inode was to support the /dev/kvm or similar use > cases where the driver is creating anonymous inodes to represent > specific objects/interfaces derived from the device node and we want > to be able to control subsequent ioctl operations on those anonymous > inodes in the same manner as for the device node. For example, ioctl > operations on /dev/kvm can end up returning file descriptors for > anonymous inodes representing a specific VM or VCPU or similar. If we > propagate the security class and SID from the /dev/kvm inode (the > context inode) to the new anonymous inode, we can write a single > policy rule over all ioctl operations related to /dev/kvm. That's > also why we used the FILE__CREATE permission here originally; that was > also intentional. All the file-related classes including anon_inode > inherit a common set of file permissions including create and thus we > often use the FILE__<permission> in common code when checking > permission against any potentially derived class. Thanks a lot for the explanation.
On Fri, Jan 8, 2021 at 2:35 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > From: Daniel Colascione <dancol@google.com> > > > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > > the previous patches to give SELinux the ability to control > > > anonymous-inode files that are created using the new > > > anon_inode_getfd_secure() function. > > > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > > allow sysadm_t uffd_t:anon_inode { 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> > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > > --- > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > > security/selinux/include/classmap.h | 2 ++ > > > 2 files changed, 58 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 6b1826fc3658..d092aa512868 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > > + return -EACCES; > > > + > > > + isec->sclass = context_isec->sclass; > > > > Taking the object class directly from the context_inode is > > interesting, and I suspect problematic. In the case below where no > > context_inode is supplied the object class is set to > > SECCLASS_ANON_INODE, which is correct, but when a context_inode is > > supplied there is no guarantee that the object class will be set to > > SECCLASS_ANON_INODE. This could both pose a problem for policy > > writers (how do you distinguish the anon inode from other normal file > > inodes in this case?) as well as an outright fault later in this > > function when we try to check the ANON_INODE__CREATE on an object > > other than a SECCLASS_ANON_INODE object. > > > > It works in the userfaultfd case because the context_inode is > > originally created with this function so the object class is correctly > > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the > > case? Do we ever need or want to support using a context_inode that > > is not SECCLASS_ANON_INODE? > > Sorry, I haven't been following this. IIRC, the original reason for > passing a context_inode was to support the /dev/kvm or similar use > cases where the driver is creating anonymous inodes to represent > specific objects/interfaces derived from the device node and we want > to be able to control subsequent ioctl operations on those anonymous > inodes in the same manner as for the device node. For example, ioctl > operations on /dev/kvm can end up returning file descriptors for > anonymous inodes representing a specific VM or VCPU or similar. If we > propagate the security class and SID from the /dev/kvm inode (the > context inode) to the new anonymous inode, we can write a single > policy rule over all ioctl operations related to /dev/kvm. Thanks for the background, and the /dev/kvm example, that is what I was missing. > That's > also why we used the FILE__CREATE permission here originally; that was > also intentional. All the file-related classes including anon_inode > inherit a common set of file permissions including create and thus we > often use the FILE__<permission> in common code when checking > permission against any potentially derived class. Yes, if all of the anonymous inodes are not going to fall into the anon_inode object class then FILE__CREATE makes the most sense. Thanks Stephen.
On Fri, Jan 8, 2021 at 3:17 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > On Fri, Jan 8, 2021 at 11:35 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > > From: Daniel Colascione <dancol@google.com> > > > > > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > > > the previous patches to give SELinux the ability to control > > > > anonymous-inode files that are created using the new > > > > anon_inode_getfd_secure() function. > > > > > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > > > allow sysadm_t uffd_t:anon_inode { 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> > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > > > --- > > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > > > security/selinux/include/classmap.h | 2 ++ > > > > 2 files changed, 58 insertions(+) > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index 6b1826fc3658..d092aa512868 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > > > + return -EACCES; > Stephen, as per your explanation below, is this check also > problematic? I mean is it possible that /dev/kvm context_inode may not > have its label initialized? If so, then v12 of the patch series can be > used as is. Otherwise, I will send the next version which rollbacks > v14 and v13, except for this check. Kindly confirm. The context_inode should always be initialized already. I'm not fond though of silently returning -EACCES here. At the least we should have a pr_err() or pr_warn() here. In reality, this could only occur in the case of a kernel bug or memory corruption so it used to be a candidate for WARN_ON() or BUG_ON() or similar but I know that BUG_ON() at least is frowned upon these days.
On Fri, Jan 8, 2021 at 1:24 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Fri, Jan 8, 2021 at 3:17 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > > On Fri, Jan 8, 2021 at 11:35 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > > > > On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote: > > > > > From: Daniel Colascione <dancol@google.com> > > > > > > > > > > This change uses the anon_inodes and LSM infrastructure introduced in > > > > > the previous patches to give SELinux the ability to control > > > > > anonymous-inode files that are created using the new > > > > > anon_inode_getfd_secure() function. > > > > > > > > > > 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 : anon_inode uffd_t "[userfaultfd]"; > > > > > allow sysadm_t uffd_t:anon_inode { 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> > > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > > > > > --- > > > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++ > > > > > security/selinux/include/classmap.h | 2 ++ > > > > > 2 files changed, 58 insertions(+) > > > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > > index 6b1826fc3658..d092aa512868 100644 > > > > > --- a/security/selinux/hooks.c > > > > > +++ b/security/selinux/hooks.c > > > > > @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) > > > > > + 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 (context_isec->initialized != LABEL_INITIALIZED) > > > > > + return -EACCES; > > Stephen, as per your explanation below, is this check also > > problematic? I mean is it possible that /dev/kvm context_inode may not > > have its label initialized? If so, then v12 of the patch series can be > > used as is. Otherwise, I will send the next version which rollbacks > > v14 and v13, except for this check. Kindly confirm. > > The context_inode should always be initialized already. I'm not fond > though of silently returning -EACCES here. At the least we should > have a pr_err() or pr_warn() here. In reality, this could only occur > in the case of a kernel bug or memory corruption so it used to be a > candidate for WARN_ON() or BUG_ON() or similar but I know that > BUG_ON() at least is frowned upon these days. Got it. I'll add a pr_err(). Thanks a lot.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 6b1826fc3658..d092aa512868 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2927,6 +2927,61 @@ 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 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(!selinux_initialized(&selinux_state))) + 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 (context_isec->initialized != LABEL_INITIALIZED) + return -EACCES; + + isec->sclass = context_isec->sclass; + isec->sid = context_isec->sid; + } else { + isec->sclass = SECCLASS_ANON_INODE; + rc = security_transition_sid( + &selinux_state, tsec->sid, tsec->sid, + isec->sclass, 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, + ANON_INODE__CREATE, + &ad); +} + static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode) { return may_create(dir, dentry, SECCLASS_FILE); @@ -6992,6 +7047,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), diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 40cebde62856..ba2e01a6955c 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -249,6 +249,8 @@ struct security_class_mapping secclass_map[] = { {"open", "cpu", "kernel", "tracepoint", "read", "write"} }, { "lockdown", { "integrity", "confidentiality", NULL } }, + { "anon_inode", + { COMMON_FILE_PERMS, NULL } }, { NULL } };