| Message ID | 873771ipib.fsf_-_@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Eric W. Biederman (ebiederm@xmission.com): > -UID: 28009 > Status: O > > > When selinux is loaded the relax permission checks for writing > security.capable are not honored. Which keeps file capabilities > from being used in user namespaces. > > Stephen Smalley <sds@tycho.nsa.gov> writes: > > Originally SELinux called the cap functions directly since there was no > > stacking support in the infrastructure and one had to manually stack a > > secondary module internally. inode_setxattr and inode_removexattr > > however were special cases because the cap functions would check > > CAP_SYS_ADMIN for any non-capability attributes in the security.* > > namespace, and we don't want to impose that requirement on setting > > security.selinux. Thus, we inlined the capabilities logic into the > > selinux hook functions and adapted it appropriately. > > Now that the permission checks in commoncap have evolved this > inlining of their contents has become a problem. So restructure > selinux_inode_removexattr, and selinux_inode_setxattr to call > both the corresponding cap_inode_ function and dentry_has_perm > when the attribute is not a selinux security xattr. This ensures > the policies of both commoncap and selinux are enforced. > > This results in smack and selinux having the same basic structure > for setxattr and removexattr. Performing their own special permission > checks when it is their modules xattr being written to, and deferring > to commoncap when that is not the case. Then finally performing their > generic module policy on all xattr writes. > > This structure is fine when you only consider stacking with the > commoncap lsm, but it becomes a problem if two lsms that don't want > the commoncap security checks on their own attributes need to be > stack. This means there will need to be updates in the future as lsm > stacking is improved, but at least now the structure between smack and > selinux is common making the code easier to refactor. > > This change also has the effect that selinux_linux_setotherxattr becomes > unnecessary so it is removed. > > Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") > Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") > Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> This is hairy, but it looks right: Reviewed-by: Serge Hallyn <serge@hallyn.com> thanks, -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 Mon, 2017-10-02 at 09:38 -0500, Eric W. Biederman wrote: > When selinux is loaded the relax permission checks for writing > security.capable are not honored. Which keeps file capabilities > from being used in user namespaces. > > Stephen Smalley <sds@tycho.nsa.gov> writes: > > Originally SELinux called the cap functions directly since there > > was no > > stacking support in the infrastructure and one had to manually > > stack a > > secondary module internally. inode_setxattr and inode_removexattr > > however were special cases because the cap functions would check > > CAP_SYS_ADMIN for any non-capability attributes in the security.* > > namespace, and we don't want to impose that requirement on setting > > security.selinux. Thus, we inlined the capabilities logic into the > > selinux hook functions and adapted it appropriately. > > Now that the permission checks in commoncap have evolved this > inlining of their contents has become a problem. So restructure > selinux_inode_removexattr, and selinux_inode_setxattr to call > both the corresponding cap_inode_ function and dentry_has_perm > when the attribute is not a selinux security xattr. This ensures > the policies of both commoncap and selinux are enforced. > > This results in smack and selinux having the same basic structure > for setxattr and removexattr. Performing their own special > permission > checks when it is their modules xattr being written to, and deferring > to commoncap when that is not the case. Then finally performing > their > generic module policy on all xattr writes. > > This structure is fine when you only consider stacking with the > commoncap lsm, but it becomes a problem if two lsms that don't want > the commoncap security checks on their own attributes need to be > stack. This means there will need to be updates in the future as lsm > stacking is improved, but at least now the structure between smack > and > selinux is common making the code easier to refactor. > > This change also has the effect that selinux_linux_setotherxattr > becomes > unnecessary so it is removed. > > Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") > Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") > Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx > /history.git > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > > While this fixes some things this isn't a regression so it should be > able to wait until the next merge window to be merged. Would you > like > to take this through the selinux tree? Or shall I take it through > mine? Deferring to Paul Moore on this, since he maintains the selinux tree. > > security/selinux/hooks.c | 43 ++++++++++++++++++--------------------- > ---- > 1 file changed, 18 insertions(+), 25 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index f5d304736852..c78dbec627f6 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3124,27 +3124,6 @@ static int selinux_inode_getattr(const struct > path *path) > return path_has_perm(current_cred(), path, FILE__GETATTR); > } > > -static int selinux_inode_setotherxattr(struct dentry *dentry, const > char *name) > -{ > - const struct cred *cred = current_cred(); > - > - if (!strncmp(name, XATTR_SECURITY_PREFIX, > - sizeof XATTR_SECURITY_PREFIX - 1)) { > - if (!strcmp(name, XATTR_NAME_CAPS)) { > - if (!capable(CAP_SETFCAP)) > - return -EPERM; > - } else if (!capable(CAP_SYS_ADMIN)) { > - /* A different attribute in the security > namespace. > - Restrict to administrator. */ > - return -EPERM; > - } > - } > - > - /* Not an attribute we recognize, so just check the > - ordinary setattr permission. */ > - return dentry_has_perm(cred, dentry, FILE__SETATTR); > -} > - > static bool has_cap_mac_admin(bool audit) > { > const struct cred *cred = current_cred(); > @@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct > dentry *dentry, const char *name, > u32 newsid, sid = current_sid(); > int rc = 0; > > - if (strcmp(name, XATTR_NAME_SELINUX)) > - return selinux_inode_setotherxattr(dentry, name); > + if (strcmp(name, XATTR_NAME_SELINUX)) { > + rc = cap_inode_setxattr(dentry, name, value, size, > flags); > + if (rc) > + return rc; > + > + /* Not an attribute we recognize, so just check the > + ordinary setattr permission. */ > + return dentry_has_perm(current_cred(), dentry, > FILE__SETATTR); > + } > > sbsec = inode->i_sb->s_security; > if (!(sbsec->flags & SBLABEL_MNT)) > @@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct > dentry *dentry) > > static int selinux_inode_removexattr(struct dentry *dentry, const > char *name) > { > - if (strcmp(name, XATTR_NAME_SELINUX)) > - return selinux_inode_setotherxattr(dentry, name); > + if (strcmp(name, XATTR_NAME_SELINUX)) { > + int rc = cap_inode_removexattr(dentry, name); > + if (rc) > + return rc; > + > + /* Not an attribute we recognize, so just check the > + ordinary setattr permission. */ > + return dentry_has_perm(current_cred(), dentry, > FILE__SETATTR); > + } > > /* No one is allowed to remove a SELinux security label. > You can change the label, but all data must be labeled. > */ -- 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 Mon, Oct 2, 2017 at 10:38 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > When selinux is loaded the relax permission checks for writing > security.capable are not honored. Which keeps file capabilities > from being used in user namespaces. > > Stephen Smalley <sds@tycho.nsa.gov> writes: >> Originally SELinux called the cap functions directly since there was no >> stacking support in the infrastructure and one had to manually stack a >> secondary module internally. inode_setxattr and inode_removexattr >> however were special cases because the cap functions would check >> CAP_SYS_ADMIN for any non-capability attributes in the security.* >> namespace, and we don't want to impose that requirement on setting >> security.selinux. Thus, we inlined the capabilities logic into the >> selinux hook functions and adapted it appropriately. > > Now that the permission checks in commoncap have evolved this > inlining of their contents has become a problem. So restructure > selinux_inode_removexattr, and selinux_inode_setxattr to call > both the corresponding cap_inode_ function and dentry_has_perm > when the attribute is not a selinux security xattr. This ensures > the policies of both commoncap and selinux are enforced. > > This results in smack and selinux having the same basic structure > for setxattr and removexattr. Performing their own special permission > checks when it is their modules xattr being written to, and deferring > to commoncap when that is not the case. Then finally performing their > generic module policy on all xattr writes. > > This structure is fine when you only consider stacking with the > commoncap lsm, but it becomes a problem if two lsms that don't want > the commoncap security checks on their own attributes need to be > stack. This means there will need to be updates in the future as lsm > stacking is improved, but at least now the structure between smack and > selinux is common making the code easier to refactor. > > This change also has the effect that selinux_linux_setotherxattr becomes > unnecessary so it is removed. > > Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") > Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") > Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > > While this fixes some things this isn't a regression so it should be > able to wait until the next merge window to be merged. Would you like > to take this through the selinux tree? Or shall I take it through mine? > > security/selinux/hooks.c | 43 ++++++++++++++++++------------------------- > 1 file changed, 18 insertions(+), 25 deletions(-) This patch looks sane to me and I believe it addresses Stephen's concerns, but I'll wait on him to comment on-list. As far as how this gets merged, I'd much prefer to take this via the SELinux tree given that all of the changes are internal to SELinux. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index f5d304736852..c78dbec627f6 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3124,27 +3124,6 @@ static int selinux_inode_getattr(const struct path *path) > return path_has_perm(current_cred(), path, FILE__GETATTR); > } > > -static int selinux_inode_setotherxattr(struct dentry *dentry, const char *name) > -{ > - const struct cred *cred = current_cred(); > - > - if (!strncmp(name, XATTR_SECURITY_PREFIX, > - sizeof XATTR_SECURITY_PREFIX - 1)) { > - if (!strcmp(name, XATTR_NAME_CAPS)) { > - if (!capable(CAP_SETFCAP)) > - return -EPERM; > - } else if (!capable(CAP_SYS_ADMIN)) { > - /* A different attribute in the security namespace. > - Restrict to administrator. */ > - return -EPERM; > - } > - } > - > - /* Not an attribute we recognize, so just check the > - ordinary setattr permission. */ > - return dentry_has_perm(cred, dentry, FILE__SETATTR); > -} > - > static bool has_cap_mac_admin(bool audit) > { > const struct cred *cred = current_cred(); > @@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, > u32 newsid, sid = current_sid(); > int rc = 0; > > - if (strcmp(name, XATTR_NAME_SELINUX)) > - return selinux_inode_setotherxattr(dentry, name); > + if (strcmp(name, XATTR_NAME_SELINUX)) { > + rc = cap_inode_setxattr(dentry, name, value, size, flags); > + if (rc) > + return rc; > + > + /* Not an attribute we recognize, so just check the > + ordinary setattr permission. */ > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > + } > > sbsec = inode->i_sb->s_security; > if (!(sbsec->flags & SBLABEL_MNT)) > @@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct dentry *dentry) > > static int selinux_inode_removexattr(struct dentry *dentry, const char *name) > { > - if (strcmp(name, XATTR_NAME_SELINUX)) > - return selinux_inode_setotherxattr(dentry, name); > + if (strcmp(name, XATTR_NAME_SELINUX)) { > + int rc = cap_inode_removexattr(dentry, name); > + if (rc) > + return rc; > + > + /* Not an attribute we recognize, so just check the > + ordinary setattr permission. */ > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > + } > > /* No one is allowed to remove a SELinux security label. > You can change the label, but all data must be labeled. */ > -- > 2.14.1 >
Paul Moore <paul@paul-moore.com> writes: > On Mon, Oct 2, 2017 at 10:38 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> When selinux is loaded the relax permission checks for writing >> security.capable are not honored. Which keeps file capabilities >> from being used in user namespaces. >> >> Stephen Smalley <sds@tycho.nsa.gov> writes: >>> Originally SELinux called the cap functions directly since there was no >>> stacking support in the infrastructure and one had to manually stack a >>> secondary module internally. inode_setxattr and inode_removexattr >>> however were special cases because the cap functions would check >>> CAP_SYS_ADMIN for any non-capability attributes in the security.* >>> namespace, and we don't want to impose that requirement on setting >>> security.selinux. Thus, we inlined the capabilities logic into the >>> selinux hook functions and adapted it appropriately. >> >> Now that the permission checks in commoncap have evolved this >> inlining of their contents has become a problem. So restructure >> selinux_inode_removexattr, and selinux_inode_setxattr to call >> both the corresponding cap_inode_ function and dentry_has_perm >> when the attribute is not a selinux security xattr. This ensures >> the policies of both commoncap and selinux are enforced. >> >> This results in smack and selinux having the same basic structure >> for setxattr and removexattr. Performing their own special permission >> checks when it is their modules xattr being written to, and deferring >> to commoncap when that is not the case. Then finally performing their >> generic module policy on all xattr writes. >> >> This structure is fine when you only consider stacking with the >> commoncap lsm, but it becomes a problem if two lsms that don't want >> the commoncap security checks on their own attributes need to be >> stack. This means there will need to be updates in the future as lsm >> stacking is improved, but at least now the structure between smack and >> selinux is common making the code easier to refactor. >> >> This change also has the effect that selinux_linux_setotherxattr becomes >> unnecessary so it is removed. >> >> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") >> Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") >> Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> >> While this fixes some things this isn't a regression so it should be >> able to wait until the next merge window to be merged. Would you like >> to take this through the selinux tree? Or shall I take it through mine? >> >> security/selinux/hooks.c | 43 ++++++++++++++++++------------------------- >> 1 file changed, 18 insertions(+), 25 deletions(-) > > This patch looks sane to me and I believe it addresses Stephen's > concerns, but I'll wait on him to comment on-list. He has alredy acked this publicly. I may have skipped Cc'ing the selinux list as the discussion was originally more general and the selinux list is reported to be subscribers (not me) only. > As far as how this gets merged, I'd much prefer to take this via the > SELinux tree given that all of the changes are internal to SELinux. Sounds good. I just care that it get's picked up. Eric >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index f5d304736852..c78dbec627f6 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -3124,27 +3124,6 @@ static int selinux_inode_getattr(const struct path *path) >> return path_has_perm(current_cred(), path, FILE__GETATTR); >> } >> >> -static int selinux_inode_setotherxattr(struct dentry *dentry, const char *name) >> -{ >> - const struct cred *cred = current_cred(); >> - >> - if (!strncmp(name, XATTR_SECURITY_PREFIX, >> - sizeof XATTR_SECURITY_PREFIX - 1)) { >> - if (!strcmp(name, XATTR_NAME_CAPS)) { >> - if (!capable(CAP_SETFCAP)) >> - return -EPERM; >> - } else if (!capable(CAP_SYS_ADMIN)) { >> - /* A different attribute in the security namespace. >> - Restrict to administrator. */ >> - return -EPERM; >> - } >> - } >> - >> - /* Not an attribute we recognize, so just check the >> - ordinary setattr permission. */ >> - return dentry_has_perm(cred, dentry, FILE__SETATTR); >> -} >> - >> static bool has_cap_mac_admin(bool audit) >> { >> const struct cred *cred = current_cred(); >> @@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, >> u32 newsid, sid = current_sid(); >> int rc = 0; >> >> - if (strcmp(name, XATTR_NAME_SELINUX)) >> - return selinux_inode_setotherxattr(dentry, name); >> + if (strcmp(name, XATTR_NAME_SELINUX)) { >> + rc = cap_inode_setxattr(dentry, name, value, size, flags); >> + if (rc) >> + return rc; >> + >> + /* Not an attribute we recognize, so just check the >> + ordinary setattr permission. */ >> + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); >> + } >> >> sbsec = inode->i_sb->s_security; >> if (!(sbsec->flags & SBLABEL_MNT)) >> @@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct dentry *dentry) >> >> static int selinux_inode_removexattr(struct dentry *dentry, const char *name) >> { >> - if (strcmp(name, XATTR_NAME_SELINUX)) >> - return selinux_inode_setotherxattr(dentry, name); >> + if (strcmp(name, XATTR_NAME_SELINUX)) { >> + int rc = cap_inode_removexattr(dentry, name); >> + if (rc) >> + return rc; >> + >> + /* Not an attribute we recognize, so just check the >> + ordinary setattr permission. */ >> + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); >> + } >> >> /* No one is allowed to remove a SELinux security label. >> You can change the label, but all data must be labeled. */ >> -- >> 2.14.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 Tue, Oct 3, 2017 at 5:26 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Paul Moore <paul@paul-moore.com> writes: > >> On Mon, Oct 2, 2017 at 10:38 AM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >>> >>> When selinux is loaded the relax permission checks for writing >>> security.capable are not honored. Which keeps file capabilities >>> from being used in user namespaces. >>> >>> Stephen Smalley <sds@tycho.nsa.gov> writes: >>>> Originally SELinux called the cap functions directly since there was no >>>> stacking support in the infrastructure and one had to manually stack a >>>> secondary module internally. inode_setxattr and inode_removexattr >>>> however were special cases because the cap functions would check >>>> CAP_SYS_ADMIN for any non-capability attributes in the security.* >>>> namespace, and we don't want to impose that requirement on setting >>>> security.selinux. Thus, we inlined the capabilities logic into the >>>> selinux hook functions and adapted it appropriately. >>> >>> Now that the permission checks in commoncap have evolved this >>> inlining of their contents has become a problem. So restructure >>> selinux_inode_removexattr, and selinux_inode_setxattr to call >>> both the corresponding cap_inode_ function and dentry_has_perm >>> when the attribute is not a selinux security xattr. This ensures >>> the policies of both commoncap and selinux are enforced. >>> >>> This results in smack and selinux having the same basic structure >>> for setxattr and removexattr. Performing their own special permission >>> checks when it is their modules xattr being written to, and deferring >>> to commoncap when that is not the case. Then finally performing their >>> generic module policy on all xattr writes. >>> >>> This structure is fine when you only consider stacking with the >>> commoncap lsm, but it becomes a problem if two lsms that don't want >>> the commoncap security checks on their own attributes need to be >>> stack. This means there will need to be updates in the future as lsm >>> stacking is improved, but at least now the structure between smack and >>> selinux is common making the code easier to refactor. >>> >>> This change also has the effect that selinux_linux_setotherxattr becomes >>> unnecessary so it is removed. >>> >>> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") >>> Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") >>> Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git >>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >>> --- >>> >>> While this fixes some things this isn't a regression so it should be >>> able to wait until the next merge window to be merged. Would you like >>> to take this through the selinux tree? Or shall I take it through mine? >>> >>> security/selinux/hooks.c | 43 ++++++++++++++++++------------------------- >>> 1 file changed, 18 insertions(+), 25 deletions(-) >> >> This patch looks sane to me and I believe it addresses Stephen's >> concerns, but I'll wait on him to comment on-list. > > He has alredy acked this publicly. So he did, for some reason I missed it in this thread, my apologies. > I may have skipped Cc'ing the selinux list as the discussion was > originally more general and the selinux list is reported to be > subscribers (not me) only. The list is quasi-open, non-members can post, they are just moderated. That said I know the mailing list has been having some problems lately. >> As far as how this gets merged, I'd much prefer to take this via the >> SELinux tree given that all of the changes are internal to SELinux. > > Sounds good. I just care that it get's picked up. Yep. I just merged it into the SELinux next branch and I'll be sending this up during the next merge window.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f5d304736852..c78dbec627f6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3124,27 +3124,6 @@ static int selinux_inode_getattr(const struct path *path) return path_has_perm(current_cred(), path, FILE__GETATTR); } -static int selinux_inode_setotherxattr(struct dentry *dentry, const char *name) -{ - const struct cred *cred = current_cred(); - - if (!strncmp(name, XATTR_SECURITY_PREFIX, - sizeof XATTR_SECURITY_PREFIX - 1)) { - if (!strcmp(name, XATTR_NAME_CAPS)) { - if (!capable(CAP_SETFCAP)) - return -EPERM; - } else if (!capable(CAP_SYS_ADMIN)) { - /* A different attribute in the security namespace. - Restrict to administrator. */ - return -EPERM; - } - } - - /* Not an attribute we recognize, so just check the - ordinary setattr permission. */ - return dentry_has_perm(cred, dentry, FILE__SETATTR); -} - static bool has_cap_mac_admin(bool audit) { const struct cred *cred = current_cred(); @@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, u32 newsid, sid = current_sid(); int rc = 0; - if (strcmp(name, XATTR_NAME_SELINUX)) - return selinux_inode_setotherxattr(dentry, name); + if (strcmp(name, XATTR_NAME_SELINUX)) { + rc = cap_inode_setxattr(dentry, name, value, size, flags); + if (rc) + return rc; + + /* Not an attribute we recognize, so just check the + ordinary setattr permission. */ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); + } sbsec = inode->i_sb->s_security; if (!(sbsec->flags & SBLABEL_MNT)) @@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct dentry *dentry) static int selinux_inode_removexattr(struct dentry *dentry, const char *name) { - if (strcmp(name, XATTR_NAME_SELINUX)) - return selinux_inode_setotherxattr(dentry, name); + if (strcmp(name, XATTR_NAME_SELINUX)) { + int rc = cap_inode_removexattr(dentry, name); + if (rc) + return rc; + + /* Not an attribute we recognize, so just check the + ordinary setattr permission. */ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); + } /* No one is allowed to remove a SELinux security label. You can change the label, but all data must be labeled. */