| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2025-08-27-needy-drab-puritan-rebates-tHEaFw@cyphar.com>
Date: Wed, 27 Aug 2025 20:29:38 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Al Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Kees Cook <keescook@...omium.org>,
Paul Moore <paul@...l-moore.com>, Serge Hallyn <serge@...lyn.com>,
Andy Lutomirski <luto@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Christian Heimes <christian@...hon.org>, Dmitry Vyukov <dvyukov@...gle.com>,
Elliott Hughes <enh@...gle.com>, Fan Wu <wufan@...ux.microsoft.com>,
Florian Weimer <fweimer@...hat.com>, Jann Horn <jannh@...gle.com>, Jeff Xu <jeffxu@...gle.com>,
Jonathan Corbet <corbet@....net>, Jordan R Abrahams <ajordanr@...gle.com>,
Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>, Luca Boccassi <bluca@...ian.org>,
Matt Bobrowski <mattbobrowski@...gle.com>, Miklos Szeredi <mszeredi@...hat.com>,
Mimi Zohar <zohar@...ux.ibm.com>, Nicolas Bouchinet <nicolas.bouchinet@....cyber.gouv.fr>,
Robert Waite <rowait@...rosoft.com>, Roberto Sassu <roberto.sassu@...wei.com>,
Scott Shell <scottsh@...rosoft.com>, Steve Dower <steve.dower@...hon.org>,
Steve Grubb <sgrubb@...hat.com>, kernel-hardening@...ts.openwall.com, linux-api@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, Andy Lutomirski <luto@...capital.net>,
Jeff Xu <jeffxu@...omium.org>
Subject: Re: [RFC PATCH v1 1/2] fs: Add O_DENY_WRITE
On 2025-08-22, Mickaël Salaün <mic@...ikod.net> wrote:
> Add a new O_DENY_WRITE flag usable at open time and on opened file (e.g.
> passed file descriptors). This changes the state of the opened file by
> making it read-only until it is closed. The main use case is for script
> interpreters to get the guarantee that script' content cannot be altered
> while being read and interpreted. This is useful for generic distros
> that may not have a write-xor-execute policy. See commit a5874fde3c08
> ("exec: Add a new AT_EXECVE_CHECK flag to execveat(2)")
>
> Both execve(2) and the IOCTL to enable fsverity can already set this
> property on files with deny_write_access(). This new O_DENY_WRITE make
> it widely available. This is similar to what other OSs may provide
> e.g., opening a file with only FILE_SHARE_READ on Windows.
>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Andy Lutomirski <luto@...capital.net>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: Jeff Xu <jeffxu@...omium.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Paul Moore <paul@...l-moore.com>
> Cc: Serge Hallyn <serge@...lyn.com>
> Reported-by: Robert Waite <rowait@...rosoft.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20250822170800.2116980-2-mic@digikod.net
> ---
> fs/fcntl.c | 26 ++++++++++++++++++++++++--
> fs/file_table.c | 2 ++
> fs/namei.c | 6 ++++++
> include/linux/fcntl.h | 2 +-
> include/uapi/asm-generic/fcntl.h | 4 ++++
> 5 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 5598e4d57422..0c80c0fbc706 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -34,7 +34,8 @@
>
> #include "internal.h"
>
> -#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
> +#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME | \
> + O_DENY_WRITE)
>
> static int setfl(int fd, struct file * filp, unsigned int arg)
> {
> @@ -80,8 +81,29 @@ static int setfl(int fd, struct file * filp, unsigned int arg)
> error = 0;
> }
> spin_lock(&filp->f_lock);
> +
> + if (arg & O_DENY_WRITE) {
> + /* Only regular files. */
> + if (!S_ISREG(inode->i_mode)) {
> + error = -EINVAL;
> + goto unlock;
> + }
> +
> + /* Only sets once. */
> + if (!(filp->f_flags & O_DENY_WRITE)) {
> + error = exe_file_deny_write_access(filp);
> + if (error)
> + goto unlock;
> + }
> + } else {
> + if (filp->f_flags & O_DENY_WRITE)
> + exe_file_allow_write_access(filp);
> + }
I appreciate the goal of making this something that can be cleared
(presumably for interpreters that mmap(MAP_PRIVATE) their scripts), but
making a security-related flag this easy to clear seems like a footgun
(any library function could mask O_DENY_WRITE or forget to copy the old
flag values).
> +
> filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
> filp->f_iocb_flags = iocb_flags(filp);
> +
> +unlock:
> spin_unlock(&filp->f_lock);
>
> out:
> @@ -1158,7 +1180,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(20 - 1 /* for O_RDONLY being 0 */ !=
> + BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
> HWEIGHT32(
> (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
> __FMODE_EXEC));
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 81c72576e548..6ba896b6a53f 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -460,6 +460,8 @@ static void __fput(struct file *file)
> locks_remove_file(file);
>
> security_file_release(file);
> + if (unlikely(file->f_flags & O_DENY_WRITE))
> + exe_file_allow_write_access(file);
> if (unlikely(file->f_flags & FASYNC)) {
> if (file->f_op->fasync)
> file->f_op->fasync(-1, file, 0);
> diff --git a/fs/namei.c b/fs/namei.c
> index cd43ff89fbaa..366530bf937d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3885,6 +3885,12 @@ static int do_open(struct nameidata *nd,
> error = may_open(idmap, &nd->path, acc_mode, open_flag);
> if (!error && !(file->f_mode & FMODE_OPENED))
> error = vfs_open(&nd->path, file);
> + if (!error && (open_flag & O_DENY_WRITE)) {
> + if (S_ISREG(file_inode(file)->i_mode))
> + error = exe_file_deny_write_access(file);
> + else
> + error = -EINVAL;
> + }
> if (!error)
> error = security_file_post_open(file, op->acc_mode);
> if (!error && do_truncate)
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index a332e79b3207..dad14101686f 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_DENY_WRITE)
I don't like this patch for the same reasons Christian has already said,
but in addition -- you cannot just add new open(2) flags like this.
Unlike openat2(2), classic open(2) does not verify invalid flag bits, so
any new flag must be designed so that old kernels will return an error
for that flag combination, which ensures that:
* No old programs set those bits inadvertently, which lets us avoid
breaking userspace in some really fun and hard-to-debug ways.
* For security-related bits, that new programs running on old kernels
do not think they are getting a security property that they aren't
actually getting.
O_TMPFILE's bitflag soup is an example of how you can resolve this issue
for open(2), but I would suggest that authors of new O_* flags seriously
consider making their flags openat2(2)-only unless it's trivial to get
the above behaviour.
> /* 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 613475285643..facd9136f5af 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -91,6 +91,10 @@
> /* a horrid kludge trying to make sure that this will fail on old kernels */
> #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
>
> +#ifndef O_DENY_WRITE
> +#define O_DENY_WRITE 040000000
> +#endif
> +
> #ifndef O_NDELAY
> #define O_NDELAY O_NONBLOCK
> #endif
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
Download attachment "signature.asc" of type "application/pgp-signature" (266 bytes)
Powered by blists - more mailing lists