[PATCH] Add new open(2) flag - O_EMPTY_PATH

[PATCH] Add new open(2) flag - O_EMPTY_PATH

[Ameer Hamza]

This patch adds a new flag O_EMPTY_PATH that allows openat and open
system calls to open a file referenced by fd if the path is empty,
and it is very similar to the FreeBSD O_EMPTY_PATH flag. This can be
beneficial in some cases since it would avoid having to grant /proc
access to things like samba containers for reopening files to change
flags in a race-free way.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
---
 fs/fcntl.c                             | 2 +-
 fs/namei.c                             | 4 ++--
 fs/open.c                              | 2 +-
 include/linux/fcntl.h                  | 2 +-
 include/uapi/asm-generic/fcntl.h       | 4 ++++
 tools/include/uapi/asm-generic/fcntl.h | 4 ++++
 6 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 146c9ab0cd4b..7aac650e16e2 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1027,7 +1027,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 309ae6fc8c99..2b2735af6d03 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -192,7 +192,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
 	if (unlikely(!len)) {
 		if (empty)
 			*empty = 1;
-		if (!(flags & LOOKUP_EMPTY)) {
+		if (!(flags & (LOOKUP_EMPTY | O_EMPTY_PATH))) {
 			putname(result);
 			return ERR_PTR(-ENOENT);
 		}
@@ -2347,7 +2347,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 	if ((flags & (LOOKUP_RCU | LOOKUP_CACHED)) == LOOKUP_CACHED)
 		return ERR_PTR(-EAGAIN);
 
-	if (!*s)
+	if (!*s && unlikely(!(flags & O_EMPTY_PATH)))
 		flags &= ~LOOKUP_RCU;
 	if (flags & LOOKUP_RCU)
 		rcu_read_lock();
diff --git a/fs/open.c b/fs/open.c
index 82c1a28b3308..b4ec054a418f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1301,7 +1301,7 @@ static long do_sys_openat2(int dfd, const char __user *filename,
 	if (fd)
 		return fd;
 
-	tmp = getname(filename);
+	tmp = getname_flags(filename, how->flags & O_EMPTY_PATH, NULL);
 	if (IS_ERR(tmp))
 		return PTR_ERR(tmp);
 
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index a332e79b3207..bf8467bb0bd2 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
 	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
 	 O_APPEND | O_NDELAY | O_NONBLOCK | __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_EMPTY_PATH)
 
 /* List of all valid flags for the how->resolve argument: */
 #define VALID_RESOLVE_FLAGS \
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 1ecdb911add8..a03f4275517b 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_EMPTY_PATH
+#define O_EMPTY_PATH	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)      
diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h
index b02c8e0f4057..f32a81604296 100644
--- a/tools/include/uapi/asm-generic/fcntl.h
+++ b/tools/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,10 @@
 #define __O_TMPFILE	020000000
 #endif
 
+#ifndef O_EMPTY_PATH
+#define O_EMPTY_PATH	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.25.1

Re: [PATCH] Add new open(2) flag - O_EMPTY_PATH

[Christian Brauner]

On Wed, Dec 28, 2022 at 09:02:49PM +0500, Ameer Hamza wrote:
> This patch adds a new flag O_EMPTY_PATH that allows openat and open
> system calls to open a file referenced by fd if the path is empty,
> and it is very similar to the FreeBSD O_EMPTY_PATH flag. This can be
> beneficial in some cases since it would avoid having to grant /proc
> access to things like samba containers for reopening files to change
> flags in a race-free way.
> 
> Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
> ---

In general this isn't a bad idea and Aleksa and I proposed this as part
of the openat2() patchset (see [1]).

However, the reason we didn't do this right away was that we concluded
that it shouldn't be simply adding a flag. Reopening file descriptors
through procfs is indeed very useful and is often required. But it's
also been an endless source of subtle bugs and security holes as it
allows reopening file descriptors with more permissions than the
original file descriptor had.

The same lax behavior should not be encoded into O_EMPTYPATH. Ideally we
would teach O_EMPTYPATH to adhere to magic link modes by default. This
would be tied to the idea of upgrade mask in openat2() (cf. [2]). They
allow a caller to specify the permissions that a file descriptor may be
reopened with at the time the fd is opened.

[1]: https://lore.kernel.org/lkml/20190930183316.10190-4-cyphar@cyphar.com/
[2]: https://lore.kernel.org/all/20220526130355.fo6gzbst455fxywy@senku/Kk

Re: [PATCH] Add new open(2) flag - O_EMPTY_PATH

[Ameer Hamza]

On Fri, Jan 06, 2023 at 02:06:51PM +0100, Christian Brauner wrote:
> On Wed, Dec 28, 2022 at 09:02:49PM +0500, Ameer Hamza wrote:
> > This patch adds a new flag O_EMPTY_PATH that allows openat and open
> > system calls to open a file referenced by fd if the path is empty,
> > and it is very similar to the FreeBSD O_EMPTY_PATH flag. This can be
> > beneficial in some cases since it would avoid having to grant /proc
> > access to things like samba containers for reopening files to change
> > flags in a race-free way.
> > 
> > Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
> > ---
> 
> In general this isn't a bad idea and Aleksa and I proposed this as part
> of the openat2() patchset (see [1]).
> 
> However, the reason we didn't do this right away was that we concluded
> that it shouldn't be simply adding a flag. Reopening file descriptors
> through procfs is indeed very useful and is often required. But it's
> also been an endless source of subtle bugs and security holes as it
> allows reopening file descriptors with more permissions than the
> original file descriptor had.
> 
> The same lax behavior should not be encoded into O_EMPTYPATH. Ideally we
> would teach O_EMPTYPATH to adhere to magic link modes by default. This
> would be tied to the idea of upgrade mask in openat2() (cf. [2]). They
> allow a caller to specify the permissions that a file descriptor may be
> reopened with at the time the fd is opened.
> 
> [1]: https://lore.kernel.org/lkml/20190930183316.10190-4-cyphar@cyphar.com/
> [2]: https://lore.kernel.org/all/20220526130355.fo6gzbst455fxywy@senku/Kk

Thank you for the detailed explanation and sorry for getting back late
at it. It seems like a pre-requisite for O_EMPTYPATH is to make it safe
and that depends on a patchset that Aleksa was working on. It would be
helpful to know the current status of that effort and if we could expect
it in the near future.

The repo[1] that was mentioned here[2] seems to be private. I am wondering
if there's a way to look at the patch somehow.

[1]: https://github.com/cyphar/linux/tree/magiclink/main
[2]: https://lore.kernel.org/all/20220526130952.z5efngrnh7xtli32@senku/

Re: [PATCH] Add new open(2) flag - O_EMPTY_PATH

[Christian Brauner]

On Wed, Apr 19, 2023 at 01:46:16AM +0000, Mezgani Ali wrote:
> Look Hamza,
> 
> The style you’re writing with project a reader of drafts and IETF documents.
> Interesting what you are doing here in Kernel files.
> 
> But let me tell you I suspect you are a hyper virtualized Moroccan ( Hypocrite ).
> 
> One solution you have HR and leave.

Nonsensical off-topic mails like this with thinly veiled personal
attacks are not acceptable. So stop it.

RE: [PATCH] Add new open(2) flag - O_EMPTY_PATH

[David Laight]

From: Ameer Hamza
> Sent: 19 April 2023 02:15
> 
> On Fri, Jan 06, 2023 at 02:06:51PM +0100, Christian Brauner wrote:
> > On Wed, Dec 28, 2022 at 09:02:49PM +0500, Ameer Hamza wrote:
> > > This patch adds a new flag O_EMPTY_PATH that allows openat and open
> > > system calls to open a file referenced by fd if the path is empty,
> > > and it is very similar to the FreeBSD O_EMPTY_PATH flag. This can be
> > > beneficial in some cases since it would avoid having to grant /proc
> > > access to things like samba containers for reopening files to change
> > > flags in a race-free way.
> > >
> > > Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
> > > ---
> >
> > In general this isn't a bad idea and Aleksa and I proposed this as part
> > of the openat2() patchset (see [1]).
> >
> > However, the reason we didn't do this right away was that we concluded
> > that it shouldn't be simply adding a flag. Reopening file descriptors
> > through procfs is indeed very useful and is often required. But it's
> > also been an endless source of subtle bugs and security holes as it
> > allows reopening file descriptors with more permissions than the
> > original file descriptor had.
> >
> > The same lax behavior should not be encoded into O_EMPTYPATH. Ideally we
> > would teach O_EMPTYPATH to adhere to magic link modes by default. This
> > would be tied to the idea of upgrade mask in openat2() (cf. [2]). They
> > allow a caller to specify the permissions that a file descriptor may be
> > reopened with at the time the fd is opened.
> >
> > [1]: https://lore.kernel.org/lkml/20190930183316.10190-4-cyphar@cyphar.com/
> > [2]: https://lore.kernel.org/all/20220526130355.fo6gzbst455fxywy@senku/Kk
> 
> Thank you for the detailed explanation and sorry for getting back late
> at it. It seems like a pre-requisite for O_EMPTYPATH is to make it safe
> and that depends on a patchset that Aleksa was working on. It would be
> helpful to know the current status of that effort and if we could expect
> it in the near future.

ISTM that reopening a file READ_WRITE shouldn't be unconditionally allowed.
Checking the inode permissions of the file isn't enough to ensure
that the process is allowed to open it.
The 'x' (search) permissions on all the parent directories needs to
be checked (going back as far as some directory the process has open).

If a full pathname is generated this check is done.
But the proposed O_EMTPY_PATH won't be doing it.

This all matters if a system is using restricted directory
permissions to block a process from opening files in some
part of the filesystem, but is also being passed an open
fd (for reading) in that part of the filesystem.
I'm sure there are systems that will be doing this.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] Add new open(2) flag - O_EMPTY_PATH

[Andrew Walker]

On Wed, Apr 19, 2023 at 4:29 PM David Laight <David.Laight@aculab.com> wrote:
> ISTM that reopening a file READ_WRITE shouldn't be unconditionally allowed.
> Checking the inode permissions of the file isn't enough to ensure
> that the process is allowed to open it.
> The 'x' (search) permissions on all the parent directories needs to
> be checked (going back as far as some directory the process has open).
>
> If a full pathname is generated this check is done.
> But the proposed O_EMTPY_PATH won't be doing it.
>
> This all matters if a system is using restricted directory
> permissions to block a process from opening files in some
> part of the filesystem, but is also being passed an open
> fd (for reading) in that part of the filesystem.
> I'm sure there are systems that will be doing this.
>
>         David
>

So to be safe, hypothetically, the caller should be required to have
CAP_DAC_READ_SEARCH like with open_by_handle_at(2)?

Andrew