On Thu, 2011-10-27 at 21:10 -0400, Kyle Moffett wrote: > I looked at audit_log_n_untrustedstring and > security_context_to_sid_core, and both seem to assume that "size" does > not include a trailing NUL. security_context_to_sid_core() is supposed to handle it either way, with or without a trailing NUL. Originally it only took NUL-terminated strings (with the original SELinux interfaces), but we had to make it more forgiving of its input when we switched to using the xattr APIs. > Reading through "security_context_to_sid_core()", it appears to have a > bug when !ss_initialized: > if (!strcmp(initial_sid_to_string[i], scontext)) > > Where's the references to scontext_len??? Fair enough, we don't seem to have updated that case. Of course, that only happens before policy is loaded and thus anything is allowed anyway. > It looks like it will allow (during early boot) a process to set an > invalid xattr on a file yet cache a valid SID for the same file, EG: > "system_u:object_r:kernel_t:s0\0HelloWorld", hm? Well, technically it will only accept strings with initial SID names (e.g. "kernel" or "kernel\0HelloWorld", which aren't even valid contexts once the policy has loaded). And that's merely to ensure that security_sid_to_context() followed by security_context_to_sid() on an initial SID during bootup yields the right SID. Those contexts should never be stored at all, as selinux_inode_setxattr() won't let you set an SELinux label on a filesystem until after policy has loaded. > Furthermore, the rest of that code is very non-obviously correct > regarding checking against the length instead of a NUL character. That's because the original code only dealt with NUL-terminated strings, and we merely changed security_context_to_sid_core() to create a NUL-terminated copy for use by the rest of the code (it was already copying it to allow modifying it during parsing, so we just extended it by a byte and cleared the last byte). And then string_to_context_struct() applies a check against scontext_len just before the policydb_context_isvalid() call to ensure that we have consumed the entire context value during parsing. So trying to set a context value with content after a NUL will just return -1 with errno EINVAL; feel free to try it (I did). -- Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with the words "unsubscribe selinux" without quotes as the message.
