From: Miklos Szeredi <[email protected]>
Add a new UMOUNT_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 used (and specify
an explicitly unused flag: UMOUNT_UNUSED). This makes it possible for
the caller to determine if a flag is supported or not.
CC: Eugene Teo <[email protected]>
CC: Michael Kerrisk <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
---
v2:
- renamed flag to UMOUNT_NOFOLLOW
- added UMOUNT_UNUSED for feature detection
fs/namespace.c | 9 ++++++++-
include/linux/fs.h | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
Index: linux-2.6/fs/namespace.c
===================================================================
--- linux-2.6.orig/fs/namespace.c 2010-02-08 12:00:02.000000000 +0100
+++ linux-2.6/fs/namespace.c 2010-02-09 13:38:03.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 | UMOUNT_NOFOLLOW))
+ return -EINVAL;
+
+ if (!(flags & UMOUNT_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-02-08 12:00:03.000000000 +0100
+++ linux-2.6/include/linux/fs.h 2010-02-09 13:35:47.000000000 +0100
@@ -1305,6 +1305,8 @@ 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 UMOUNT_NOFOLLOW 0x00000008 /* Don't follow symlink on umount */
+#define UMOUNT_UNUSED 0x80000000 /* Flag guaranteed to be unused */
extern struct list_head super_blocks;
extern spinlock_t sb_lock;
On Wed, Feb 10, 2010 at 12:15:53PM +0100, Miklos Szeredi wrote:
> - renamed flag to UMOUNT_NOFOLLOW
> - added UMOUNT_UNUSED for feature detection
Umm, why? MNT_ certainly isn't the best naming for unmount flags,
but switching convention after we had a few doesn't make any sense.
On Thu, 11 Feb 2010, Christoph Hellwig wrote:
> On Wed, Feb 10, 2010 at 12:15:53PM +0100, Miklos Szeredi wrote:
> > - renamed flag to UMOUNT_NOFOLLOW
> > - added UMOUNT_UNUSED for feature detection
>
> Umm, why? MNT_ certainly isn't the best naming for unmount flags,
> but switching convention after we had a few doesn't make any sense.
It's not just bad naming, having another set of unrelated flags with
the same prefix *and* in close proximity to each other is asking for
trouble.
We could rename all the umount flags, and leave userspace do #defines
for backward compat. It's not as if this was some widely used or
portable API.
Thanks,
Miklos
On Thu, Feb 11, 2010 at 12:21:00PM -0500, Christoph Hellwig wrote:
> On Wed, Feb 10, 2010 at 12:15:53PM +0100, Miklos Szeredi wrote:
> > - renamed flag to UMOUNT_NOFOLLOW
> > - added UMOUNT_UNUSED for feature detection
>
> Umm, why? MNT_ certainly isn't the best naming for unmount flags,
> but switching convention after we had a few doesn't make any sense.
Actually, I've got more interesting question: what's being attempted
there? Is that just a "let's protect ourselves against somebody feeding
us an untrusted symlink"? I'm not sure if it makes much sense; if we
are dealing with pathnames on untrusted fs, there's nothing to stop the
attacker from having /mnt/foo/dir (originally containing a mountpoint
at /mnt/foo/dir/usr) killed and replaced with a symlink to /, making any
code that does umount() on such pathnames vulnerable as hell anyway.
Lack of LOOKUP_FOLLOW affects only the last pathname component. So what
is that patch trying to make safe?
On Sun, 21 Feb 2010, Al Viro wrote:
> On Thu, Feb 11, 2010 at 12:21:00PM -0500, Christoph Hellwig wrote:
> > On Wed, Feb 10, 2010 at 12:15:53PM +0100, Miklos Szeredi wrote:
> > > - renamed flag to UMOUNT_NOFOLLOW
> > > - added UMOUNT_UNUSED for feature detection
> >
> > Umm, why? MNT_ certainly isn't the best naming for unmount flags,
> > but switching convention after we had a few doesn't make any sense.
>
> Actually, I've got more interesting question: what's being attempted
> there? Is that just a "let's protect ourselves against somebody feeding
> us an untrusted symlink"? I'm not sure if it makes much sense; if we
> are dealing with pathnames on untrusted fs, there's nothing to stop the
> attacker from having /mnt/foo/dir (originally containing a mountpoint
> at /mnt/foo/dir/usr) killed and replaced with a symlink to /, making any
> code that does umount() on such pathnames vulnerable as hell anyway.
It is trivial to check the path up to the mountpoint (chdir + getcwd).
But doing that on the mountpoint will make it busy, so NOFOLLOW is
really needed there.
Thanks,
Miklos