2010-02-04 17:58:13

by Miklos Szeredi

[permalink] [raw]
Subject: [RFC PATCH] vfs: add MNT_NOFOLLOW flag to umount(2)

From: Miklos Szeredi <[email protected]>

Add a new MNT_NOFOLLOW flag to umount(2). This is needed to prevent
symlink attacks in unprivileged unmounts (fuse, samba, ncpfs).

Additionally, return -EINVAL if an unknown flag is encountered. This
makes it possible for the caller to determine if a flag is supported
or not (at least on kernels with this patch).

Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/namespace.c | 9 ++++++++-
include/linux/fs.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)

Index: linux-2.6/fs/namespace.c
===================================================================
--- linux-2.6.orig/fs/namespace.c 2010-01-29 09:21:45.000000000 +0100
+++ linux-2.6/fs/namespace.c 2010-02-04 10:03:08.000000000 +0100
@@ -1121,8 +1121,15 @@ SYSCALL_DEFINE2(umount, char __user *, n
{
struct path path;
int retval;
+ int lookup_flags = 0;

- retval = user_path(name, &path);
+ if (flags & ~(MNT_FORCE | MNT_DETACH | MNT_EXPIRE | MNT_NOFOLLOW))
+ return -EINVAL;
+
+ if (!(flags & MNT_NOFOLLOW))
+ lookup_flags |= LOOKUP_FOLLOW;
+
+ retval = user_path_at(AT_FDCWD, name, lookup_flags, &path);
if (retval)
goto out;
retval = -EINVAL;
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h 2010-01-22 08:46:57.000000000 +0100
+++ linux-2.6/include/linux/fs.h 2010-02-04 09:57:39.000000000 +0100
@@ -1305,6 +1305,7 @@ extern int send_sigurg(struct fown_struc
#define MNT_FORCE 0x00000001 /* Attempt to forcibily umount */
#define MNT_DETACH 0x00000002 /* Just detach from the tree */
#define MNT_EXPIRE 0x00000004 /* Mark for expiry */
+#define MNT_NOFOLLOW 0x00000008 /* Don't follow symlink on umount */

extern struct list_head super_blocks;
extern spinlock_t sb_lock;


2010-02-04 18:48:34

by Bodo Eggert

[permalink] [raw]
Subject: Re: [RFC PATCH] vfs: add MNT_NOFOLLOW flag to umount(2)

Miklos Szeredi <[email protected]> wrote:

> Additionally, return -EINVAL if an unknown flag is encountered. This
> makes it possible for the caller to determine if a flag is supported
> or not (at least on kernels with this patch).

There should be a guaranteed-to-be-invalid flag or flag-combination in
order to safely detect this feature.

2010-02-05 08:14:20

by Miklos Szeredi

[permalink] [raw]
Subject: Re: [RFC PATCH] vfs: add MNT_NOFOLLOW flag to umount(2)

On Thu, 04 Feb 2010, Bodo Eggert wrote:
> Miklos Szeredi <[email protected]> wrote:
>
> > Additionally, return -EINVAL if an unknown flag is encountered. This
> > makes it possible for the caller to determine if a flag is supported
> > or not (at least on kernels with this patch).
>
> There should be a guaranteed-to-be-invalid flag or flag-combination in
> order to safely detect this feature.

It's difficult though, because the app would have to make sure the
detection itself would reliably fail, and with a different error.

Simply checking the kernel version might be easier.

Thanks,
Miklos

2010-02-08 10:37:45

by Bodo Eggert

[permalink] [raw]
Subject: Re: [RFC PATCH] vfs: add MNT_NOFOLLOW flag to umount(2)

On Fri, 5 Feb 2010, Miklos Szeredi wrote:

> On Thu, 04 Feb 2010, Bodo Eggert wrote:
>> Miklos Szeredi <[email protected]> wrote:
>>
>>> Additionally, return -EINVAL if an unknown flag is encountered. This
>>> makes it possible for the caller to determine if a flag is supported
>>> or not (at least on kernels with this patch).
>>
>> There should be a guaranteed-to-be-invalid flag or flag-combination in
>> order to safely detect this feature.
>
> It's difficult though, because the app would have to make sure the
> detection itself would reliably fail, and with a different error.
>
> Simply checking the kernel version might be easier.

Yes, that's exactly what I intend to make possible:

| ret = umount("/tmp/mkstmp", UMOUNT_IMPOSSIBLE_FLAG);
| if (ret != -1 || errno != -EINVAL)
| goto extended_mount_flags_are_not_supported;

Checking the kernel version is a bad idea, because the application might
be portable, or features might be backported. ??