Temporarily use root credentials to mount snapshots in .zfs

[allanjude]

When mounting a snapshot in the .zfs/snapshots control directory,
temporarily assume roots credentials to perform the VFS_MOUNT().

This allows regular users and users inside jails to access these
snapshots.

The regular usermount code is not helpful here, since it requires
that the user performing the mount own the mountpoint, which won't
be the case for .zfs/snapshot/

Signed-off-by: Allan Jude allan@klarasystems.com
Sponsored-By: Modirum MDPay
Sponsored-By: Klara Inc.

Motivation and Context

In eff6210 the check used by INGLOBALZONE changed from checking if the first thread of the process was jailed, to checking the entire process it self. This simplication is good, but it broke the credential swapping used in mount_snapshot(). This patch changes that credential swapping to apply to both the thread and the process credentials, so any future access checks pass properly.

Description

Temporarily assume root credentials for curthread and curproc

How Has This Been Tested?

A regular user, and users in a jail can now mount and read from snapshots via the .zfs/snapshot directory.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[ghost]

Changing the credentials for the whole process can't be good. Wouldn't other threads then be able to do things as if they were root?

[ghost]

If reverting eff6210 fixes the issue, that might just be the better solution for now.

[sempervictus]

This does look a bit concerning from a security perspective, moreso if that code is built in userspace as well. If nothing else, that's an awesome ROP gadget to hit, but might have less esoteric approaches as well (glanced over the code, will try to gain context when i have some time).

[behlendorf]

Wouldn't other threads then be able to do things as if they were root?

Looking at the patch I don't think so. This path is only reachable from the .zfs/snapshot lookup code, it's only built for the kernel module, and the kernel credential is only assumed for the duration of the mount. This definitely feels a little evil as mentioned in the comment but it seems OK unless VFS_MOUNT can somehow be co-opted to something else. I'm not familiar enough with FreeBSD to know if that's really possible or not.

[mjguzik]

This introduces a security bug. Normally everything is authorized with per-thread credentials. Another thread in the same process can race against mount in order to get (now root) proc credentials assigned to itself and retaining root privs even after mount is done.

The previous iteration of the globalzone macro contains a use-after-free:
!jailed(FIRST_THREAD_IN_PROC((p))->td_ucred));

if curthread != FIRST_THREAD_IN_PROC(p), then td_ucred can't safely accessed. It is rather unclear to me why any of these shenanigans to begin with instead of just using curthread->td_ucred.

I wonder if replacing creds for curthread is safe or necessary to begin with. If this is to make INGLOBALZONE pass here and there, perhaps the affected code should be modified instead to allow non-globalzone users to mount stuff? I'm mostly worried about possibility of mounting a snapshot over an arbitrary directory.

[allanjude]

This introduces a security bug. Normally everything is authorized with per-thread credentials. Another thread in the same process can race against mount in order to get (now root) proc credentials assigned to itself and retaining root privs even after mount is done.

The previous iteration of the globalzone macro contains a use-after-free: !jailed(FIRST_THREAD_IN_PROC((p))->td_ucred));

if curthread != FIRST_THREAD_IN_PROC(p), then td_ucred can't safely accessed. It is rather unclear to me why any of these shenanigans to begin with instead of just using curthread->td_ucred.

Here is the original code from FreeBSD 12:
https://cgit.freebsd.org/src/tree/sys/cddl/compat/opensolaris/kern/opensolaris_vfs.c?h=stable/12#n189

When eff6210 changed INGLOBALZONE() to check the thread creds, not the proc creds, it broke the ability to visit the .zfs/snapshot/<name> directory inside the jail as a regular user, which had always worked on FreeBSD.

What is the right change to maintain this feature?

[allanjude]

I wonder if replacing creds for curthread is safe or necessary to begin with. If this is to make INGLOBALZONE pass here and there, perhaps the affected code should be modified instead to allow non-globalzone users to mount stuff? I'm mostly worried about possibility of mounting a snapshot over an arbitrary directory.

In this case, it is to call VFS_MOUNT(mp) with root credentials, even though curthread only has regular user (non root) creds

[mjguzik]

This introduces a security bug. Normally everything is authorized with per-thread credentials. Another thread in the same process can race against mount in order to get (now root) proc credentials assigned to itself and retaining root privs even after mount is done.
The previous iteration of the globalzone macro contains a use-after-free: !jailed(FIRST_THREAD_IN_PROC((p))->td_ucred));
if curthread != FIRST_THREAD_IN_PROC(p), then td_ucred can't safely accessed. It is rather unclear to me why any of these shenanigans to begin with instead of just using curthread->td_ucred.

Here is the original code from FreeBSD 12: https://cgit.freebsd.org/src/tree/sys/cddl/compat/opensolaris/kern/opensolaris_vfs.c?h=stable/12#n189

When eff6210 changed INGLOBALZONE() to check the thread creds, not the proc creds, it broke the ability to visit the .zfs/snapshot/<name> directory inside the jail as a regular user, which had always worked on FreeBSD.

What is the right change to maintain this feature?

Find whatever places which deny access and patch them.

[jrtc27]

I see no progress on this issue for the past 5 months despite the privilege escalation vulnerability introduced; the code in question is still there.

[behlendorf]

@allanjude @mjguzik it sounds like reverting both 4a1195c and eff6210 would resolve this. If that's the case, then let's do so until someone puts together the right patch for this functionality.

[mjguzik]

Just doing that brings back the use-after-free:

return (!jailed(FIRST_THREAD_IN_PROC((p))->td_ucred));

this should just use curthread->td_ucred, except that somehow causes a regression(?). worst case as a quick hack the IN_GLOBALZONE macro can be patched to PROC_LOCK around the check.

[ghost]

I'm working on fixing this properly.

[allanjude]

I have a cleaner fix, let me post that for everyone to see. I had happened to take it a bit further with some sysctls too, but, lets see what people think first.

[allanjude]

See: #13758