overlay: mkdir fails if directory exists in lowerdir in a user namespace

[Serge Hallyn]

If a directory exists in the lowerdir but not in the mounted
overlay, then mkdir of the directory in the target dir results
in a mysterious -EPERM. I've seen this both in wily kernel
(4.2.0-22-generic #27-Ubuntu) and in a hand-built xenial
master-next (with unrelated patches added).

=====================================================
#!/bin/sh -ex
dir=`mktemp -d`
cleanup() {
 umount -l $dir/t
 rm -rf $dir
}

trap cleanup EXIT

echo "dir is $dir"
mkdir -p $dir/l $dir/u $dir/w $dir/t
mkdir $dir/l/dev
mount -t overlay -o lowerdir=$dir/l,upperdir=$dir/u,workdir=$dir/w o $dir/t
stat $dir/t/dev
rmdir $dir/t/dev
mkdir $dir/t/dev
echo $?
echo "mkdir should have succeeded"
=====================================================

The above will work on the host, but fail in a user namespace, i.e
in a regular lxd container.

[Brad Figg (brad-figg)]

This bug is missing log files that will aid in diagnosing the problem. From a terminal window please run:

apport-collect 1531747

and then change the status of the bug to 'Confirmed'.

If, due to the nature of the issue you have encountered, you are unable to run this command, please add a comment stating that fact and change the bug status to 'Confirmed'.

This change has been made by an automated script, maintained by the Ubuntu Kernel Team.

[Serge Hallyn (serge-hallyn)]

 summary overlay: mkdir in user namespace fails if directory exists in lowerdir"

[Joseph Salisbury (jsalisbury)]

Can you see if this bug also happens with the latest mainline kernel? It can be downloaded from:
http://kernel.ubuntu.com/~kernel-ppa/mainline/v4.4-rc8-wily

[Serge Hallyn (serge-hallyn)]

Quoting Joseph Salisbury (<email address hidden>):
> Can you see if this bug also happens with the latest mainline kernel? It can be downloaded from:

That is not an option, because the mainline kernel doesn't support unprivileged
overlayfs mounting which is where this happens.

[Serge Hallyn (serge-hallyn)]

In ovl_create_over_whiteout(), the ovl_set_opaque() in the S_ISDIR() block failed.

[Serge Hallyn (serge-hallyn)]

The type of the underlaying file does not matter, only the type of the replacing object.

So if you

touch $t/dev; rm $t/dev; touch $t/dev
mkdir $t/dev; rmdir $t/ev; touch $t/dev

those succeed, while

touch $t/dev; rm $t/dev; mkdir $t/dev
mkdir $t/dev; rm $t/dev; mkdir $t/dev

both fail.

[Serge Hallyn (serge-hallyn)]

Ok, I see. At one point we had a special case to allow the overlay code to write trusted.* xattrs for creating whiteouts.

However that is gone. Therefore when overlayfs v1 (mount -t overlayfs) is mounted, root in a user namespace also is not able to rm a file which exists in the lower fs.

Some ways to fix this:

1. Add a special case in fs/xattr.c to allow the overlay code to create the trusted.overlay xattrs
2. In ovl_create_or_link(), target the override cred at init_user_ns. Since we don't do that, the capabilities we are adding do not grant "capable(CAP_SYS_ADMIN)", only ns_capable.
3. Find another way to do this without requiring the trusted.overlay xattr. It isn't needed for files so I don't know what the complications are, which require it to be done for directories.

[Seth Forshee (sforshee)]

Hmm. In this case it's really the kernel that is writing the xattr, so in that case #2 kind of makes sense. It's also more than a bit scary, assigning CAP_SYS_ADMIN in init_user_ns to a task from a user namespace. Now we're already doing it for unprivileged users in init_user_ns which isn't all that different, except for the fact that in the user namespace that unprivileged user can also create the overlay mount, and that leaves me feeling a bit uneasy. I'm not familiar enough with overlayfs to decide whether or not this really presents an opportunity for someone to do something malicious to the lower fs.

With #1, I don't think we have a way to distinguish between overlayfs trying to write this xattr and userspace writing it directly, do we? This also might present an opportunity for a user to do something mildly malicious.

I can't comment on #3, I just don't know enough about overlayfs.

I don't really any other ideas. #2 seems the most logical to me if we can be sure that it's safe.

[Serge Hallyn (serge-hallyn)]

#2 is probably a bit too gross - we really only need the cap for the setting
of the OVL_XATTR_OPAQUE xattr in ovl_set_opaque. So we could simply override
creds again there.

[Serge Hallyn (serge-hallyn)]

hat may not be ok for the ovl_rename2 case.

What we want is for inode permissions to be checked, but only the
bit in xattr_permission() checking for trusted.* to accept ns_capable.

We could special-case that in xattr_permission(), but that's not
particularly nice.

[Seth Forshee (sforshee)]

I don't know why #2 is that much grosser than what's there now. It's already only taking the cap for setting the xattr, and taking CAP_SYS_ADMIN in init_user_ns seems to be what it's really wanting to do there. The difference now though is that before that capability would have been required to do the mount and now it isn't.

If we were to use ns_capable, which namespace do we use? current_user_ns? Then that check becomes worthless because any user can make a new namespace to bypass it. If we had the s_user_ns patches it might make sense to use that, but that probably doesn't solve the problem anyway since the lower mount was probably mounted in init_user_ns.

[Serge Hallyn (serge-hallyn)]

Quoting Seth Forshee (<email address hidden>):
> I don't know why #2 is that much grosser than what's there now. It's

I didn't mean gross as in eeuw, I meant not fine-grained enough.

Because the capability will apply to inode permissions checks,
and we only want it to be used for the check authorizing the
writing of the trusted.overlay.opaque xattr.

> already only taking the cap for setting the xattr, and taking
> CAP_SYS_ADMIN in init_user_ns seems to be what it's really wanting to do

Maybe - that's what I'm not sure about. As you said earlier, in the
upstream code only an admin can do the actual mount. The fact that an
unpriv user can create the mount may change assumptions about the
underlying fs's.

> there. The difference now though is that before that capability would
> have been required to do the mount and now it isn't.

Right.

> If we were to use ns_capable, which namespace do we use?

I don't know. We're almost better off shipping a new version of
vfs_xattr() which is only for use by kernel writers.

If we had your patch we could maybe check against the sb->user_ns?

> current_user_ns? Then that check becomes worthless because any user can
> make a new namespace to bypass it. If we had the s_user_ns patches it

Quit saying in the next paragraph what I say in reply to the previous!

> might make sense to use that, but that probably doesn't solve the
> problem anyway since the lower mount was probably mounted in
> init_user_ns.

Good point, hadn't thought of that.

[Serge Hallyn (serge-hallyn)]

in ovl_clear_empty(), the opaque bit is set on the dir in workingdir

in ovl_create_over_whiteout() (the case we're currently looking at) it is
also being set in the working dir.

in ovl_rename2(), it is set in two places, on the upper dentries for
both the old and new.

So it is never set on the lowerdir, at least.

I'm still looking, but it may be safe to say that all needed inode
checks are already done before we call ovl_set_opaque() so that we
can indeed just use prepare_kernel_cred(NULL) instead of prepare_cred().

[Seth Forshee (sforshee)]

On Tue, Jan 12, 2016 at 04:56:24PM -0000, Serge Hallyn wrote:
> Quoting Seth Forshee (<email address hidden>):
> > I don't know why #2 is that much grosser than what's there now. It's
>
> I didn't mean gross as in eeuw, I meant not fine-grained enough.
>
> Because the capability will apply to inode permissions checks,
> and we only want it to be used for the check authorizing the
> writing of the trusted.overlay.opaque xattr.

That makes more sense. And that's the part that concerns me the most
too.

> > already only taking the cap for setting the xattr, and taking
> > CAP_SYS_ADMIN in init_user_ns seems to be what it's really wanting to do
>
> Maybe - that's what I'm not sure about. As you said earlier, in the
> upstream code only an admin can do the actual mount. The fact that an
> unpriv user can create the mount may change assumptions about the
> underlying fs's.

Yeah, that's something I'm just not sure about. It seems like by
allowing the unprivileged user to mount in the first place we're
implicitly saying that it's okay to write these xattrs to the underlying
fs based on checks which happen at mount time. I don't know what checks
are actually done at mount time though; unless we've augmented them they
may be minimal based on the assumption that only CAP_SYS_ADMIN can
mount.

> > If we were to use ns_capable, which namespace do we use?
>
> I don't know. We're almost better off shipping a new version of
> vfs_xattr() which is only for use by kernel writers.

Maybe so.

[Seth Forshee (sforshee)]

On Tue, Jan 12, 2016 at 05:16:28PM -0000, Serge Hallyn wrote:
> in ovl_clear_empty(), the opaque bit is set on the dir in workingdir
>
> in ovl_create_over_whiteout() (the case we're currently looking at) it is
> also being set in the working dir.
>
> in ovl_rename2(), it is set in two places, on the upper dentries for
> both the old and new.
>
> So it is never set on the lowerdir, at least.

Ah. So if it's never set on the lowerdir that does remove much of the
concern.

> I'm still looking, but it may be safe to say that all needed inode
> checks are already done before we call ovl_set_opaque() so that we
> can indeed just use prepare_kernel_cred(NULL) instead of prepare_cred().

Cool.

[Seth Forshee (sforshee)]

On Tue, Jan 12, 2016 at 05:16:28PM -0000, Serge Hallyn wrote:
> I'm still looking, but it may be safe to say that all needed inode
> checks are already done before we call ovl_set_opaque() so that we
> can indeed just use prepare_kernel_cred(NULL) instead of prepare_cred().

I've been looking too, only at ovl_create_over_whiteout so far. Here's
my take, though I'm still not an overlayfs expert so I may have missed
something.

Obviously the vfs is going to validate permissions to do the requested
operation in the overlayfs superblock. This will result in
ovl_permission getting called, which (for any write operation) is going
to validate that the caller has the necessary permissions towards the
relevant inode in uppderdir. That's good.

So by the time we raise the caps and call ovl_create_over_whiteout we
know that the user is allowed to do the operation, and
ovl_create_or_link is only changing upperdir in the ways needed to
satisfy this operation. Thus having the kernel assume full-on
CAP_SYS_ADMIN before calling ovl_create_or_link seems fine wrt to
upperdir.

But nothing is being checked wrt workdir before elevating the
capabilities. So maybe there are some checks at mount time to make sure
the user has permissions needed towards workdir? ovl_fill_super doesn't
appear to do so explicitly. However it does call ovl_workdir_create, and
this expects that $WORKDIR/work doesn't already exist (where $WORKDIR is
the string passed to mount via workdir=). If it does exist it tries to
remove it with either vfs_rmdir or vfs_unlink (both of which call
may_delete), then it tries to create $WORKDIR/work using vfs_mkdir
(which calls may_create). If either of these fails the mount is
read-only. This directory is what is used as the workdir for the mount,
so I think this effectively validates that the kernel can safely modify
the workdir in the same ways that it's modifying the upperdir.

[Serge Hallyn (serge-hallyn)]

Does it require the workdir to be empty?

I.e. is there a way (symlink, bind mount, something else) that a user could use a dir they own which has a child which they don't own?

It looks like no, since

root@w1:/tmp# mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=workdir overlay /mnt
root@w1:/tmp# ls /mnt
cisco
root@w1:/tmp# rmdir /mnt/cisco
rmdir: failed to remove ‘/mnt/cisco’: Read-only file system
root@w1:/tmp# mv /mnt/cisco /mnt/c2
mv: cannot move ‘/mnt/cisco’ to ‘/mnt/c2’: Read-only file system

(here w1 is a unpriv container with /hostopt a bind mount of /opt on the host; cisco a directory both in host's /opt and in /tmp/lowerdir)

[Seth Forshee (sforshee)]

On Thu, Jan 14, 2016 at 12:17:06AM -0000, Serge Hallyn wrote:
> Does it require the workdir to be empty?

Not empty. It wants to remove workdir/work if it exists and then create
workdir/work, see ovl_workdir_create. Removing workdir/work is done as
the user which is mounting, so nothing can be removed that the user
couldn't already remove. ovl_workdir_create is ultimately just using
vfs_rmdir if the directory exists, which is going to fail if
workdir/work isn't empty.

> I.e. is there a way (symlink, bind mount, something else) that a user
> could use a dir they own which has a child which they don't own?

I don't think so, since

1. lowerdir is not written to.
2. We've established that what's done with workdir won't make this
   possible.
3. When the vfs checks inode permissions for writing ovl_permission is
   called, which checks the permissions for the inode in upperdir for
   any write operation.

I've checked most of the places where overlay fs raises its credentials,
and I haven't found anything where it's doing anything with elevated
creds that isn't in line with the permission checks that would have
already been done. I'm still looking at ovl_rename2 though, which is
doing a lot more stuff with elevated creds than anywhere else.

Something I'm still not sure about is what would happen if you made a
symlink, bind mount, etc. in upperdir with the same name as an unrelated
file in lowerdir. This is worth checking out.

> It looks like no, since
>
> root@w1:/tmp# mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=workdir overlay /mnt
> root@w1:/tmp# ls /mnt
> cisco
> root@w1:/tmp# rmdir /mnt/cisco
> rmdir: failed to remove ‘/mnt/cisco’: Read-only file system
> root@w1:/tmp# mv /mnt/cisco /mnt/c2
> mv: cannot move ‘/mnt/cisco’ to ‘/mnt/c2’: Read-only file system
>
> (here w1 is a unpriv container with /hostopt a bind mount of /opt on the
> host; cisco a directory both in host's /opt and in /tmp/lowerdir)

I think I'm missing something here. I don't know why your mount is
read-only. But even if it wasn't, cisco is in lowerdir and thus should
never be modified or removed in any case. Removing it in /mnt should (I
think, still learning here) result in making a file in upperdir with the
whiteout xattr set.

[Serge Hallyn (serge-hallyn)]

> Something I'm still not sure about is what would happen if you made a
> symlink, bind mount, etc. in upperdir with the same name as an unrelated
> file in lowerdir. This is worth checking out.

just tried a symlink and it didn't seem to affect the host directory
(/opt/cisco) which was symlinked to /tmp/upper/cisco in the container to
begin with.

> > It looks like no, since
> >
> > root@w1:/tmp# mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=workdir overlay /mnt
> > root@w1:/tmp# ls /mnt
> > cisco
> > root@w1:/tmp# rmdir /mnt/cisco
> > rmdir: failed to remove ‘/mnt/cisco’: Read-only file system
> > root@w1:/tmp# mv /mnt/cisco /mnt/c2
> > mv: cannot move ‘/mnt/cisco’ to ‘/mnt/c2’: Read-only file system
> >
> > (here w1 is a unpriv container with /hostopt a bind mount of /opt on the
> > host; cisco a directory both in host's /opt and in /tmp/lowerdir)
>
> I think I'm missing something here. I don't know why your mount is
> read-only.

Because a directory in workdir is owned by uid -1 (root on the host).

> But even if it wasn't, cisco is in lowerdir and thus should
> never be modified or removed in any case. Removing it in /mnt should (I

Right, but I was trying to use workdir as a vector to make changes to
something in the host's opt. Not lowerdir.

[Seth Forshee (sforshee)]

ovl_rename2 seems okay. As before, the vfs does inode permission checks on the overlayfs inodes which results in checks on the upper and/or lower dir inodes as appropriate. These checks appear to be sufficient to imply permission to do everything ovl_rename2 does with elevated credentials. That said, ovl_rename2 does quite a bit of stuff with elevated creds, and that leaves me feeling a bit uncomfortable.

[oleg (overlayfs)]

FWIW, bug #1480411, "rm -r * fails to delete directories when using overlayfs in a user-namespace", might be a duplicate of this bug. In both bugs the script works on the host but fails in a user-namespace with error EPERM. In this bug the operation which triggers the error is 'mkdir', while in bug #1480411 it is 'rmdir'.

The equivalent script for bug #1480411 is,

#================================
#!/bin/sh -ex
dir=`mktemp -d`
cleanup() {
 umount -l $dir/t
 rm -rf $dir
}

trap cleanup EXIT

echo "dir is $dir"
mkdir -p $dir/l $dir/u $dir/w $dir/t
mkdir $dir/l/dev
touch $dir/l/dev/foo.txt
mount -t overlay -o lowerdir=$dir/l,upperdir=$dir/u,workdir=$dir/w o $dir/t
stat $dir/t/dev
rm $dir/t/dev/foo.txt
rmdir $dir/t/dev
echo $?
echo "rmdir should have succeeded"
#===================================

[Launchpad Janitor (janitor)]

This bug was fixed in the package linux - 4.2.0-30.35

---------------
linux (4.2.0-30.35) wily; urgency=low

  [ Seth Forshee ]

  * SAUCE: cred: Add clone_cred() interface
    - LP: #1531747, #1534961, #1535150
    - CVE-2016-1575 CVE-2016-1576
  * SAUCE: overlayfs: Use mounter's credentials instead of selectively
    raising caps
    - LP: #1531747, #1534961, #1535150
    - CVE-2016-1575 CVE-2016-1576
  * SAUCE: overlayfs: Skip permission checking for trusted.overlayfs.*
    xattrs
    - LP: #1531747, #1534961, #1535150
    - CVE-2016-1575 CVE-2016-1576
  * SAUCE: overlayfs: Be more careful about copying up sxid files
    - LP: #1534961, #1535150
    - CVE-2016-1575 CVE-2016-1576
  * SAUCE: overlayfs: Propogate nosuid from lower and upper mounts
    - LP: #1534961, #1535150
    - CVE-2016-1575 CVE-2016-1576

linux (4.2.0-29.34) wily; urgency=low

  [ Luis Henriques ]

  * Release Tracking Bug
    - LP: #1543167

  [ Brad Figg ]

  * Revert "SAUCE: apparmor: fix sleep from invalid context"
    - LP: #1542049

  [ Upstream Kernel Changes ]

  * Revert "af_unix: Revert 'lock_interruptible' in stream receive code"
    - LP: #1540731

linux (4.2.0-28.33) wily; urgency=low

  [ Brad Figg ]

  * Release Tracking Bug
    - LP: #1540634

  [ Brad Figg ]

  * CONFIG: CONFIG_DEBUG_UART_BCM63XX is not set

  [ J. R. Okajima ]

  * SAUCE: ubuntu: aufs: tiny, extract a new func xino_fwrite_wkq()
    - LP: #1533043
  * SAUCE: ubuntu: aufs: for 4.3, XINO handles EINTR from the dying process
    - LP: #1533043

  [ John Johansen ]

  * SAUCE: (no-up): apparmor: fix for failed mediation of socket that is
    being shutdown
    - LP: #1446906
  * SAUCE: apparmor: fix sleep from invalid context
    - LP: #1539349

  [ Tim Gardner ]

  * [Config] Add pvpanic to virtual flavour
    - LP: #1537923

  [ Upstream Kernel Changes ]

  * Revert "ACPI / LPSS: allow to use specific PM domain during ->probe()"
    - LP: #1540532
  * tools: Add a "make all" rule
    - LP: #1536370
  * vf610_adc: Fix internal temperature calculation
    - LP: #1536370
  * iio: lpc32xx_adc: fix warnings caused by enabling unprepared clock
    - LP: #1536370
  * iio:ad5064: Make sure ad5064_i2c_write() returns 0 on success
    - LP: #1536370
  * iio: ad5064: Fix ad5629/ad5669 shift
    - LP: #1536370
  * iio:ad7793: Fix ad7785 product ID
    - LP: #1536370
  * iio: adc: vf610_adc: Fix division by zero error
    - LP: #1536370
  * mmc: mmc: Improve reliability of mmc_select_hs200()
    - LP: #1536370
  * mmc: mmc: Fix HS setting in mmc_select_hs400()
    - LP: #1536370
  * mmc: mmc: Move mmc_switch_status()
    - LP: #1536370
  * mmc: mmc: Improve reliability of mmc_select_hs400()
    - LP: #1536370
  * crypto: qat - don't use userspace pointer
    - LP: #1536370
  * iio: si7020: Swap data byte order
    - LP: #1536370
  * iio: adc: xilinx: Fix VREFN scale
    - LP: #1536370
  * ipmi: Start the timer and thread on internal msgs
    - LP: #1536370
  * drm/i915: quirk backlight present on Macbook 4, 1
    - LP: #1536370
  * drm/i915: get runtime PM reference around GEM set_caching IOCTL
    - LP: #1536370
  * drm/radeon: Disable uncacheable CPU mappings of GTT with RV6xx
    - LP: #1536370
  *...

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Launchpad Janitor (janitor)]

This bug was fixed in the package linux - 4.4.0-8.23

---------------
linux (4.4.0-8.23) xenial; urgency=low

  * cgroup namespace mounts broken in containers (LP: #1549398)
    - SAUCE: kernfs: Always set super block owner to init_user_ns

  * 4.4.0-7.22 no longer boots on arm64 (LP: #1547718)
    - arm64: mm: avoid calling apply_to_page_range on empty range
    - UBUNTU SAUCE: arm: mm: avoid calling apply_to_page_range on empty range

  * kernel install failed /bin/cp: cannot stat ‘/boot/initrd.img-4.3.0-7-generic’: No such file or directory (LP: #1536810)
    - [Config] postinst -- handle recreating symlinks when a real file is present

  * insecure overlayfs xattrs handling in copy_up (LP: #1534961)
    - SAUCE: cred: Add clone_cred() interface
    - SAUCE: overlayfs: Use mounter's credentials instead of selectively raising caps
    - SAUCE: overlayfs: Skip permission checking for trusted.overlayfs.* xattrs
    - SAUCE: overlayfs: Be more careful about copying up sxid files
    - SAUCE: overlayfs: Propogate nosuid from lower and upper mounts

  * overlayfs over fuse should refuse copy_up of files if uid/gid not mapped (LP: #1535150)
    - SAUCE: cred: Add clone_cred() interface
    - SAUCE: overlayfs: Use mounter's credentials instead of selectively raising caps
    - SAUCE: overlayfs: Skip permission checking for trusted.overlayfs.* xattrs
    - SAUCE: overlayfs: Be more careful about copying up sxid files
    - SAUCE: overlayfs: Propogate nosuid from lower and upper mounts

  * overlay: mkdir fails if directory exists in lowerdir in a user namespace (LP: #1531747)
    - SAUCE: cred: Add clone_cred() interface
    - SAUCE: overlayfs: Use mounter's credentials instead of selectively raising caps
    - SAUCE: overlayfs: Skip permission checking for trusted.overlayfs.* xattrs

  * Update Intel ethernet drivers to Fortville SW5 (LP: #1547674)
    - net: bulk free infrastructure for NAPI context, use napi_consume_skb
    - net: Add eth_platform_get_mac_address() helper.
    - i40e: Add mac_filter_element at the end of the list instead of HEAD
    - i40e/i40evf: Fix RSS rx-flow-hash configuration through ethtool
    - i40e: Replace X722 mac check in ethtool get_settings
    - i40evf: allow channel bonding of VFs
    - i40e: define function capabilities in only one place
    - i40evf: null out ring pointers on free
    - i40e: Cleanup the code with respect to restarting autoneg
    - i40e: update features with right offload
    - i40e: bump version to 1.4.10
    - i40e: add new device IDs for X722
    - i40e: Extend ethtool RSS hooks for X722
    - i40e/i40evf: Fix for UDP/TCP RSS for X722
    - i40evf: add new write-back mode
    - i40e/i40evf: Use private workqueue
    - i40e: add new proxy-wol bit for X722
    - i40e: Limit DCB FW version checks to X710/XL710 devices
    - i40e: AQ Add Run PHY Activity struct
    - i40e: AQ Geneve cloud tunnel type
    - i40e: AQ Add external power class to get link status
    - i40e: add 100Mb ethtool reporting
    - ixgbe: bulk free SKBs during TX completion cleanup cycle
    - igb: Remove unnecessary flag setting in igb_set_flag_queue_pairs()
    - igb: Unpair the queues when changing the number of queues...