setgid bit in chmod dropped inconsistently

[Blub]

System information

Type	Version/Name
Distribution Name	Debian
Distribution Version	Stretch
Linux Kernel	4.13.4-1-pve
Architecture	x86_64
ZFS Version	tested git-master & 0.7.2
SPL Version	tested git-master & 0.7.2

Describe the problem you're observing

The setgid bit of a chmod() call as root (with all usual capabilities
(CAP_FSETID, CAP_FOWNER, ...) from within a user namespace drops the setgid bit
if the user (root) is the owner of the file while the file belongs to a group
one is currently not part of.

# id
uid=0(root) gid=0(root) groups=0(root)
# touch asdf
# chmod 2755 asdf
# ls -l asdf
-rwxr-sr-x 1 root root 0 Oct 30 15:33 asdf     <- ok - worked
# chgrp 88 asdf
# ls -l asdf
-rwxr-xr-x 1 root 88 0 Oct 30 15:33 asdf       <- ok - lost setgid as as expected
# chmod 2755 asdf
# ls -l asdf
-rwxr-xr-x 1 root 88 0 Oct 30 15:33 asdf       <- wrong - setgid ignored
# chown 88 asdf
# chmod 2755 asdf
# ls -l asdf
-rwxr-sr-x 1 88 88 0 Oct 30 15:33 asdf         <- ok - setgid honored (inconsistent!)

Note that as root one can simply use setgroups() to work around this issue
usually.

This seems to have happened together with the zfs allow change set. At least
when adapting secpolicy_vnode_setids_setgids() in module/zfs/policy.c to use
an ns_capable(current_user_ns(), cap) check rather than the regular
capable() check from priv_policy() the setgid bit is not dropped anymore.
I used the following change to test (it also adds a check for whether the gid
has a mapping in the current namespace):
Blub@dd97fcf

I'm not sure whether the other functions using priv_policy() also require more
user namespace specific adjustments, and the comment above the function doesn't
help me figure that out ;-)
I was, however, unable to find additional easily triggerable code-paths which
misbehave in user namespaces, but I'm quite new to the code, so there is that...

[behlendorf]

I'm not sure whether the other functions using priv_policy() also require more
user namespace specific adjustments.

Nice find. In fact, I believe priv_policy() can be updated to always check the user namespace. But we'll have to do some manual testing to verify this is true. We should add some user namespace tests to the ZFS Test Suite to verify the behavior is correct.

[MarkGavalda]

I can confirm this affects all releases since (at least) 0.7.1, we're running ZFS on lots of Ubuntu servers and all of them are affected that have been upgraded to 0.7.x. Thanks for the detailed tests @Blub we were able to work around this bug with your help.

[Blub]

After reading through more the zfs & user namespace related kernel codes I was wondering if something like this was going in the right direction: master...Blub:rfc-wip-userns-policy
This still lacks id mapping in the allow ioctl, so user namespaces still see the "real" uids/gids, which makes the allow permission somewhat messy with user namespaces.
The first patch there is the important one to fix the regression, so I'd like to get some version of that merged soon if possible, as for the rest, I'd like some feedback before investing more time :-)

[behlendorf]

@Blub yes, I think your on exactly the right track with this and should continue. I'm particularly happy to see you've added some initial coverage for user namespaces!

[Blub]

While adding uid/gid mapping for ZFS_IOC_{GET,SET}_FSACL I was wondering if there's a reason why some of the loops over nvlists take the nvpair's name and then do an nvlist_lookup_* instead of using nvpair_value_*, for instance module/zcommon/zfs_deleg.c's zfs_deleg_verify_nvlist(). In my mapping code using nvpair_value_nvlist() seems to work as well? Since the mentioned function explicitly handles ENOENT separately I thought it might be a concurrency thing, but the function is only used coming from these ioctls, so the data should only be accessed by one thread in this case.

[behlendorf]

Depending on the flags passed when the nvlist_t was created there may or may not be a difference. If the nvlist_t was created with the NV_UNIQUE_NAME* flag then they're going to be equivalent. Otherwise the list may contain duplicate names which the lookup variant may return instead. As for concurrency the caller is responsible for any locking required.

[stgraber]

@Blub as there been some progress on a fix for this?

We (LXD) have just noticed a number of our users start hitting this as they switched to distributions shipping 0.7.x, most notably the upcoming Ubuntu 18.04 LTS.

Having package installations silently drop setgid makes for extremely annoying bugs to figure out...

[Blub]

For that particular patch to be merged the only missing parts are the detection of older kernels without ns_capable/cred->user_ns. I really need to give this a higher priority now and should get separate this part out into its own PR.