BUG: SELinux invalid_context is logged inconsistently

[pcmoore]

From @stevegrubb:

In /security/selinux/hooks.c whenever there is a SELINUX_ERR event, invalid_context is logged as untrusted_string. In security/selinux/ss/services.c in the function security_sid_mls_copy() it is logged as a normal string.

[rgbriggs]

@stevegrubb
Should only selinux invalid_context be treated as untrusted string, or all contexts?

Should all contexts be encoded the same way?

Looking at AUDIT_SELINUX_ERR in security/selinux/ss/services.c: I'm assuming compute_sid_handle_invalid_context() should be untrusted in addition to security_sid_mls_copy() mentioned above, but security_dump_masked_av(), security_bounded_transition() and security_validtrans_handle_fail() may be trusted?

[stevegrubb]

The answer is another question, can the user or admin name the contexts? For example, files and user names can be changed at a whim and have to be untrusted. However, tty names from the kernel are trusted and not escaped. Can arbitrary context names be created?

[rgbriggs]

On 2019-06-10 13:06, Steve Grubb wrote: The answer is another question, can the user or admin name the contexts? For example, files and user names can be changed at a whim and have to be untrusted. However, tty names from the kernel are trusted and not escaped. Can arbitrary context names be created?

From a conversation on freenode:#selinux, 2019-06-10 16:45: Q: Can arbitrary context names be created? I ask to learn if audit should be reporting contexts other than "invalid_context=" as untrusted_string. redragon: you can create any context you want if you create a policy to define the context redragon: you can't arbitrarily assign an undefined context to a file Q: Can that context contain spaces and other punctuation? (and even control characters) sfix SunRaycer: no sfix: The security.selinux xattr can contain an arbitrary string but it'll be reported as an invalid contxt Valid punctuation only includes "_-,:.". danieldg: All components of a valid context will be selected from the names defined in the security policy, which is usually considered trusted. While they can be combined in lots of ways, you can't get arbitrary characters in a context even if it's not checked elsewhere. I just have to not trust invalid_context.

[WOnder93]

Should only selinux invalid_context be treated as untrusted string, or all contexts?

Should all contexts be encoded the same way?

Looking at AUDIT_SELINUX_ERR in security/selinux/ss/services.c: I'm assuming compute_sid_handle_invalid_context() should be untrusted in addition to security_sid_mls_copy() mentioned above, but security_dump_masked_av(), security_bounded_transition() and security_validtrans_handle_fail() may be trusted?

Unless I'm mistaken, context_struct_to_string() should always return a string that follows the general SELinux context format, just the user/role/type/mls can be invalid under the current policy (e.g. no type defined with such name). So all of the above cases should be trusted. (@pcmoore, @stephensmalley, please correct me if I'm wrong.)

You should only need to deal with untrusted invalid contexts when they come from an external source, e.g. a file's extended attribute or the value written to a special procfs file. Actually, now that I think of it, I should have logged the srawcon/trawcon fields as untrusted in [1]... *runs off to write a patch*

[1] SELinuxProject/selinux-kernel@fede148

[rgbriggs]

On 2019-06-11 00:53, Ondrej Mosnáček wrote: > Looking at AUDIT_SELINUX_ERR in security/selinux/ss/services.c: I'm assuming compute_sid_handle_invalid_context() should be untrusted in addition to security_sid_mls_copy() mentioned above, but security_dump_masked_av(), security_bounded_transition() and security_validtrans_handle_fail() may be trusted? Unless I'm mistaken, `context_struct_to_string()` should always return a string that follows the general SELinux context format, just the user/role/type/mls can be invalid under the current policy (e.g. no type defined with such name). So all of the above cases should be trusted. ***@***.***, @stephensmalley, please correct me if I'm wrong.)

By looking at the code I'm gathering that all user/role/type/mls strings are passed in via the policydb which is presumed trusted. I conclude that if the policydb is gamed there are bigger problems than audit record formatting. If so, then selinux_inode_setxattr() and selinux_setprocattr() must remain as untrusted invalid_context. @stevegrubb The question remains: Must all instances of the invalid_context field be encoded the same way, untrusted_string for the parser?

[stevegrubb]

Yes, a field name has a specific format or it needs a new name. Said another way, all instances of the invalid_context field must be encoded the same way.

[rgbriggs]

Staged in selinux/next: ea74a68 ("selinux: format all invalid context as untrusted")
Upstreamed for v5.3-rc1 in 7c0f896 ("Merge tag 'selinux-pr-20190702' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux")

[rgbriggs]

Please close. Upstream in Linux 5.3 4d856f7