On Thu, Nov 18, 2021 at 11:53 AM David Anderson [off-list ref] wrote:

On Tue, Nov 16, 2021 at 11:36 PM Amir Goldstein [off-list ref] wrote:

quoted

Hi David,

I see that the patch set has changed hands (presumably to Android upstreaming
team), but you just rebased v18 without addressing the maintainers concerns [1].

Indeed I'm carrying this forward as Mark is no longer working on it.
My apologies for
missing those comments!

quoted

Specifically, the patch 2/4 is very wrong for unprivileged mount and
I think that the very noisy patch 1/4 could be completely avoided:
Can't you use -o userxattr mount option for Android use case and limit
the manipulation of user.ovrelay.* xattr based on sepolicy for actors
that are allowed
to make changes in overlayfs mount? or not limit at all?
The access to those xattr is forbidden via "incoming" xattr ops on
overlay inodes.

Can you clarify a bit more? The patch is definitely super noisy and I'd love
to have a better solution. The problem it's trying to solve is:
 1. Kernel-privileged init mounts /mnt/blah-lower and /mnt/blah-upper.
 2. Kernel-privileged init mounts /blah with overlayfs using the above dirs.
 2. Kernel-privileged init loads sepolicy off /blah/policy. Enforcing begins.
 3. Kernel-privileged init tries to execute /blah/init to initiate a
domain transition.
 4. exec() fails because the overlayfs mounter creds (kernel domain) does
     not have getxattr permission to /blah/init.

Eg, we're hitting this problem without even making changes to the mount, and
without anything being written to /mnt/blah-upper.

So what is your solution?
Remove the security check from overlayfs setting xattr?
How does that work for the person who composed the security policy?
You will need to grant some basic privileges to the mounter.
If you do not want to grant the mounter privileges to set trusted.overlay xattr
you may use mount option -o userxattr and grant it permissions to set
user.overlay xattrs.

quoted

Can an unprivileged user create an overlay over a directory that they have
access to and redirect an innocent looking file name to an underlying file that
said the mounting user has no access to and by doing that, tricking a privileged
user to modify the innocent looking file on the  mounter's behalf?
Of course this could be avoided by forbidding unprivileged mount with
override_creds=off, but there could be other scenarios, so a clear model
would help to understand the risks.

For example:
If user 1 was able to read in lower dir A, now the content of overlay dir A
is cached and user 2, that has permissions to read upper dir A and does
not have read permissions on lower dir A will see the content of lower dir A.

I'll need to think about this more and test to verify. It's not a scenario that
would come up in our use case (both dirs effectively have the same permissions).

Your argument is taking the wrong POV.
The reason that previous posts of this patch set have been rejected
is not because it doesn't work for your use case.
It is because other players can exploit the feature to bypass security
policies, so the feature cannot be merged as is.

If the answer is "yes, that can happen" - do you see this as a problem of
clarifying the model, or a problem of fixing that loophole?

It is something that is not at all easy to fix.
In the example above, instead of checking permissions against the
overlay inode (on "incoming" readdir) will need to check permissions of every
accessing user against all layers, before allowing access to the merged
directory content (which is cached).
A lot more work - and this is just for this one example.

quoted

quoted

I think that the core problem with the approach is using Non-uniform

credentials to access underlying layers. I don't see a simple way around
a big audit that checks all those cases, but maybe I'm missing some quick
shortcut or maybe your use case can add some restrictions about the
users that could access this overlay that would simplify the generic problem.

In a security model like ours, I think there's no way around it, that
we really need
accesses to be from the caller's credentials and not the mounter's. It's even
worse than earlier iterations of this patch perhaps let on: we mount
before sepolicy
is loaded (so we can overlay the policy itself), and thus the
mounter's creds are
effectively "the kernel". This domain is highly restricted in our
sepolicy for obvious
reasons. There's no way our security team will let us unrestrict it.

Not sure what that means or what I can do with this information.
If I had a simple suggestion on how to solve your problem I would have
suggested it, but I cannot think of any right now.
The best I can do is to try to make you understand the problems that your
patch causes to others, so you can figure out a way that meets your goals
without breaking other use cases.

Thanks,
Amir.