* [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW @ 2019-02-12 14:54 demiobenour 2019-02-12 14:54 ` [PATCH 2/4] Expose O_PATHSTATIC to userspace demiobenour ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw) To: Alexander Viro, David Howells, Demi M. Obenour, open list:FILESYSTEMS (VFS and infrastructure), open list 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/4] Expose O_PATHSTATIC to userspace 2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour @ 2019-02-12 14:54 ` demiobenour 2019-02-12 20:18 ` Andreas Dilger 2019-02-12 14:54 ` [PATCH 3/4] Add AT_PATHSTATIC to linkat() demiobenour 2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour 2 siblings, 1 reply; 8+ messages in thread From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw) To: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann, open list:FILE LOCKING (flock() and fcntl()/lockf()), open list, open list:GENERIC INCLUDE/ASM HEADER FILES Cc: Demi M. Obenour 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] Expose O_PATHSTATIC to userspace 2019-02-12 14:54 ` [PATCH 2/4] Expose O_PATHSTATIC to userspace demiobenour @ 2019-02-12 20:18 ` Andreas Dilger 2019-02-12 22:41 ` Demi M. Obenour 0 siblings, 1 reply; 8+ messages in thread From: Andreas Dilger @ 2019-02-12 20:18 UTC (permalink / raw) To: demiobenour Cc: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann, linux-fsdevel, open list, linux-arch [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] Expose O_PATHSTATIC to userspace 2019-02-12 20:18 ` Andreas Dilger @ 2019-02-12 22:41 ` Demi M. Obenour 0 siblings, 0 replies; 8+ messages in thread From: Demi M. Obenour @ 2019-02-12 22:41 UTC (permalink / raw) To: Andreas Dilger Cc: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann, linux-fsdevel, open list, linux-arch [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/4] Add AT_PATHSTATIC to linkat() 2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour 2019-02-12 14:54 ` [PATCH 2/4] Expose O_PATHSTATIC to userspace demiobenour @ 2019-02-12 14:54 ` demiobenour 2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour 2 siblings, 0 replies; 8+ messages in thread From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw) To: Alexander Viro, Jeff Layton, J. Bruce Fields, open list:FILESYSTEMS (VFS and infrastructure), open list Cc: Demi M. Obenour 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() 2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour 2019-02-12 14:54 ` [PATCH 2/4] Expose O_PATHSTATIC to userspace demiobenour 2019-02-12 14:54 ` [PATCH 3/4] Add AT_PATHSTATIC to linkat() demiobenour @ 2019-02-12 14:54 ` demiobenour 2019-02-12 20:38 ` Al Viro 2 siblings, 1 reply; 8+ messages in thread From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw) To: Alexander Viro, open list:FILESYSTEMS (VFS and infrastructure), open list Cc: Demi M. Obenour 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() 2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour @ 2019-02-12 20:38 ` Al Viro 2019-02-12 23:00 ` Demi M. Obenour 0 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2019-02-12 20:38 UTC (permalink / raw) To: demiobenour; +Cc: open list:FILESYSTEMS (VFS and infrastructure), open list 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() 2019-02-12 20:38 ` Al Viro @ 2019-02-12 23:00 ` Demi M. Obenour 0 siblings, 0 replies; 8+ messages in thread From: Demi M. Obenour @ 2019-02-12 23:00 UTC (permalink / raw) To: Al Viro; +Cc: open list:FILESYSTEMS (VFS and infrastructure), open list [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-12 23:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour 2019-02-12 14:54 ` [PATCH 2/4] Expose O_PATHSTATIC to userspace demiobenour 2019-02-12 20:18 ` Andreas Dilger 2019-02-12 22:41 ` Demi M. Obenour 2019-02-12 14:54 ` [PATCH 3/4] Add AT_PATHSTATIC to linkat() demiobenour 2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour 2019-02-12 20:38 ` Al Viro 2019-02-12 23:00 ` Demi M. Obenour
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).