security: Make the selinux setxattr and removexattr hooks behave

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr. However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes to the security.capable xattr.

To keep things working

and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++
 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 2 lines

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 16 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

security/selinux/hooks.c +1 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)

Quoted 3 lines

security/selinux/hooks.c +1 -0

+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

?

Quoted 10 lines

? if (strcmp(name, XATTR_NAME_SELINUX))
? return selinux_inode_setotherxattr(dentry, name);
?

security/selinux/hooks.c +0 -0

@@ -3282,6 +3285,9 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

?
?static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
?{

security/selinux/hooks.c +1 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)

Quoted 5 lines

security/selinux/hooks.c +2 -0

+		return cap_inode_removexattr(dentry, name);
+

? if (strcmp(name, XATTR_NAME_SELINUX))
? return selinux_inode_setotherxattr(dentry, name);
?

.

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

 and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 12 lines

Quoted 9 lines

  if (strcmp(name, XATTR_NAME_SELINUX))
  return selinux_inode_setotherxattr(dentry, name);

security/selinux/hooks.c +0 -0

@@ -3282,6 +3285,9 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

 static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
 {

Quoted 8 lines

Quoted 6 lines

security/selinux/hooks.c +3 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_removexattr(dentry, name);
+

  if (strcmp(name, XATTR_NAME_SELINUX))
  return selinux_inode_setotherxattr(dentry, name);

.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 12 lines

?

Quoted 9 lines

? if (strcmp(name, XATTR_NAME_SELINUX))
? return selinux_inode_setotherxattr(dentry, name);
?

security/selinux/hooks.c +0 -0

@@ -3282,6 +3285,9 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

?
?static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
?{

Quoted 8 lines

Quoted 6 lines

security/selinux/hooks.c +3 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_removexattr(dentry, name);
+

? if (strcmp(name, XATTR_NAME_SELINUX))
? return selinux_inode_setotherxattr(dentry, name);
?

.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

 and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.
?

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.
?

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Eric

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.
?

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Quoted 156 lines · Casey Schaufler

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

 and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 8 lines

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Merged as of 4.14-rc1 is the support for user namespace root to set sercurity.capable. This fails when selinux is loaded.

removexattr has the same problem and the code is a little less convoluted in that case.

Not being able to set the capability when you should be able to is very noticable. Like running into a brick wall noticable.

Which is where the minimal patch for selinux comes in. I think it solves the exact case in question, even if it isn't the perfect long term solution.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 156 lines · Casey Schaufler

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.
?

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 8 lines

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Merged as of 4.14-rc1 is the support for user namespace root to set sercurity.capable. This fails when selinux is loaded.

removexattr has the same problem and the code is a little less convoluted in that case.

Not being able to set the capability when you should be able to is very noticable. Like running into a brick wall noticable.

Which is where the minimal patch for selinux comes in. I think it solves the exact case in question, even if it isn't the perfect long term solution.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 166 lines · Eric W. Biederman

Quoted 164 lines · Casey Schaufler

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

 and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Quoted 2 lines

Merged as of 4.14-rc1 is the support for user namespace root to set sercurity.capable. This fails when selinux is loaded.

OK. Is the failure unique to SELinux, or does it fail with Smack as well?

Quoted 2 lines

removexattr has the same problem and the code is a little less convoluted in that case.

Right. Because removexattr is a simpler situation.

Quoted 2 lines

Not being able to set the capability when you should be able to is very noticable. Like running into a brick wall noticable.

Ah, now you've identified the problem. Yes, I would agree that you've uncovered an undesirable behavior.

Quoted 3 lines

Which is where the minimal patch for selinux comes in. I think it solves the exact case in question, even if it isn't the perfect long term solution.

If the problem is unique to SELinux I can see your logic. If it isn't, that is, if it's also a problem with any other security module, there either needs to be a fix for that/those module/s as well or a "real" fix.

I'm not opposed to the SELinux short term fix if you can say that that's the only module with the problem.

Quoted 166 lines · Eric W. Biederman

Quoted 164 lines · Casey Schaufler

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.
?

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Quoted 2 lines

Merged as of 4.14-rc1 is the support for user namespace root to set sercurity.capable. This fails when selinux is loaded.

OK. Is the failure unique to SELinux, or does it fail with Smack as well?

Quoted 2 lines

removexattr has the same problem and the code is a little less convoluted in that case.

Right. Because removexattr is a simpler situation.

Quoted 2 lines

Not being able to set the capability when you should be able to is very noticable. Like running into a brick wall noticable.

Ah, now you've identified the problem. Yes, I would agree that you've uncovered an undesirable behavior.

Quoted 3 lines

Which is where the minimal patch for selinux comes in. I think it solves the exact case in question, even if it isn't the perfect long term solution.

If the problem is unique to SELinux I can see your logic. If it isn't, that is, if it's also a problem with any other security module, there either needs to be a fix for that/those module/s as well or a "real" fix.

I'm not opposed to the SELinux short term fix if you can say that that's the only module with the problem.

Quoted 167 lines · Casey Schaufler

Quoted 166 lines · Eric W. Biederman

Quoted 164 lines · Casey Schaufler

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

?and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.
?

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Quoted 4 lines

Quoted 2 lines

Merged as of 4.14-rc1 is the support for user namespace root to set sercurity.capable. This fails when selinux is loaded.

OK. Is the failure unique to SELinux, or does it fail with Smack as well?

I don't have a smack configuration handy, but reading through the code smack setxattr the permission checks for all xattrs that are not smack xattrs to cap_inode_setxattr.

So smack and commoncap combined will not fail.

smack and selinux will result in people who should be able to set selinux xattrs not being able to. That however is less of an immediate problem.

Quoted 9 lines

Quoted 3 lines

removexattr has the same problem and the code is a little less convoluted in that case.

Right. Because removexattr is a simpler situation.

Quoted 2 lines

Not being able to set the capability when you should be able to is very noticable. Like running into a brick wall noticable.

Ah, now you've identified the problem. Yes, I would agree that you've uncovered an undesirable behavior.

Apologies for not being clearer earlier, but I was still in shock from running into a brick wall.

Quoted 11 lines

Quoted 4 lines

Which is where the minimal patch for selinux comes in. I think it solves the exact case in question, even if it isn't the perfect long term solution.

If the problem is unique to SELinux I can see your logic. If it isn't, that is, if it's also a problem with any other security module, there either needs to be a fix for that/those module/s as well or a "real" fix.

I'm not opposed to the SELinux short term fix if you can say that that's the only module with the problem.

So far there are exactly two implementation of
LSM_HOOK_INIT(inode_setxattr, ...)

So as a practical case it looks like combination with selinux is the only case where the problem will be observed right now. And it makes the code at least somewhat match the comments in security_inode_setxattr.

Eric

I'm kind of surprised that the capability changes got sent upstream without SELinux ever being tested. That's a very common configuration in the Android enabled world.

Quoted 3 lines · Eric W. Biederman

I don't have a smack configuration handy, but reading through the code smack setxattr the permission checks for all xattrs that are not smack xattrs to cap_inode_setxattr.

It's not hard to configure Smack. But, if you have a test case I can run it for you.

From memory:

Quoted 5 lines

So smack and commoncap combined will not fail.

smack and selinux will result in people who should be able to set selinux xattrs not being able to. That however is less of an immediate problem.

That's not currently a problem as you can't configure them both to be enabled.

You clearly don't work in security is running into a brick wall is a shocking experience :)

Quoted 155 lines · Eric W. Biederman

Quoted 75 lines · Casey Schaufler

Quoted 50 lines · Stephen Smalley

Quoted 41 lines · Casey Schaufler

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 1 line

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

 and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 14 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

Quoted 8 lines

Quoted 4 lines

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 3 lines

Except that it isn't, not if any other security module is enabled and implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the worst I've done in a long time. This is a case where we already need special case stacking infrastructure. It looks like we'll have to separate setting the cap attribute from checking the cap state in order to make this work. In any case, the security_inode_setxattr() code is where the change belongs. There will likely be fallout changes in the modules, including the cap module.

Quoted 11 lines

An alternative approach to fixing this would be to change the cap functions to only apply their checks if setting the capability attribute and defer any checks on other security.* attributes to either the security framework or the other security modules. Then the framework could always call all the modules on the inode_setxattr and inode_removexattr hooks as with other hooks. The security framework would then need to ensure that a check is still applied when setting security.* attributes if it isn't already handled by one of the enabled security modules, as you don't want unprivileged userspace to be able to set arbitrary security.foo attributes or to set up security.selinux or security.SMACK* attributes if those modules happen to be disabled.

Quoted 8 lines

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making assumptions when I go to fix a problem. I will start looking at a patch, but it would really help if I could say for sure what I'm out to accomplish. It may be obvious to the casual observer, but that description has not been applied to me very often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c applies no permission checks.

Each security module that implements an xattr in security.* then imposes it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs. ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for the security.* in selinux_inode_setotherxattr. Testing for capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN) for the others.

With the added complication that selinux calls selinux_inode_setotherxattr also for the remove_xattr case. So fixing this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all invocations of setxattr.

So I think to really fix this we need to separate the cases of is this your security modules attribute from general policy checks added by the security modules. Perhaps something like this for security_inode_setxattr:

Hmm. Looking at least ima also has the distinction between protecting it's own xattr writes and running generaly security module policy on xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)

{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Quoted 1 line

Eric

Yup, that's pretty much what I'm thinking. It's unfortunate that the magic_ API isn't fully implemented. There's going to be a good deal of code surgery instead. Is there an observed problem today? This is going to have to get addressed for stacking, so if there isn't a behavioral issue that impacts something real I would like to defer spending significant time on it. Do you have a case where this is not working correctly?

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes
to the security.capable xattr.

What leads you to believe that this isn't intentional? It's most likely the case that this change occurred as part of the first round module stacking change. What behavior do you see that you're unhappy with?

Quoted 2 lines

To keep things working

Which "things"? How are they not "working"?

Quoted 7 lines

 and to make the comments in security/security.c correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've encountered I can't tell if it's here, in the infrastructure, or in your perception of what constitutes "broken".

Quoted 16 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

security/selinux/hooks.c +1 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)

Quoted 3 lines

security/selinux/hooks.c +1 -0

+		return cap_inode_setxattr(dentry, name, value,

size, flags);
+

No. Don't even think of contemplating considering embedding the cap attribute check in the SELinux code. cap_inode_setxattr() is called in
the infrastructure.

Quoted 10 lines

  if (strcmp(name, XATTR_NAME_SELINUX))
  return selinux_inode_setotherxattr(dentry, name);

security/selinux/hooks.c +0 -0

@@ -3282,6 +3285,9 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

 static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
 {

security/selinux/hooks.c +1 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)

Quoted 5 lines

security/selinux/hooks.c +2 -0

+		return cap_inode_removexattr(dentry, name);
+

  if (strcmp(name, XATTR_NAME_SELINUX))
  return selinux_inode_setotherxattr(dentry, name);

.

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr. However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes to the security.capable xattr.

To keep things working

and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 security/selinux/hooks.c | 6 ++++++
 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes to the security.capable xattr.

To keep things working and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 security/selinux/hooks.c | 6 ++++++

 1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct dentry
 *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value, size,

flags);
+
  if (strcmp(name, XATTR_NAME_SELINUX))
  return selinux_inode_setotherxattr(dentry, name);

security/selinux/hooks.c +0 -0

@@ -3282,6 +3285,9 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

 static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
 {

security/selinux/hooks.c +3 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_removexattr(dentry, name);
+

  if (strcmp(name, XATTR_NAME_SELINUX))
  return selinux_inode_setotherxattr(dentry, name);

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes to the security.capable xattr.

To keep things working and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

security/selinux/hooks.c +0 -0

index f5d304736852..edf4bd292dc7 100644
@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct dentry
 *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

security/selinux/hooks.c +2 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value, size,

flags);
+
? if (strcmp(name, XATTR_NAME_SELINUX))
? return selinux_inode_setotherxattr(dentry, name);
?

security/selinux/hooks.c +0 -0

@@ -3282,6 +3285,9 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

?
?static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
?{

security/selinux/hooks.c +3 -0

+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_removexattr(dentry, name);
+

? if (strcmp(name, XATTR_NAME_SELINUX))
? return selinux_inode_setotherxattr(dentry, name);
?

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.  However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes to the security.capable xattr.

Quoted 8 lines

To keep things working and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

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. When the stacking support was introduced, it had to also special case these hooks so that only the primary module's hook is used for the same reason; otherwise, the kernel would end up applying a CAP_SYS_ADMIN check on setting security.selinux. Your change below is almost but not quite right since it only calls the cap functions when setting the capability

attribute; the residual problem is that it will then skip the SELinux FILE__SETATTR (file setattr) permission check when setting those attributes, which we want to retain. So you need to only return early if cap_inode_setxattr()/removexattr() return an error; otherwise, you need to proceed to the SELinux check, and you can then delete the duplicated logic from selinux_inode_setotherxattr(). At which point it just becomes a call to dentry_has_perm() and you can just inline that into selinux_inode_setxattr() and selinux_inode_removexattr().

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.

Quoted 7 lines · Eric W. Biederman

It looks like once upon a time a long time ago selinux copied code from cap_inode_removexattr and cap_inode_setxattr into selinux_inode_setotherxattr.??However the code has now diverged and selinux is implementing a policy that is quite different than cap_inode_setxattr and cap_inode_removexattr especially when it comes to the security.capable xattr.

Quoted 8 lines

To keep things working and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this is enough to keep selinux from implementing a non-sense hard coded policy that breaks other parts of the kernel.

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. When the stacking support was introduced, it had to also special case these hooks so that only the primary module's hook is used for the same reason; otherwise, the kernel would end up applying a CAP_SYS_ADMIN check on setting security.selinux. Your change below is almost but not quite right since it only calls the cap functions when setting the capability

attribute; the residual problem is that it will then skip the SELinux FILE__SETATTR (file setattr) permission check when setting those attributes, which we want to retain. So you need to only return early if cap_inode_setxattr()/removexattr() return an error; otherwise, you need to proceed to the SELinux check, and you can then delete the duplicated logic from selinux_inode_setotherxattr(). At which point it just becomes a call to dentry_has_perm() and you can just inline that into selinux_inode_setxattr() and selinux_inode_removexattr().

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.

-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.

Quoted 8 lines · Stephen Smalley

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-aS9lmoZGLiVWk0Htik3J/[email protected]>

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.

Quoted 10 lines · Stephen Smalley

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" <[email protected]>

---

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(-)

security/selinux/hooks.c +0 -0

index f5d304736852..c78dbec627f6 100644
@@ -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)

security/selinux/hooks.c +0 -10

-{
-	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.

security/selinux/hooks.c +0 -10

-			   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();

security/selinux/hooks.c +0 -0

@@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
  	u32 newsid, sid = current_sid();
  	int rc = 0;

security/selinux/hooks.c +2 -2

-	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);

security/selinux/hooks.c +6 -0

+		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);

security/selinux/hooks.c +1 -0

+	}

  sbsec = inode->i_sb->s_security;
  if (!(sbsec->flags & SBLABEL_MNT))

security/selinux/hooks.c +0 -0

@@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

 static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
 {

security/selinux/hooks.c +8 -2

-	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);

security/selinux/hooks.c +1 -0

+	}

  /* No one is allowed to remove a SELinux security label. You can change the label, but all data must be labeled. */

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.

Quoted 8 lines · Stephen Smalley

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-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

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(-)

security/selinux/hooks.c +18 -26

index f5d304736852..c78dbec627f6 100644
@@ -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

Quoted 48 lines · Eric W. Biederman

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.

Quoted 8 lines · Stephen Smalley

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" <[email protected]>

---

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?

Quoted 2 lines

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.

Quoted 66 lines

security/selinux/hooks.c +18 -25

index f5d304736852..c78dbec627f6 100644
@@ -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);
+       }
 

Quoted 5 lines

security/selinux/hooks.c +0 -1

        /* No one is allowed to remove a SELinux security label.
           You can change the label, but all data must be labeled. */
--

2.14.1

Quoted 50 lines · Paul Moore

Quoted 48 lines · Eric W. Biederman

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.

Quoted 8 lines · Stephen Smalley

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-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

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?

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 50 lines · Paul Moore

Quoted 48 lines · Eric W. Biederman

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.

Quoted 8 lines · Stephen Smalley

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" <[email protected]>

---

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?

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 48 lines · Eric W. Biederman

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.

Quoted 8 lines · Stephen Smalley

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-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

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?

Quoted 2 lines

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.

Quoted 71 lines

security/selinux/hooks.c +18 -26

index f5d304736852..c78dbec627f6 100644
@@ -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

-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.

Quoted 8 lines · Stephen Smalley

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" <[email protected]>

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.

Quoted 10 lines · Stephen Smalley

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" <[email protected]>

---

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(-)

security/selinux/hooks.c +0 -0

index f5d304736852..c78dbec627f6 100644
@@ -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)

security/selinux/hooks.c +0 -10

-{
-	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.

security/selinux/hooks.c +0 -10

-			???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();

security/selinux/hooks.c +0 -0

@@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct
 dentry *dentry, const char *name,
 ?	u32 newsid, sid = current_sid();
 ?	int rc = 0;

?

security/selinux/hooks.c +2 -2

-	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);

security/selinux/hooks.c +6 -0

+		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);

security/selinux/hooks.c +1 -0

+	}

?
? sbsec = inode->i_sb->s_security;
? if (!(sbsec->flags & SBLABEL_MNT))

security/selinux/hooks.c +0 -0

@@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct
 dentry *dentry)

?
?static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
?{

security/selinux/hooks.c +8 -2

-	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);

security/selinux/hooks.c +1 -0

+	}

?
? /* No one is allowed to remove a SELinux security label.
? ???You can change the label, but all data must be labeled.
*/