diff options
author | Al Viro <viro@ZenIV.linux.org.uk> | 2012-06-09 03:15:16 -0400 |
---|---|---|
committer | James Morris <james.l.morris@oracle.com> | 2012-07-30 01:36:50 -0400 |
commit | e3fea3f70fd68af0574a5f24246cdb4ed07f2b74 (patch) | |
tree | c3a8ae06734b2987646dd89c87c0a16ee50d420e /security/selinux/hooks.c | |
parent | 5935e6dcaaa8f666dd7f1169fa87d36752ebeb94 (diff) |
selinux: fix selinux_inode_setxattr oops
OK, what we have so far is e.g.
setxattr(path, name, whatever, 0, XATTR_REPLACE)
with name being good enough to get through xattr_permission().
Then we reach security_inode_setxattr() with the desired value and size.
Aha. name should begin with "security.selinux", or we won't get that
far in selinux_inode_setxattr(). Suppose we got there and have enough
permissions to relabel that sucker. We call security_context_to_sid()
with value == NULL, size == 0. OK, we want ss_initialized to be non-zero.
I.e. after everything had been set up and running. No problem...
We do 1-byte kmalloc(), zero-length memcpy() (which doesn't oops, even
thought the source is NULL) and put a NUL there. I.e. form an empty
string. string_to_context_struct() is called and looks for the first
':' in there. Not found, -EINVAL we get. OK, security_context_to_sid_core()
has rc == -EINVAL, force == 0, so it silently returns -EINVAL.
All it takes now is not having CAP_MAC_ADMIN and we are fucked.
All right, it might be a different bug (modulo strange code quoted in the
report), but it's real. Easily fixed, AFAICS:
Deal with size == 0, value == NULL case in selinux_inode_setxattr()
Cc: stable@vger.kernel.org
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Tested-by: Dave Jones <davej@redhat.com>
Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Diffstat (limited to 'security/selinux/hooks.c')
-rw-r--r-- | security/selinux/hooks.c | 15 |
1 files changed, 10 insertions, 5 deletions
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 94c45a1531a4..79690f401a58 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c | |||
@@ -2791,11 +2791,16 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, | |||
2791 | 2791 | ||
2792 | /* We strip a nul only if it is at the end, otherwise the | 2792 | /* We strip a nul only if it is at the end, otherwise the |
2793 | * context contains a nul and we should audit that */ | 2793 | * context contains a nul and we should audit that */ |
2794 | str = value; | 2794 | if (value) { |
2795 | if (str[size - 1] == '\0') | 2795 | str = value; |
2796 | audit_size = size - 1; | 2796 | if (str[size - 1] == '\0') |
2797 | else | 2797 | audit_size = size - 1; |
2798 | audit_size = size; | 2798 | else |
2799 | audit_size = size; | ||
2800 | } else { | ||
2801 | str = ""; | ||
2802 | audit_size = 0; | ||
2803 | } | ||
2799 | ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR); | 2804 | ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR); |
2800 | audit_log_format(ab, "op=setxattr invalid_context="); | 2805 | audit_log_format(ab, "op=setxattr invalid_context="); |
2801 | audit_log_n_untrustedstring(ab, value, audit_size); | 2806 | audit_log_n_untrustedstring(ab, value, audit_size); |