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