From: "Demi M. Obenour" <[email protected]>
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
From: "Demi M. Obenour" <[email protected]>
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
From: "Demi M. Obenour" <[email protected]>
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
From: "Demi M. Obenour" <[email protected]>
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
On Feb 12, 2019, at 7:54 AM, [email protected] wrote:
>
> From: "Demi M. Obenour" <[email protected]>
>
> 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
On Tue, Feb 12, 2019 at 09:54:47AM -0500, [email protected] wrote:
> From: "Demi M. Obenour" <[email protected]>
>
> 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.
On 2/12/19 3:18 PM, Andreas Dilger wrote:
> On Feb 12, 2019, at 7:54 AM, [email protected] wrote:
>>
>> From: "Demi M. Obenour" <[email protected]>
>>
>> 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.
On 2/12/19 3:38 PM, Al Viro wrote:
> On Tue, Feb 12, 2019 at 09:54:47AM -0500, [email protected] wrote:
>> From: "Demi M. Obenour" <[email protected]>
>>
>> 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.