Re: [PATCH v2 0/7] Smack namespace

[Lukasz Pawelczyk <l.pawelczyk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

Hi,

Thanks for taking the interest and commenting on this.
Replies below.


On wto, 2015-05-26 at 10:35 -0400, Stephen Smalley wrote:
> On 05/25/2015 08:32 AM, Lukasz Pawelczyk wrote:
> > --- Design ideas ---
> > 
> > "Smack namespace" is rather "Smack labels namespace" as not the whole
> > MAC is namespaced, only the labels. There is a great analogy between
> > Smack labels namespace and the user namespace part that remaps UIDs.
> > 
> > The idea is to create a map of labels for a namespace so the namespace
> > is only allowed to use those labels. Smack rules are always the same
> > as in the init namespace (limited only by what labels are mapped) and
> > cannot be manipulated from the child namespace. The map is actually
> > only for labels' names. The underlying structures for labels remain
> > the same. The filesystem also stores the "unmapped" labels from the
> > init namespace.
> 
> How do you achieve that without introducing additional hooks or
> reworking the current hooks in the setxattr code path?  At present, the
> security module is allowed to rewrite getxattr requests on the
> security.* namespace but it isn't allowed to do that for setxattr, so if
> the process invokes setxattr with a mapped label, then it will be the
> mapped label that gets passed to the filesystem implementation, not the
> unmapped label.  The security module may internally store it in unmapped
> form and may even return that upon getxattr() calls, but if you then
> reboot the system and later fetch from the filesystem, it will get the
> mapped label value.

I call the inode operation by hand in the post_setxattr.

The label will effectively be set twice, which is not ideal, but there
is no other option right now without reworking the hooks as you said.

This shouldn't really be a problem because the Smack operations will not
use the filesystem label (even when it's set incorrectly for a moment)
but an already initialized smack_known structure for this inode that has
all the values filled in properly.

The only attack vector I can think of is hard rebooting the machine in a
way that mapped label is really saved in the filesystem before the
unmapped will have a chance. Should I be worried about that? This sounds
a little unreal.


@@ -1163,12 +1214,24 @@ static void smack_inode_post_setxattr(struct dentry *dentry, const char *name,
                skpp = &isp->smk_mmap;
 
        if (skpp) {
-               skp = smk_get_label(value, size, true);
+               skp = smk_get_label(value, size, true, ns);
 
                if (!IS_ERR(skp))
                        *skpp = skp;
                else
                        *skpp = &smack_known_invalid;
+
+               /*
+                * The label we get above might be a different than the one
+                * kernel has already set before calling this function.
+                * Be consistent and set the final value in the filesystem.
+                * The cases for this are errors and labels being used
+                * in a namespace where we want to store an unmapped
+                * value in the filesystem.
+                */
+               dentry->d_inode->i_op->setxattr(dentry, name,
+                                               (*skpp)->smk_known,
+                                               size, flags);
        }
 }
 


> 
> > --- Usage ---
> > 
> > Smack namespace is written using LSM hooks inside user namespace. That
> > means it's connected to it.
> > 
> > To create a new Smack namespace you need to unshare() user namespace
> > as usual. If that is all you do though, than there is no difference to
> > what is now. To activate the Smack namespace you need to fill the
> > labels' map. It is in a file /proc/$PID/smack_map.
> 
> This should be /proc/$PID/attr/label_map or similar, modeled after the
> existing /proc/$PID/attr/current and similar nodes.  Then it isn't
> module-specific and can be reused for other modules.

Sure. I had some thoughts about that, but couldn't really figure out
which option would be best. I'll change it as suggested.


> > Writing to the map file is not disabled after the first write as it is
> > in uid_map. For Smack we have no means to map ranges of labels, hence
> > it can really be advantageous to be able to expand the map later
> > on. But you can only add to the map. You cannot remove already mapped
> > labels. You cannot change the already existing mappings. Also mappings
> > has to be 1-1. All requests to create a map where either the unmapped
> > or the mapped label already exists in the map will be denied.
> 
> Isn't it a concern that I can then add additional labels to the mapping
> for which I am not authorized?  Or is this mitigated by the fact that I
> cannot alter the rules?  

You can't add mappings for which you are not authorized.
This is mitigated by both, by the fact that adding a mapping is a
privileged operation in the parent (only init for now) namespace and
that modifying the rules is a privileged operation allowed only in the
init namespace (as it is now).


> What about the situation for the predefined
> labels in Smack - are you assuming that they will always be mapped up
> front in the mapping file?

If somebody wants to have a full Smack aware container with full Smack
functionality that's what I'd expect, but it's not mandatory.



-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics