Re: [PATCH v19 0/4] overlayfs override_creds=off & nested get xattr fix

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Mar 11, 2022 at 12:11 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Mar 9, 2022 at 4:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Mar 1, 2022 at 12:05 AM David Anderson <dvander@google.com> wrote:
> > > On Mon, Feb 28, 2022 at 5:09 PM Paul Moore <paul@paul-moore.com> wrote:
>
> ...
>
> > >> This patchset may not have been The Answer, but surely there is
> > >> something we can do to support this use-case.
> > >
> > > Yup exactly, and we still need patches 3 & 4 to deal with this. My current plan is to try and rework our sepolicy (we have some ideas on how it could be made compatible with how overlayfs works). If that doesn't pan out we'll revisit these patches and think harder about how to deal with the coherency issues.
> >
> > Can you elaborate a bit more on the coherency issues?  Is this the dir
> > cache issue that is alluded to in the patchset?  Anything else that
> > has come up on review?
> >
> > Before I start looking at the dir cache in any detail, did you have
> > any thoughts on how to resolve the problems that have arisen?
>
> David, Vivek, Amir, Miklos, or anyone for that matter, can you please
> go into more detail on the cache issues?  I *think* I may have found a
> potential solution for an issue that could arise when the credential
> override is not in place, but I'm not certain it's the only issue :)
>

Hi Paul,

In this thread I claimed that the authors of the patches did not present
a security model for overlayfs, such as the one currently in overlayfs.rst.
If we had a model we could have debated its correctness and review its
implementation.

As a proof that there is no solid model, I gave an *example* regarding
the overlay readdir cache.

When listing a merged dir, meaning, a directory containing entries from
several overlay layers, ovl_permission() is called to check user's permission,
but ovl_permission() does not currently check permissions to read all layers,
because that is not the current overlayfs model.

Overlayfs has a readdir cache, so without override_cred, a user with high
credentials can populate the readdir cache and then a user will fewer
credentials, not enough to access the lower layers, but enough to access
the upper most layer, will pass ovl_permission() check and be allowed to
read from readdir cache.

This specific problem can be solved in several ways - disable readdir
cache with override_cred=off, check all layers in ovl_permission().
That's not my point. My point is that I provided a proof that the current
model of override_cred=off is flawed and it is up to the authors of the
patch to fix the model and provide the analysis of overlayfs code to
prove the model's correctness.

The core of the matter is there is no easy way to "merge" the permissions
from all layers into a single permission blob that could be checked once.

Maybe the example I gave is the only flaw in the model, maybe not
I am not sure. I will be happy to help you in review of a model and the
solution that you may have found.

Thanks,
Amir.