[Toybox] Squinting at LSM support in cp.

[Rob Landley]

On 07/01/2015 12:17 AM, Hyejin Kim wrote:
> There is a reference in tizen, repository called external/tizen-coreutils
> It has a patch to cover smack feature into some commands.
>  
> https://review.tizen.org/gerrit/gitweb?p=external/tizen-coreutils.git;a=blob;f=packaging/coreutils-6.9-smack.patch;h=1ac188e50938d51c4ac8f2ce605580ee1b8da2f0;hb=HEAD
> (If you can't access this, plz register account or let me know) 

It popped up a login request and wouldn't let me access the page without
a username/password, and there was no obvious link to account creation
from the error page.

If I need an account to view the source, it's not an open source project.

> I guess it will be helpful that you understand their intention more.
>  
> As Jose mentioned, there is one more xattr for directory, named
> security.SMACK64TRANSMUTE (Now what we are using to save label is
> security.SMACK64)
> plz, refer to the explanation.

Below, or at the link above?

> What is __security.SMACK64TRANSMUTE__ ?
> ==> Can only have the value "TRUE".

So they've reinvented the sticky bit with a new meaning. And for some
reason, it has a "64" in it. (Was there a SMACK32?)

> If this attribute is present on a directory when an object is created in
> the directory and the Smack rule (more below) that permitted the write
> access to the directory includes the transmute ("t") mode
> the object gets the label of the directory instead of the label of the
> creating process. If the object being created is a directory the
> SMACK64TRANSMUTE attribute is set as well.

The first patch I was sent (Krzysztof Sasiak <k.sasiak at samsung.com> in
the tizen pull request) didn't do that. It only copied xattrs on files,
not on directories or dev nodes or fifos or symlinks... (Can symlinks
have xattrs?)

Presumably this avoids the issue of confusing your own cp -r by creating
directories that then alter their contents to not match the original.
(At least using the "set creation context" mode of labeling objects to
avoid leaving an exploitable race window between create and label.)

It's also the wrong thing to do. (If I'm going to do this at all, I want
to do it right.) We already _know_ there are labels that do something on
a directory, we're discussing one right now, is --preserve=context _not_
supposed to preserve them? (How about --preserve=xattr?)

The other question is how far up the tree does this check go? Can you cp
-r into a directory that already has SMACK64TRANSMUTE on it? (Even if it
only goes one level deep, it can still screw up the top level...)

> For this reason, I guess that a file can't have a bucket of labels. and
> security.SMACK64 xattr is just one per a file or path.

The second patch (from Jose, at
http://lists.landley.net/pipermail/toybox-landley.net/2015-May/004184.html
) has this:

+    flistxattr(fdin, list, listlen);
+    for (name = list ; (name - list) < listlen ; name +=
strlen(name)+1) {
+      /* test if the xattribute have to be copied */
+      if ((TT.preserve & p_xattr)
+      || ((TT.preserve & p_mode) &&
+              !
strncmp(name,"system.posix_acl_",sizeof("system.posix_acl_")-1))
+      || ((TT.preserve & p_context) &&
+              !strncmp(name,"security.",sizeof("security.")-1)))
+      {
+        /* yes, try to copy it */
+        if ((len = fgetxattr(fdin, name, 0, 0)) > 0 && (value =
malloc(len))) {
+          fgetxattr(fdin, name, value, len);
+          err += !!fsetxattr(fdout, name, value, len, 0);
+          free(value);
+        } else err++;
+      }
+    }

Note the two strncmp(), hardwiring in smack and selinux security label
names without a config symbol. Those are operating on prefixes, which
imply there could be multiple entries after them.

I also very very vaguely remember enh or somebody mentioning that the
selinux defaults can be something like a comma separated list of
attributes, but really want to just pass through unmodified strings to
let the existing infrastructure handle that if at all possible.

The above snippet is from a cp_xattrs() function which is only called
once, right after xsendfile(), and thus once again only operates on
strings. (Which is inda broken in that it's handling both
--preserve=xattrs and --preserve=context and the _first_ of those should
definitely operate on everything.)

If the definition is that --preserve=context only operates on files,
then I can set the default creation context and reset it back to
whatever it was right afterwards. (Unless the right thing to do is
_clear_ it at the start because --preserve=context overrides creation
context the same way -p overrides umask().)

In theory I can have --preserve=xattr just copy xattrs for every node
after creation in the loop, and --preserve=context set the creation
context just on files after copying the contents, and that way if you
specify both it may do it twice but at least for files it should both
give you the right eventual result _and_ not have an obvious race where
you can fiddle with a file's contents before the label shows up.

(The really really annoying part is I feel like I'm putting more thought
into this than the people who created it. I want somebody to tell _me_
how to do this, without me being able to spot holes in the first 15
seconds...)

> Further, above link doesnt call flistxattr() like Jose did for cp command.

Above link didn't give me a page.

Sigh, lemme see what I can dig up. Google for tizen-coreutils gives me
https://review.tizen.org/git/?p=external/tizen-coreutils.git;a=tree
and "log" on that says the most recent commit was almost 3 years ago,
but let's see if I click on a commit and then cut and paste your link's
hash into that URL...

https://review.tizen.org/git/?p=external/tizen-coreutils.git;a=commit;h=1ac188e50938

Unknown commit object.

Nope. No idea how to navigate this. There are a couple of tags newer,
but not tree commits? (Am I misunderstanding branches here? A log on the
most recent tag gives me two commits, and when I click the newest _is_ a
more recent commit than a log on the tree but it has no parent...?)

If I was just wandering past and examining the repository I just found
with Google (first hit for "tizen coreutils", I'd think development
ceased years ago. It might have been included verbatim in a couple new
releases by some sort of distro-wide tagging, but it hasn't even had
bugfixes applied since 2012.

I'm guessing this is not actually the case, merely the public face you
present to the open source community?

> Thanks.

Rob

 1435779686.0