[Toybox] [PATCH] Implement SELinux ls -Z support.

[Rob Landley]

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"?)

(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).

(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

 1431808525.0