[PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW

[PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW

[demiobenour]

From: "Demi M. Obenour" <demiobenour@gmail.com>

This adds the flag LOOKUP_NEVER_FOLLOW to path resolution, which tells
the code in fs/namei.c to never follow symlinks.  This flag overrides
LOOKUP_FOLLOW, since this makes internal APIs simpler: code can set the
flag without needing to also clear LOOKUP_FOLLOW, which is often set by
default.

This is a prerequisite to adding O_PATHSTATIC, but is also useful for
kernel-internal use.
---
 fs/namei.c            | 4 +++-
 include/linux/namei.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 914178cdbe94..54fbd2c7ba82 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1720,7 +1720,9 @@ static int pick_link(struct nameidata *nd, struct path *link,
 {
 	int error;
 	struct saved *last;
-	if (unlikely(nd->total_link_count++ >= MAXSYMLINKS)) {
+	const int max_symlinks = (nd->flags & LOOKUP_NEVER_FOLLOW) ?
+		0 : MAXSYMLINKS;
+	if (unlikely(nd->total_link_count++ >= max_symlinks)) {
 		path_to_nameidata(link, nd);
 		return -ELOOP;
 	}
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..f065502a653d 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -24,10 +24,12 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
  *  - internal "there are more path components" flag
  *  - dentry cache is untrusted; force a real lookup
  *  - suppress terminal automount
+ *  - never follow symbolic links, even internally
  */
 #define LOOKUP_FOLLOW		0x0001
 #define LOOKUP_DIRECTORY	0x0002
 #define LOOKUP_AUTOMOUNT	0x0004
+#define LOOKUP_NEVER_FOLLOW	0x0008
 
 #define LOOKUP_PARENT		0x0010
 #define LOOKUP_REVAL		0x0020
-- 
2.20.1

[PATCH 2/4] Expose O_PATHSTATIC to userspace

[demiobenour]

From: "Demi M. Obenour" <demiobenour@gmail.com>

This adds the file open flag O_PATHSTATIC, which ensures that symbolic
links are *never* followed, even in path components other than the last.
This is distinct from O_NOFOLLOW, which only prevents symlinks in the
*last* component from being followed.

This is useful for avoiding race conditions in userspace code that
should expose only a subset of the filesystem to clients.  This includes
FTP and SFTP servers, QEMU, and others.

Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set.  Otherwise,
open() fails with -EINVAL.
---
 fs/fcntl.c                       |  2 +-
 fs/namei.c                       |  6 ++++++
 fs/open.c                        | 24 ++++++++++++++++++++++--
 include/linux/fcntl.h            |  2 +-
 include/uapi/asm-generic/fcntl.h |  4 ++++
 5 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..6c85c4d0c006 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
 	 * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
 	 * is defined as O_NONBLOCK on some platforms and not on others.
 	 */
-	BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+	BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
 		HWEIGHT32(
 			(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
 			__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index 54fbd2c7ba82..4c90f265c103 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3282,6 +3282,12 @@ static int do_last(struct nameidata *nd,
 	if (!(open_flag & O_CREAT)) {
 		if (nd->last.name[nd->last.len])
 			nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+
+		if (open_flag & O_PATHSTATIC) {
+			nd->flags |= LOOKUP_NEVER_FOLLOW;
+			nd->flags &= ~LOOKUP_FOLLOW;
+		}
+
 		/* we _can_ be in RCU mode here */
 		error = lookup_fast(nd, &path, &inode, &seq);
 		if (likely(error > 0))
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..717afa8179c0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -940,6 +940,24 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
 	/* Must never be set by userspace */
 	flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC;
 
+	/*
+	 * If nonzero, setting O_PATHSTATIC but not O_NOFOLLOW fails with
+	 * -EINVAL.  Otherwise, setting O_PATHSTATIC automatically sets
+	 * O_NOFOLLOW.
+	 */
+#define REQUIRE_NOFOLLOW_FOR_PATHSTATIC 1
+
+#if REQUIRE_NOFOLLOW_FOR_PATHSTATIC
+	/* O_PATHSTATIC doesn't make sense without O_NOFOLLOW */
+	if (unlikely((flags & O_PATHSTATIC) && !(flags & O_NOFOLLOW)))
+		return -EINVAL;
+#elif defined REQUIRE_NOFOLLOW_FOR_PATHSTATIC
+	if (flags & O_PATHSTATIC)
+		flags &= O_NOFOLLOW;
+#else
+#error REQUIRE_NOFOLLOW_FOR_PATHSTATIC must be defined
+#endif
+
 	/*
 	 * O_SYNC is implemented as __O_SYNC|O_DSYNC.  As many places only
 	 * check for O_DSYNC if the need any syncing at all we enforce it's
@@ -959,7 +977,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
 		 * If we have O_PATH in the open flag. Then we
 		 * cannot have anything other than the below set of flags
 		 */
-		flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
+		flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_PATHSTATIC;
 		acc_mode = 0;
 	}
 
@@ -986,7 +1004,9 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
 
 	if (flags & O_DIRECTORY)
 		lookup_flags |= LOOKUP_DIRECTORY;
-	if (!(flags & O_NOFOLLOW))
+	if (flags & O_PATHSTATIC)
+		lookup_flags |= LOOKUP_NEVER_FOLLOW;
+	else if (!(flags & O_NOFOLLOW))
 		lookup_flags |= LOOKUP_FOLLOW;
 	op->lookup_flags = lookup_flags;
 	return 0;
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..6f91e1490592 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,7 @@
 	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
 	 O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
 	 FASYNC	| O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
-	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_PATHSTATIC)
 
 #ifndef force_o_largefile
 #define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..314ea1cecf44 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,10 @@
 #define __O_TMPFILE	020000000
 #endif
 
+#ifndef O_PATHSTATIC
+#define O_PATHSTATIC	040000000
+#endif
+
 /* a horrid kludge trying to make sure that this will fail on old kernels */
 #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
 #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)      
-- 
2.20.1

Re: [PATCH 2/4] Expose O_PATHSTATIC to userspace

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

On Feb 12, 2019, at 7:54 AM, demiobenour@gmail.com wrote:
> 
> From: "Demi M. Obenour" <demiobenour@gmail.com>
> 
> This adds the file open flag O_PATHSTATIC, which ensures that symbolic
> links are *never* followed, even in path components other than the last.
> This is distinct from O_NOFOLLOW, which only prevents symlinks in the
> *last* component from being followed.
> 
> This is useful for avoiding race conditions in userspace code that
> should expose only a subset of the filesystem to clients.  This includes
> FTP and SFTP servers, QEMU, and others.
> 
> Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set.  Otherwise,
> open() fails with -EINVAL.

I don't want to bikeshed (discard suggestion if you disagree), but why not
name the flag "O_NEVER_FOLLOW" so that users can see it is also related to
"O_NOFOLLOW"?  Otherwise it seems like they are two completely different
things from looking at the names, when in fact they are closely related.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 2/4] Expose O_PATHSTATIC to userspace

[Demi M. Obenour]

[-- Attachment #1.1: Type: text/plain, Size: 1425 bytes --]



On 2/12/19 3:18 PM, Andreas Dilger wrote:
> On Feb 12, 2019, at 7:54 AM, demiobenour@gmail.com wrote:
>>
>> From: "Demi M. Obenour" <demiobenour@gmail.com>
>>
>> This adds the file open flag O_PATHSTATIC, which ensures that symbolic
>> links are *never* followed, even in path components other than the last.
>> This is distinct from O_NOFOLLOW, which only prevents symlinks in the
>> *last* component from being followed.
>>
>> This is useful for avoiding race conditions in userspace code that
>> should expose only a subset of the filesystem to clients.  This includes
>> FTP and SFTP servers, QEMU, and others.
>>
>> Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set.  Otherwise,
>> open() fails with -EINVAL.
> 
> I don't want to bikeshed (discard suggestion if you disagree), but why not
> name the flag "O_NEVER_FOLLOW" so that users can see it is also related to
> "O_NOFOLLOW"?  Otherwise it seems like they are two completely different
> things from looking at the names, when in fact they are closely related.
> 
> Cheers, Andreas
> 

Searching for O_PATHSTATIC gives two results:

* https://www.halfdog.net/Security/2010/FilesystemRecursionAndSymlinks
* https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06225.html

O_DIRECTORY_NOFOLLOW would also be a good choice, since that is what the
flag actually does.  Ideally, we would rename O_NOFOLLOW, but we can’t.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] Add AT_PATHSTATIC to linkat()

[demiobenour]

From: "Demi M. Obenour" <demiobenour@gmail.com>

This has the same meaning as O_PATHSTATIC does in openat(), and has the
same uses.
---
 fs/namei.c                 | 8 +++++++-
 include/uapi/linux/fcntl.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 4c90f265c103..b47f89af00f2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4291,8 +4291,14 @@ int do_linkat(int olddfd, const char __user *oldname, int newdfd,
 		how = LOOKUP_EMPTY;
 	}
 
-	if (flags & AT_SYMLINK_FOLLOW)
+	if (flags & AT_SYMLINK_FOLLOW) {
+		if (flags & AT_PATHSTATIC)
+			return -EINVAL;
 		how |= LOOKUP_FOLLOW;
+	}
+
+	if (flags & AT_PATHSTATIC)
+		how |= LOOKUP_NEVER_FOLLOW;
 retry:
 	error = user_path_at(olddfd, oldname, how, &old_path);
 	if (error)
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6448cdd9a350..a2f65635c8fc 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -89,6 +89,7 @@
 #define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
 #define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
 #define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
+#define AT_PATHSTATIC		0x8000	/* Do not follow symbolic links anywhere. */
 
 
 #endif /* _UAPI_LINUX_FCNTL_H */
-- 
2.20.1

[PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open()

[demiobenour]

From: "Demi M. Obenour" <demiobenour@gmail.com>

While testing the O_PATHSTATIC patch, I discovered that Linux does not
return any error if an invalid flag is passed to open(2).  This prevents
adding new flags without a (minor) risk of breaking userspace.
Therefore, add a check for invalid flags, and return -EINVAL if any are
found.
---
 fs/open.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/open.c b/fs/open.c
index 717afa8179c0..eeaa2eeb342a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1074,6 +1074,13 @@ long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode)
 	if (fd)
 		return fd;
 
+	/*
+	 * Enforce that open flags are valid, to ensure that new flags can be
+	 * added later.
+	 */
+	if (unlikely(flags & ~VALID_OPEN_FLAGS))
+		return -EINVAL;
+
 	tmp = getname(filename);
 	if (IS_ERR(tmp))
 		return PTR_ERR(tmp);
-- 
2.20.1

Re: [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open()

[Al Viro]

On Tue, Feb 12, 2019 at 09:54:47AM -0500, demiobenour@gmail.com wrote:
> From: "Demi M. Obenour" <demiobenour@gmail.com>
> 
> While testing the O_PATHSTATIC patch, I discovered that Linux does not
> return any error if an invalid flag is passed to open(2).  This prevents
> adding new flags without a (minor) risk of breaking userspace.
> Therefore, add a check for invalid flags, and return -EINVAL if any are
> found.

... which would qualtify as userland ABI breakage all by itself.

Re: [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open()

[Demi M. Obenour]

[-- Attachment #1.1: Type: text/plain, Size: 843 bytes --]



On 2/12/19 3:38 PM, Al Viro wrote:
> On Tue, Feb 12, 2019 at 09:54:47AM -0500, demiobenour@gmail.com wrote:
>> From: "Demi M. Obenour" <demiobenour@gmail.com>
>>
>> While testing the O_PATHSTATIC patch, I discovered that Linux does not
>> return any error if an invalid flag is passed to open(2).  This prevents
>> adding new flags without a (minor) risk of breaking userspace.
>> Therefore, add a check for invalid flags, and return -EINVAL if any are
>> found.
> 
> ... which would qualtify as userland ABI breakage all by itself.
> 
I suspect that very few (if any) programs pass invalid flags to open().
Additionally, O_DIRECT was added in Linux 2.4, even though it had
previously been ignored.  If someone knows of a real program that does,
I can instead create an open2() syscall, but I would prefer to avoid that.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]