Commit e3fea3f7 authored by Al Viro's avatar Al Viro Committed by James Morris

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: default avatarAl Viro <viro@zeniv.linux.org.uk>
Tested-by: default avatarDave Jones <davej@redhat.com>
Reported-by: default avatarDave Jones <davej@redhat.com>
Signed-off-by: default avatarJames Morris <james.l.morris@oracle.com>
parent 5935e6dc
...@@ -2791,11 +2791,16 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, ...@@ -2791,11 +2791,16 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
/* We strip a nul only if it is at the end, otherwise the /* We strip a nul only if it is at the end, otherwise the
* context contains a nul and we should audit that */ * context contains a nul and we should audit that */
str = value; if (value) {
if (str[size - 1] == '\0') str = value;
audit_size = size - 1; if (str[size - 1] == '\0')
else audit_size = size - 1;
audit_size = size; else
audit_size = size;
} else {
str = "";
audit_size = 0;
}
ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR); ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
audit_log_format(ab, "op=setxattr invalid_context="); audit_log_format(ab, "op=setxattr invalid_context=");
audit_log_n_untrustedstring(ab, value, audit_size); audit_log_n_untrustedstring(ab, value, audit_size);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment