[Toybox] [PATCH] Implement SELinux ls -Z support.
enh
enh at google.com
Sat May 16 13:49:28 PDT 2015
On Sat, May 16, 2015 at 1:35 PM, Rob Landley <rob at landley.net> wrote:
> On 05/03/2015 09:53 PM, enh wrote:
> > Implement SELinux ls -Z support.
> >
> > This patch uses lgetfilecon rather than fgetfilecon because
> > dirtree_parentfd always seems to return -1 in this function. If/when
> > the SMACK code is fixed to work with dirtree_parentfd, I'll send a
> > matching patch for SELinux.
>
> I mentioned the bug there, need to fix it.
>
> The O_PATH stuff making extended attributes inaccessable is sad, it's
> _defined_ as metadata in the man pages but apparently it's treated as
> normal data for these system calls.
>
> (In reality it's multiple data forks and each one needs its own set of
> rwx permission bits and _this_ is why complicating a system does not
> make it more secure. This whole nightmare was invented so the macintosh
> finder could store X/Y coordinates on the icons when you moved files
> around in the gui, which was not sufficient justification for it.
> Copying it breaks unix's "everything is a file" paradigm because it
> means a FILE is no longer a file, instead it's now a _directory_ of
> files with magic meanings that you can't traverse using the existing
> APIs.)
>
> Right, anyway, here's what happened when I looked at the first commit
> in the smack-10 repo. (I am waaaay behind on this conversation...)
>
> ------
>
> Ok, looking at smack-10 commit c34c4d7225b7, "security: as a module".
>
> This does two things, the first of which is basically:
>
> --- a/Config.in
> +++ b/Config.in
> @@ -37,25 +37,25 @@ config TOYBOX_SUID
>
> choice
> prompt "Security Blanket"
> - default TOYBOX_LSM_NONE
> + default SECURITY_NONE
> help
> Select a Linux Security Module to complicate your system
> until you can't find holes in it.
>
> Which hits a toybox kconfig policy thing: config symbols that _don't_
> belong
> to a specific command all start with TOYBOX_ (I.E. global config symbols
> nominally belong to the multiplexer). This is changing the symbol name to
> belong to a command named "security", which isn't there now but it's
> unclean.
>
> If you want TOYBOX_LSM_SELINUX or TOYBOX_LSM_SMACK we could do that. It's
> more consistent, but longer and presumably unneeded. (The TOYBOX_ symbol
> space isn't cluttered enough to worry about conflicts or losing track of
> what symbols mean, and I _really_ don't want to grow enough for that to
> become a problem.)
>
> The second thing this does is factor out and expand security theatre code,
> adding a lot of code that has else cases for being called when security
> theatre is not enabled, ala:
>
> +int security_set_context(char *filename, char *context)
> +{
> + if (CFG_SECURITY_SMACK)
> + return smack_set_label_for_path(filename, XATTR_NAME_SMACK, 1,
> context);
> + if (CFG_SECURITY_SELINUX)
> + return setfilecon(filename, context);
> + return -1;
> +}
>
> The compiler can't reliably optimize that out (since it crosses compilation
> unit boundaries), but having the infrastructure encourages people to call
> these functions without if (!CFG_TOYBOX_LSM_NONE) guards, so the security
> theatre calls can wind up adding code when configured out.
>
> The #defines that you moved from portability.h to your lib/security.c
> did allow the code to become constants that get optimized out, ala
> #define smack_new_label_from_file(...) (-1). It's nice to factor out
> repeated code, but this is the issue that I worry about when I see this.
>
> Also, the factored out versions don't have any comments explaining the
> functions, and I don't know what "self_context" means, nor the difference
> between lset_context() and fset_context(). (I take it "file" in fset
> is actually "fd"?)
>
yeah, just like stat, fstat, and lstat.
> (I also admit a very minor nit-pick about the names: "security" implies
> the functions are actually providing some, while lsm just says we're
> implementing support for a linux security module, which is just an API.)
>
> Ok, let's look at the actual newly added code:
>
> Static variables are essentially globals, I try to avoid them. (They add
> up,
> and get allocated in every command instance, which hits nommu especially
> hard.)
>
> Function definitions should have (void) in their argument list if they take
> no arguments, otherwise it's an implicit (...) meaning you can feed
> arbitrary varargs. (Not that we're likely to, it's just slightly unclean.)
>
> Huh, we really do need the == 1 for is_selinux_enabled() because of
> _course_
> the function is so badly designed that using its return value as true or
> false doesn't work. Imagine if isatty() did that. (When it returns 0, it
> sets errno. It can set it to 0 if it's just not a tty but there wasn't
> otherwise a problem. Why couldn't is_selinux_enabled() do that? Because
> it's
> bureaucratic punchcard code writen by mainframe developers trying to
> impress multiple layers of management. What a mess...)
>
> Actually, if I make lsm_enabled() a static inline and move it into lsm.h,
> then I can have _that_ be the test that other things use which should
> resolve
> to a constant...
>
> Looking at the SELINUX stuff and comparing with ubuntu's man pages:
> security_context_t is a char *, and freecon() is just free()? Is this an
> android-ism or is this how Red Hat Enterprise wrote it to take away Sun's
> Navy contacts a decade and change back? I don't know what's expected here,
> I did a grep, but the string "freecon" does not appear in the musl source
> code (as of April 14th anyway).
>
from selinux/selinux.h:
/* No longer used; here for compatibility with legacy callers.
*/typedef char *security_context_t;
i'll ask whether there are plans to deprecate freecon.
> (It would be nice if I had a man page for smack_new_label_from_self() but
> ubuntu hasn't got that.)
>
> Ok, yeah, this is broken:
>
> +char *security_self_context()
> +{
> + int ok;
> + char *result;
> +
> + if (CFG_SECURITY_SMACK) ok = smack_new_label_from_self(&result) > 0;
> + else if (CFG_SECURITY_SELINUX) ok = getcon(&result) == 0;
> + else ok = 0;
> +
> + return ok ? result : strdup("?");
> +}
>
> +void security_free_context(char *context)
> +{
> + if (CFG_SECURITY_SELINUX)
> + freecon(context);
> + else
> + free(context);
> +}
>
> You can return "?" and freecon() it. Alright, who's using this? Only id.c
> so
> far:
>
> if (CFG_ID_SECURITY) {
> if (security_is_enabled()) {
> char *context = security_self_context();
> printf(" context=%.*s"+!!TT.do_Z, context);
> if (CFG_TOYBOX_FREE) free(context);
> } else if (TT.do_Z) error_exit("security disabled");
> }
>
> Well, I see why you didn't hit it (not testing with CFG_TOYBOX_FREE).
>
> The error_exit() on the end there seems wrong because id_main() has:
>
> if (toys.optc) while(*toys.optargs) do_id(*toys.optargs++);
>
> And the exit will prevent evaluation of later arguments. (What do
> other id implementations do in this situation? Beats me, haven't got
> a test environment with this stuff enabled...)
>
> Right, ok, focusing on cleaning up the _rest_ of the infrastructure...
> Hmmm, I think I'm going to treat SELINUX as the else case, since it's
> pretty
> much a default and we're #defining all its functions to -1 so if I do
> else return setfilecon(filename, context) and CFG_TOYBOX_SELINUX is off,
> it returns -1 anyway.
>
> Actually... why aren't _all_ of these static inlines? In any given config
> they should optimize down to basically one function call...
>
> Rob
>
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20150516/d69aac53/attachment-0004.htm>
More information about the Toybox
mailing list