[Toybox] What's setransd?

[Rob Landley]

On 8/18/21 5:57 PM, enh wrote:> On Wed, Aug 18, 2021 at 3:27 AM Rob Landley
<rob at landley.net
> <mailto:rob at landley.net>> wrote:
> 
>     I'm trying to implement tar --selinux and it really looks like setfscreatecon()
>     and getfileconat() mostly boil down to simple things like "open a /proc file and
>     read/write a blob of data", ala:
> 
>       int sefd = -1;
>       ...
>       if ((FLAG(selinux) && !(FLAG(t)|FLAG(O)))
>           && strstart(&pp, "RHT.security.selinux="))
> 
> 
> (it's just called security.selinux where i come from... see XATTR_NAME_SELINUX
> in <linux/xattr.h>.)

It's saved in the tarball differently than in the filesystem, because the posix
pax format standard is actively stupid:

  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html

Between the 'x' extension header of "%u %s=%s\n" SIZE KEYWORD=VALUE\n records
saying:

  Keywords consisting entirely of lowercase letters, digits, and periods are
  reserved for future standardization.

And then:

  The standard developers have reserved keyword name space for vendor
  extensions. It is suggested that the format to be used is:

  VENDOR.keyword

  where VENDOR is the name of the vendor or organization in all uppercase
  letters. It is further suggested that the keyword following the <period> be
  named differently than any of the standard keywords so that it could be used
  for future standardization, if appropriate, by omitting the VENDOR prefix.

Therefore when Red Hat was introducing a new keyword it HAD to prefix uppercase
crap onto it to avoid violating posix, so "RHT.security.selinux" is what Red Hat
introduced for its own use, and that's what everybody copied verbatim (including
gnu tar), and now it's what everybody has been using for over a decade, and at
this point changing it would break stuff. Posix!

What did they THINK was going to happen?

>       {
>         i = strlen(pp);
>         sefd = xopen("/proc/self/attr/fscreate", O_WRONLY|WARN_ONLY);
>         if (sefd == -1 ||  i!=write(sefd, pp, i))
>           perror_msg("setfscreatecon %s", pp);
>       }
>       ...
>       if (sefd != -1) {
>         write(sefd, 0, 0);
>         close(sefd);
>         sefd = -1;
>       }
> 
> 
>     ...except that's the _raw versions of the function. The non-raw versions do a
>     network transaction (via _iovec_???) with "setransd" and googling for that one
>     of the first hits is https://selinuxproject.org/page/MLSStatements
>     <https://selinuxproject.org/page/MLSStatements> which talks
>     about translating security context info to/from "human readable" format?
> 
> 
> (fwiw, Android builds with -DDISABLE_SETRANS, but i don't know about other systems.)

I.E. the translation's a NOP on android anyway, and not doing it doesn't break
anything there? :)

>     Yeah, I _CAN_ add a lot more "#define setfscreatecon(...)" crap to stub out the
>     functions when it's disabled, but I don't want to? "Reach out and talk to the
>     kernel directly" looked almost sane for a moment as long as I didn't have to
>     care what was IN the data blob. Which none of the other implementations seems
>     to, but they're using the "cooked" functions which go through this daemon I've
>     never heard of which doesn't appear to have anything like a man page...
> 
> yeah, i'm not sure why you're bothering with this?

Because toybox tries to minimize build dependencies, and if I can do this
directly myself without sucking in an external library I'd like to at least
investigate doing so?

> just let libselinux hide this
> cruft like you do for the rest of the selinux stuff? "not using libselinux"
> seems likely to upset the folks who _want_ selinux.

Only if it breaks stuff.

What I've read of that library so far is all trivial wrappers over system calls
with no actual _logic_, except for the undocumented daemon Android doesn't use.
It's very tempting to try to just do what it's doing under the covers myself,
especially since tar didn't previously support --selinux so an 80/20 solution
here isn't a regression.

If people show up and go "no really, my centos system NEEDS this translation
daemon 100% otherwise flying monkeys" then I'd have learned something. (Maybe
Gayellette knows what setransd actually _does_ to the data...)

> even if it is, as you point
> out, pretty bad when you actually look at the implementation --- duplicating it
> just means  now they have _two_ things that they have to audit.

Not necessarily?

Last time I open coded setfscreatecon_raw(), and getfilecon_raw() is just size =
getxattr(path, "security.selinux", buf, size); with the same "do it twice"
malloc() dance as xmprintf(). There's almost nothing THERE.

The current lib/lsm.h stubs are:

#if CFG_TOYBOX_SELINUX
#include <selinux/selinux.h>
#else
#define is_selinux_enabled() 0
#define setfscreatecon(...) (-1)
#define getcon(...) (-1)
#define getfilecon(...) (-1)
#define lgetfilecon(...) (-1)
#define fgetfilecon(...) (-1)
#define setfilecon(...) (-1)
#define lsetfilecon(...) (-1)
#define fsetfilecon(...) (-1)
#endif

The set/l/f stuff is pretty much the same plumbing (the difference is just
getxattr, lgetxattr, or fgetxattr under the covers), and while I haven't traced
all the way through the obfuscation yet I strongly suspect getcon() is just "cat
/proc/self/attr/current". (Yeah ok the implementation actually uses
/proc/thread-self but toybox isn't threaded so those symlinks point to the same
place.)

It all boils down to read/write proc files and a couple syscalls: the library
itself isn't even filtering the data. There really isn't a whole lot of there
there, at least for toybox's use cases.

In theory libselinux can add new layers of complexity in future, but in practice
it has to maintain binary compatability with the existing binaries and system
calls basically forever, AND it's ~20 years old (18), so "what it's doing now"
should be reasonably stable? So if what it's doing now is actually fairly simple
(which it is)... (I admit I haven't read through android's selinux
implementation yet. You say you're doing more stuff than the linux one is...
Nope, your setfilecon_raw() is calling setxattr(). As of July, anyway.)

I do admit that "restorecon" is WAAAAAAAY outside the scope of toybox.
Specifically, selinux_android_restorecon() is more or less its own library, a
giant pile of heuristics interrogating /proc/mounts for reasons I'm happier not
knowing.

But that's not really "selinux support": toys/android/restorecon.c doesn't even
have any non-android plumbing outside the #ifdef __ANDROID__ where it calls a
selinux_android_restorecon() function with android in the name. It's in
toys/android for a reason, and lib/lsm.h doesn't even try to address it.

I'm also looking at the SMACK support and going "this hasn't been regression
tested in years, I haven't heard from the tizen guys in forever, I don't have a
smack test environment or anybody to ask about setting one up, and I hear
conflicting things about
https://www.theverge.com/2021/5/18/22440483/samsung-smartwatch-google-wearos-tizen-watch
vs
https://www.androidheadlines.com/2021/05/samsung-tvs-continue-use-tizen-os.html...
If I'm not gonna use lsm_get_context() instead of getfilecon() for tar --selinux
anyway then why have that layer at all...?

I admit that USING the lsm_*() wrappers might be a good reason not to do it
directly. But if not I kinda wanna go the other way: lose the extra config
symbol and compile-time probe and just ALWAYS support ls -Z and such. If the
/proc file isn't there, cope at runtime. The syscalls predate the 2.6 release
(beyond even Centos' support horizon) so are unlikely to break the build
anywhere we care about.

> (similar to the
> libcrypto argument, though there there's FIPS that explicitly *mandates* not
> duplicating things, but i think the mentality is similar regardless.)

Crypto is a twisty maze of side channel data leaks, all different. Professional
cryptographers can't write something they trust without getting a second and
third opinion from other professional cryptographers about an IR camera picking
up differential pulls on the power supply. Rolling your own crypto is widely
acknowledged as a bad idea... which busybox did. (Builtin https support. Denys
wrote his own, in networking/tls.c. Figuring out what to do about that is a
post-1.0 todo item, I admit wget and httpd are kinda useless without it these
days...)

But toybox doesn't ever ENFORCE selinux policy. It just gets and sets labels,
which are opaque (to it) data blobs. It neither knows nor cares what they mean,
the engine that runs the rules to implement policy is somewhere else entirely.

We're also (mostly) talking about _labels_, not _rules_...

> more practically, there's a bunch of local Android configuration in our
> libselinux, and i'm guessing (from your claim that RedHat doesn't even use the
> same name for the attribute!) that's true of the various Linux distros too... so
> potentially you end up needing to carry all the local cruft for every user too.
> doesn't seem worth it?

I agree toybox should not duplicate any of that. If toybox NEEDS to duplicate
any of that, then obviously what I'm thinking of doing isn't workable. (I.E.
restorecon is still gonna need to -lselinux because for some reason there's no
-landroid to contain the new function with android in the name.)

But the rest of toybox doesn't do that much with selinux, and what it DOES do
turns out not to actually need libselinux so far...

> just #define no-op variants in portability.h and let
> selinux users rely on their local libselinux?

Yeah, that's what I've done before. But at least the parts of selinux I've
looked at so far don't really seem to have any real reason to exist. The
kernel's doing all the work, the library's just a pointless glue layer.

So far. Maybe I've missed something, and as I said I'm looking at the label
parts not the rule parts. I still have a todo item to track down
is_selinux_enabled() security_getenforce() from getenforce.c, and I think really
what I'm trying to do is yank the lib/lsm.h wrapper and go the other way with it?

But right NOW I'm just trying to add --selinux support to tar, since adding it
in a new semi-experimental way can't break existing stuff. Switching over stuff
like ls -Z can wait until we see if that holds water.

(And a potential advantage of this is on BSD or macos it just naturally reads as
"selinux is disabled". Possibly portability.h would need a readxattr() NOP stub.
Using their xattr variants to preserve the labels needs more thought. And asking
a domain expert. Goes on the todo list, and not near the top either.)

>     Rob

Still Rob

P.S. is_selinux_enabled() is just !statfs("/sys/fs/selinux", &buf) && buf.f_type
== 0xf97cff8c) plus checking it's not read only, and if you're NOT on android
(it's got an #ifdef) also checking that /etc/selinux/config exists. Then
security_getenforce() is just reading a number out of /sys/fs/selinux/enforce
and returning whether or not it's nonzero. (So the kernel is potentially
returning MORE information, and the library is squashing it with !!value.
Because... reasons?)

P.P.S. Ok, technically it can fall back to checking /proc/filesystems to see if
selinuxfs is a known filesystem, and if so check /proc/mounts to  see if it's
mounted anywhere. Why not just do the second one directly? No idea. Why check
two hardwired locations first and THEN do this fallback? I couldn't tell you.
That filesystem's only used for rules, not labels, so toybox MOSTLY doesn't have
to care outside enabled and getenforce.

P.P.P.S. If you mount selinuxfs on a directory with a space in the path to it,
the kernel will %xx escape it in octal (see linux/fs/proc_namespace.c function
mangle()) and init_selinuxmnt() won't de-escape it. Same goes for % in a path
component...

P.P.P.P.S. The kernel's mangle() calling seq_escape() calling seq_escape_str()
calling seq_escape_mem() calling string_escape_mem() calling escape_octal() to
actually DO FOUR LINES OF WORK is just SAD. Also, I went through that exercise
to answer "did they remember to escape % as well" and I STILL don't know, if
they did I missed it. But given the existence of "unescape_octal()" in
lib/string_helpers.c it's entirely possible you could have mountpoint including
a filename with %57 in it do something nasty inside the kernel, but I am now far
enough down this rathole I'm going to back away slowly.)