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

From: Mark Salyzyn
Date: Wed Oct 23 2019 - 10:13:09 EST

On 10/22/19 11:54 PM, Amir Goldstein wrote:
On Tue, Oct 22, 2019 at 11:45 PM Mark Salyzyn <salyzyn@xxxxxxxxxxx> wrote:
Patch series:

Mark Salyzyn (5):
Add flags option to get xattr method paired to __vfs_getxattr
overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh
overlayfs: handle XATTR_NOSECURITY flag for get xattr method
overlayfs: internal getxattr operations without sepolicy checking
overlayfs: override_creds=off option bypass creator_cred

The first four patches address fundamental security issues that should
be solved regardless of the override_creds=off feature.

The fifth adds the feature depends on these other fixes.

By default, all access to the upper, lower and work directories is the
recorded mounter's MAC and DAC credentials. The incoming accesses are
checked against the caller's credentials.

If the principles of least privilege are applied for sepolicy, the
mounter's credentials might not overlap the credentials of the caller's
when accessing the overlayfs filesystem. For example, a file that a
lower DAC privileged caller can execute, is MAC denied to the
generally higher DAC privileged mounter, to prevent an attack vector.

We add the option to turn off override_creds in the mount options; all
subsequent operations after mount on the filesystem will be only the
caller's credentials. The module boolean parameter and mount option
override_creds is also added as a presence check for this "feature",
existence of /sys/module/overlay/parameters/overlay_creds

Signed-off-by: Mark Salyzyn <salyzyn@xxxxxxxxxxx>
Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
Cc: Jonathan Corbet <corbet@xxxxxxx>
Cc: Vivek Goyal <vgoyal@xxxxxxxxxx>
Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
Cc: Amir Goldstein <amir73il@xxxxxxxxx>
Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
Cc: Stephen Smalley <sds@xxxxxxxxxxxxx>
Cc: linux-unionfs@xxxxxxxxxxxxxxx
Cc: linux-doc@xxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx

- Rejoin, rebase and a few adjustments.

- Pull out first patch and try to get it in alone feedback, some
Acks, and then <crickets> because people forgot why we were doing i.

I do not see the first patch on fsdevel
and I am confused from all the suggested APIs
I recall Christoph's comment on v8 for not using xattr_gs_args
and just adding flags to existing get() method.
I agree to that comment.

As already responded, third (?) patch version was like that, gregkh@ said it passed the limit for number of arguments, is looking a bit silly (my paraphrase), and that it should be passed as a structure. Two others agreed. We gained because both set and get use the same structure after this change (this allows a simplified read-modify-write cycle).

We will need a quorum on this, 3 (structure) to 2 (flag) now (but really basically between Greg and Christoph?). Coding style issue: Add a flag, or switch to a common xattr argument structure?

I remember asking - don't remember the answer -
do you have any testing for this feature?
Yes, on an unnamed 4.19-based and mainline-based Android and virtual cuttlefish product ... which was critically unworkable without this patch series.
I have a WIP branch to run unionmount-testsuite not as root,
which is a start, but I didn't get to finish the work.
Let me know if you want to take up this work.
Please refer it in private email to me, no guarantees, my cycles are so sparse right now that it took a month to respin this patch series to upstream. If I can make it test on Android with overlayfs activated, big gain.