[PATCH] tmpfs: verify {g,u}id mount options correctly

[PATCH] tmpfs: verify {g,u}id mount options correctly

[Christian Brauner]

A while ago we received the following report:

"The other outstanding issue I noticed comes from the fact that
fsconfig syscalls may occur in a different userns than that which
called fsopen. That means that resolving the uid/gid via
current_user_ns() can save a kuid that isn't mapped in the associated
namespace when the filesystem is finally mounted. This means that it
is possible for an unprivileged user to create files owned by any
group in a tmpfs mount (since we can set the SUID bit on the tmpfs
directory), or a tmpfs that is owned by any user, including the root
group/user."

The contract for {g,u}id mount options and {g,u}id values in general set
from userspace has always been that they are translated according to the
caller's idmapping. In so far, tmpfs has been doing the correct thing.
But since tmpfs is mountable in unprivileged contexts it is also
necessary to verify that the resulting {k,g}uid is representable in the
namespace of the superblock to avoid such bugs as above.

The new mount api's cross-namespace delegation abilities are already
widely used. After having talked to a bunch of userspace this is the
most faithful solution with minimal regression risks. I know of one
users - systemd - that makes use of the new mount api in this way and
they don't set unresolable {g,u}ids. So the regression risk is minimal.

Link: https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com
Fixes: f32356261d44 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API")
Reported-by: Seth Jenkins <sethjenkins@google.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---

---
 mm/shmem.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 2f2e0e618072..1c0b2dafafe5 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3636,6 +3636,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 	unsigned long long size;
 	char *rest;
 	int opt;
+	kuid_t kuid;
+	kgid_t kgid;
 
 	opt = fs_parse(fc, shmem_fs_parameters, param, &result);
 	if (opt < 0)
@@ -3671,14 +3673,32 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 		ctx->mode = result.uint_32 & 07777;
 		break;
 	case Opt_uid:
-		ctx->uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(ctx->uid))
+		kuid = make_kuid(current_user_ns(), result.uint_32);
+		if (!uid_valid(kuid))
 			goto bad_value;
+
+		/*
+		 * The requested uid must be representable in the
+		 * filesystem's idmapping.
+		 */
+		if (!kuid_has_mapping(fc->user_ns, kuid))
+			goto bad_value;
+
+		ctx->uid = kuid;
 		break;
 	case Opt_gid:
-		ctx->gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(ctx->gid))
+		kgid = make_kgid(current_user_ns(), result.uint_32);
+		if (!gid_valid(kgid))
 			goto bad_value;
+
+		/*
+		 * The requested gid must be representable in the
+		 * filesystem's idmapping.
+		 */
+		if (!kgid_has_mapping(fc->user_ns, kgid))
+			goto bad_value;
+
+		ctx->gid = kgid;
 		break;
 	case Opt_huge:
 		ctx->huge = result.uint_32;

---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230801-vfs-fs_context-uidgid-7756c8dcb1c0

Re: [PATCH] tmpfs: verify {g,u}id mount options correctly

[Seth Forshee]

On Tue, Aug 01, 2023 at 06:17:04PM +0200, Christian Brauner wrote:
> A while ago we received the following report:
> 
> "The other outstanding issue I noticed comes from the fact that
> fsconfig syscalls may occur in a different userns than that which
> called fsopen. That means that resolving the uid/gid via
> current_user_ns() can save a kuid that isn't mapped in the associated
> namespace when the filesystem is finally mounted. This means that it
> is possible for an unprivileged user to create files owned by any
> group in a tmpfs mount (since we can set the SUID bit on the tmpfs
> directory), or a tmpfs that is owned by any user, including the root
> group/user."
> 
> The contract for {g,u}id mount options and {g,u}id values in general set
> from userspace has always been that they are translated according to the
> caller's idmapping. In so far, tmpfs has been doing the correct thing.
> But since tmpfs is mountable in unprivileged contexts it is also
> necessary to verify that the resulting {k,g}uid is representable in the
> namespace of the superblock to avoid such bugs as above.
> 
> The new mount api's cross-namespace delegation abilities are already
> widely used. After having talked to a bunch of userspace this is the
> most faithful solution with minimal regression risks. I know of one
> users - systemd - that makes use of the new mount api in this way and
> they don't set unresolable {g,u}ids. So the regression risk is minimal.
> 
> Link: https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com
> Fixes: f32356261d44 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API")
> Reported-by: Seth Jenkins <sethjenkins@google.com>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> 
> ---
>  mm/shmem.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 2f2e0e618072..1c0b2dafafe5 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3636,6 +3636,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
>  	unsigned long long size;
>  	char *rest;
>  	int opt;
> +	kuid_t kuid;
> +	kgid_t kgid;
>  
>  	opt = fs_parse(fc, shmem_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -3671,14 +3673,32 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
>  		ctx->mode = result.uint_32 & 07777;
>  		break;
>  	case Opt_uid:
> -		ctx->uid = make_kuid(current_user_ns(), result.uint_32);
> -		if (!uid_valid(ctx->uid))
> +		kuid = make_kuid(current_user_ns(), result.uint_32);
> +		if (!uid_valid(kuid))
>  			goto bad_value;
> +
> +		/*
> +		 * The requested uid must be representable in the
> +		 * filesystem's idmapping.
> +		 */
> +		if (!kuid_has_mapping(fc->user_ns, kuid))
> +			goto bad_value;
> +
> +		ctx->uid = kuid;

This seems like the most sensible way to handle ids in mount options.
Wouldn't some other filesystems (e.g. fuse) benefit from the same sort
of handling though? Rather than having filesystems handle these checks
themselves, what about adding k{uid,gid}_t members to the
fs_parse_result union with fsparam_is_{uid,gid}() helpers which peform
these checks?

Seth

>  		break;
>  	case Opt_gid:
> -		ctx->gid = make_kgid(current_user_ns(), result.uint_32);
> -		if (!gid_valid(ctx->gid))
> +		kgid = make_kgid(current_user_ns(), result.uint_32);
> +		if (!gid_valid(kgid))
>  			goto bad_value;
> +
> +		/*
> +		 * The requested gid must be representable in the
> +		 * filesystem's idmapping.
> +		 */
> +		if (!kgid_has_mapping(fc->user_ns, kgid))
> +			goto bad_value;
> +
> +		ctx->gid = kgid;
>  		break;
>  	case Opt_huge:
>  		ctx->huge = result.uint_32;
> 
> ---
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> change-id: 20230801-vfs-fs_context-uidgid-7756c8dcb1c0
> 

Re: [PATCH] tmpfs: verify {g,u}id mount options correctly

[Christian Brauner]

On Tue, Aug 01, 2023 at 11:47:41AM -0500, Seth Forshee wrote:
> On Tue, Aug 01, 2023 at 06:17:04PM +0200, Christian Brauner wrote:
> > A while ago we received the following report:
> > 
> > "The other outstanding issue I noticed comes from the fact that
> > fsconfig syscalls may occur in a different userns than that which
> > called fsopen. That means that resolving the uid/gid via
> > current_user_ns() can save a kuid that isn't mapped in the associated
> > namespace when the filesystem is finally mounted. This means that it
> > is possible for an unprivileged user to create files owned by any
> > group in a tmpfs mount (since we can set the SUID bit on the tmpfs
> > directory), or a tmpfs that is owned by any user, including the root
> > group/user."
> > 
> > The contract for {g,u}id mount options and {g,u}id values in general set
> > from userspace has always been that they are translated according to the
> > caller's idmapping. In so far, tmpfs has been doing the correct thing.
> > But since tmpfs is mountable in unprivileged contexts it is also
> > necessary to verify that the resulting {k,g}uid is representable in the
> > namespace of the superblock to avoid such bugs as above.
> > 
> > The new mount api's cross-namespace delegation abilities are already
> > widely used. After having talked to a bunch of userspace this is the
> > most faithful solution with minimal regression risks. I know of one
> > users - systemd - that makes use of the new mount api in this way and
> > they don't set unresolable {g,u}ids. So the regression risk is minimal.
> > 
> > Link: https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com
> > Fixes: f32356261d44 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API")
> > Reported-by: Seth Jenkins <sethjenkins@google.com>
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > 
> > ---
> >  mm/shmem.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 2f2e0e618072..1c0b2dafafe5 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -3636,6 +3636,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> >  	unsigned long long size;
> >  	char *rest;
> >  	int opt;
> > +	kuid_t kuid;
> > +	kgid_t kgid;
> >  
> >  	opt = fs_parse(fc, shmem_fs_parameters, param, &result);
> >  	if (opt < 0)
> > @@ -3671,14 +3673,32 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> >  		ctx->mode = result.uint_32 & 07777;
> >  		break;
> >  	case Opt_uid:
> > -		ctx->uid = make_kuid(current_user_ns(), result.uint_32);
> > -		if (!uid_valid(ctx->uid))
> > +		kuid = make_kuid(current_user_ns(), result.uint_32);
> > +		if (!uid_valid(kuid))
> >  			goto bad_value;
> > +
> > +		/*
> > +		 * The requested uid must be representable in the
> > +		 * filesystem's idmapping.
> > +		 */
> > +		if (!kuid_has_mapping(fc->user_ns, kuid))
> > +			goto bad_value;
> > +
> > +		ctx->uid = kuid;
> 
> This seems like the most sensible way to handle ids in mount options.
> Wouldn't some other filesystems (e.g. fuse) benefit from the same sort
> of handling though? Rather than having filesystems handle these checks
> themselves, what about adding k{uid,gid}_t members to the
> fs_parse_result union with fsparam_is_{uid,gid}() helpers which peform
> these checks?

Yes, I like that proposal. Let's see if that works.

Re: [PATCH] tmpfs: verify {g,u}id mount options correctly

[Seth Forshee]

On Wed, Aug 02, 2023 at 02:06:26PM +0200, Christian Brauner wrote:
> On Tue, Aug 01, 2023 at 11:47:41AM -0500, Seth Forshee wrote:
> > On Tue, Aug 01, 2023 at 06:17:04PM +0200, Christian Brauner wrote:
> > > A while ago we received the following report:
> > > 
> > > "The other outstanding issue I noticed comes from the fact that
> > > fsconfig syscalls may occur in a different userns than that which
> > > called fsopen. That means that resolving the uid/gid via
> > > current_user_ns() can save a kuid that isn't mapped in the associated
> > > namespace when the filesystem is finally mounted. This means that it
> > > is possible for an unprivileged user to create files owned by any
> > > group in a tmpfs mount (since we can set the SUID bit on the tmpfs
> > > directory), or a tmpfs that is owned by any user, including the root
> > > group/user."
> > > 
> > > The contract for {g,u}id mount options and {g,u}id values in general set
> > > from userspace has always been that they are translated according to the
> > > caller's idmapping. In so far, tmpfs has been doing the correct thing.
> > > But since tmpfs is mountable in unprivileged contexts it is also
> > > necessary to verify that the resulting {k,g}uid is representable in the
> > > namespace of the superblock to avoid such bugs as above.
> > > 
> > > The new mount api's cross-namespace delegation abilities are already
> > > widely used. After having talked to a bunch of userspace this is the
> > > most faithful solution with minimal regression risks. I know of one
> > > users - systemd - that makes use of the new mount api in this way and
> > > they don't set unresolable {g,u}ids. So the regression risk is minimal.
> > > 
> > > Link: https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com
> > > Fixes: f32356261d44 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API")
> > > Reported-by: Seth Jenkins <sethjenkins@google.com>
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > ---
> > > 
> > > ---
> > >  mm/shmem.c | 28 ++++++++++++++++++++++++----
> > >  1 file changed, 24 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index 2f2e0e618072..1c0b2dafafe5 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -3636,6 +3636,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> > >  	unsigned long long size;
> > >  	char *rest;
> > >  	int opt;
> > > +	kuid_t kuid;
> > > +	kgid_t kgid;
> > >  
> > >  	opt = fs_parse(fc, shmem_fs_parameters, param, &result);
> > >  	if (opt < 0)
> > > @@ -3671,14 +3673,32 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
> > >  		ctx->mode = result.uint_32 & 07777;
> > >  		break;
> > >  	case Opt_uid:
> > > -		ctx->uid = make_kuid(current_user_ns(), result.uint_32);
> > > -		if (!uid_valid(ctx->uid))
> > > +		kuid = make_kuid(current_user_ns(), result.uint_32);
> > > +		if (!uid_valid(kuid))
> > >  			goto bad_value;
> > > +
> > > +		/*
> > > +		 * The requested uid must be representable in the
> > > +		 * filesystem's idmapping.
> > > +		 */
> > > +		if (!kuid_has_mapping(fc->user_ns, kuid))
> > > +			goto bad_value;
> > > +
> > > +		ctx->uid = kuid;
> > 
> > This seems like the most sensible way to handle ids in mount options.
> > Wouldn't some other filesystems (e.g. fuse) benefit from the same sort
> > of handling though? Rather than having filesystems handle these checks
> > themselves, what about adding k{uid,gid}_t members to the
> > fs_parse_result union with fsparam_is_{uid,gid}() helpers which peform
> > these checks?
> 
> Yes, I like that proposal. Let's see if that works.

After a little poking around, this is more complicated than I'd
initially thought. The parameter helpers don't currently get passed an
fs_context, and ceph/rbd seem to be using the parameter parsing like a
library when there legitimately is not an fs_context to be passed. So it
makes sense to take this patch as an immediate fix, and we can take a
look at trying to make it more generic later.


Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>

Re: [PATCH] tmpfs: verify {g,u}id mount options correctly

[Christian Brauner]

On Tue, 01 Aug 2023 18:17:04 +0200, Christian Brauner wrote:
> A while ago we received the following report:
> 
> "The other outstanding issue I noticed comes from the fact that
> fsconfig syscalls may occur in a different userns than that which
> called fsopen. That means that resolving the uid/gid via
> current_user_ns() can save a kuid that isn't mapped in the associated
> namespace when the filesystem is finally mounted. This means that it
> is possible for an unprivileged user to create files owned by any
> group in a tmpfs mount (since we can set the SUID bit on the tmpfs
> directory), or a tmpfs that is owned by any user, including the root
> group/user."
> 
> [...]

Applied to the vfs.tmpfs branch of the vfs/vfs.git tree.
Patches in the vfs.tmpfs branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.tmpfs

[1/1] tmpfs: verify {g,u}id mount options correctly
      https://git.kernel.org/vfs/vfs/c/f90277cb4cae