From: Christian Brauner <brauner@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: Adrian Ratiu <adrian.ratiu@collabora.com>,
linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
linux-doc@vger.kernel.org, kernel@collabora.com,
gbiv@google.com, ryanbeltran@google.com, inglorion@google.com,
ajordanr@google.com, jorgelo@chromium.org,
Guenter Roeck <groeck@chromium.org>,
Doug Anderson <dianders@chromium.org>,
Jann Horn <jannh@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Randy Dunlap <rdunlap@infradead.org>,
Mike Frysinger <vapier@chromium.org>
Subject: Re: [PATCH v3 1/2] proc: restrict /proc/pid/mem access via param knobs
Date: Fri, 3 May 2024 11:57:56 +0200 [thread overview]
Message-ID: <20240503-nulltarif-karten-82213463dedc@brauner> (raw)
In-Reply-To: <202404261544.1EAD63D@keescook>
On Fri, Apr 26, 2024 at 04:10:49PM -0700, Kees Cook wrote:
> On Tue, Apr 09, 2024 at 08:57:49PM +0300, Adrian Ratiu wrote:
> > Prior to v2.6.39 write access to /proc/<pid>/mem was restricted,
> > after which it got allowed in commit 198214a7ee50 ("proc: enable
> > writing to /proc/pid/mem"). Famous last words from that patch:
> > "no longer a security hazard". :)
> >
> > Afterwards exploits started causing drama like [1]. The exploits
> > using /proc/*/mem can be rather sophisticated like [2] which
> > installed an arbitrary payload from noexec storage into a running
> > process then exec'd it, which itself could include an ELF loader
> > to run arbitrary code off noexec storage.
> >
> > One of the well-known problems with /proc/*/mem writes is they
> > ignore page permissions via FOLL_FORCE, as opposed to writes via
> > process_vm_writev which respect page permissions. These writes can
> > also be used to bypass mode bits.
> >
> > To harden against these types of attacks, distrbutions might want
> > to restrict /proc/pid/mem accesses, either entirely or partially,
> > for eg. to restrict FOLL_FORCE usage.
> >
> > Known valid use-cases which still need these accesses are:
> >
> > * Debuggers which also have ptrace permissions, so they can access
> > memory anyway via PTRACE_POKEDATA & co. Some debuggers like GDB
> > are designed to write /proc/pid/mem for basic functionality.
> >
> > * Container supervisors using the seccomp notifier to intercept
> > syscalls and rewrite memory of calling processes by passing
> > around /proc/pid/mem file descriptors.
> >
> > There might be more, that's why these params default to disabled.
> >
> > Regarding other mechanisms which can block these accesses:
> >
> > * seccomp filters can be used to block mmap/mprotect calls with W|X
> > perms, but they often can't block open calls as daemons want to
> > read/write their runtime state and seccomp filters cannot check
> > file paths, so plain write calls can't be easily blocked.
> >
> > * Since the mem file is part of the dynamic /proc/<pid>/ space, we
> > can't run chmod once at boot to restrict it (and trying to react
> > to every process and run chmod doesn't scale, and the kernel no
> > longer allows chmod on any of these paths).
> >
> > * SELinux could be used with a rule to cover all /proc/*/mem files,
> > but even then having multiple ways to deny an attack is useful in
> > case one layer fails.
> >
> > Thus we introduce three kernel parameters to restrict /proc/*/mem
> > access: read, write and foll_force. All three can be independently
> > set to the following values:
> >
> > all => restrict all access unconditionally.
> > ptracer => restrict all access except for ptracer processes.
> >
> > If left unset, the existing behaviour is preserved, i.e. access
> > is governed by basic file permissions.
> >
> > Examples which can be passed by bootloaders:
> >
> > restrict_proc_mem_foll_force=all
> > restrict_proc_mem_write=ptracer
> > restrict_proc_mem_read=ptracer
> >
> > Each distribution needs to decide what restrictions to apply,
> > depending on its use-cases. Embedded systems might want to do
> > more, while general-purpouse distros might want a more relaxed
> > policy, because for e.g. foll_force=all and write=all both break
> > break GDB, so it might be a bit excessive.
> >
> > Based on an initial patch by Mike Frysinger <vapier@chromium.org>.
>
> Thanks for this new version!
>
> >
> > Link: https://lwn.net/Articles/476947/ [1]
> > Link: https://issues.chromium.org/issues/40089045 [2]
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Doug Anderson <dianders@chromium.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Co-developed-by: Mike Frysinger <vapier@chromium.org>
> > Signed-off-by: Mike Frysinger <vapier@chromium.org>
> > Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> > ---
> > .../admin-guide/kernel-parameters.txt | 27 +++++
> > fs/proc/base.c | 103 +++++++++++++++++-
> > include/linux/jump_label.h | 5 +
> > 3 files changed, 133 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 6e62b8cb19c8d..d7f7db41369c7 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5665,6 +5665,33 @@
> > reset_devices [KNL] Force drivers to reset the underlying device
> > during initialization.
> >
> > + restrict_proc_mem_read= [KNL]
> > + Format: {all | ptracer}
> > + Allows restricting read access to /proc/*/mem files.
> > + Depending on restriction level, open for reads return -EACCESS.
> > + Can be one of:
> > + - 'all' restricts all access unconditionally.
> > + - 'ptracer' allows access only for ptracer processes.
> > + If not specified, then basic file permissions continue to apply.
> > +
> > + restrict_proc_mem_write= [KNL]
> > + Format: {all | ptracer}
> > + Allows restricting write access to /proc/*/mem files.
> > + Depending on restriction level, open for writes return -EACCESS.
> > + Can be one of:
> > + - 'all' restricts all access unconditionally.
> > + - 'ptracer' allows access only for ptracer processes.
> > + If not specified, then basic file permissions continue to apply.
> > +
> > + restrict_proc_mem_foll_force= [KNL]
> > + Format: {all | ptracer}
> > + Restricts the use of the FOLL_FORCE flag for /proc/*/mem access.
> > + If restricted, the FOLL_FORCE flag will not be added to vm accesses.
> > + Can be one of:
> > + - 'all' restricts all access unconditionally.
> > + - 'ptracer' allows access only for ptracer processes.
> > + If not specified, FOLL_FORCE is always used.
>
> bike shedding: I wonder if this should be a fake namespace (adding a dot
> just to break it up for reading more easily), and have words reordered
> to the kernel's more common subject-verb-object: proc_mem.restrict_read=...
>
> > +
> > resume= [SWSUSP]
> > Specify the partition device for software suspend
> > Format:
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 18550c071d71c..c733836c42a65 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -152,6 +152,41 @@ struct pid_entry {
> > NULL, &proc_pid_attr_operations, \
> > { .lsmid = LSMID })
> >
> > +/*
> > + * each restrict_proc_mem_* param controls the following static branches:
> > + * key[0] = restrict all writes
> > + * key[1] = restrict writes except for ptracers
> > + * key[2] = restrict all reads
> > + * key[3] = restrict reads except for ptracers
> > + * key[4] = restrict all FOLL_FORCE usage
> > + * key[5] = restrict FOLL_FORCE usage except for ptracers
> > + */
> > +DEFINE_STATIC_KEY_ARRAY_FALSE_RO(restrict_proc_mem, 6);
>
> So, I don't like having open-coded numbers. And I'm not sure there's a
> benefit to stuffing these all into an array? So:
>
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_read);
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_write);
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_foll_force);
>
> > +
> > +static int __init early_restrict_proc_mem(char *buf, int offset)
> > +{
> > + if (!buf)
> > + return -EINVAL;
> > +
> > + if (strncmp(buf, "all", 3) == 0)
>
> I'd use strcmp() to get exact matches. That way "allalksdjflas" doesn't
> match. :)
>
> > + static_branch_enable(&restrict_proc_mem[offset]);
> > + else if (strncmp(buf, "ptracer", 7) == 0)
> > + static_branch_enable(&restrict_proc_mem[offset + 1]);
> > +
> > + return 0;
> > +}
>
> Then don't bother with a common helper since you've got a macro, and
> it'll all get tossed after __init anyway.
>
> > +
> > +#define DEFINE_EARLY_RESTRICT_PROC_MEM(name, offset) \
> > +static int __init early_restrict_proc_mem_##name(char *buf) \
> > +{ \
> > + return early_restrict_proc_mem(buf, offset); \
> > +} \
> > +early_param("restrict_proc_mem_" #name, early_restrict_proc_mem_##name)
> > +
> > +DEFINE_EARLY_RESTRICT_PROC_MEM(write, 0);
> > +DEFINE_EARLY_RESTRICT_PROC_MEM(read, 2);
> > +DEFINE_EARLY_RESTRICT_PROC_MEM(foll_force, 4);
>
> #define DEFINE_EARLY_PROC_MEM_RESTRICT(name) \
> static int __init early_proc_mem_restrict_##name(char *buf) \
> { \
> if (!buf) \
> return -EINVAL; \
> \
> if (strcmp(buf, "all") == 0) \
> static_branch_enable(&proc_mem_restrict_##name); \
> else if (strcmp(buf, "ptracer") == 0) \
> static_branch_enable(&proc_mem_restrict_##name); \
> \
> return 0; \
> } \
> early_param("proc_mem_restrict_" #name, early_proc_mem_restrict_##name)
>
>
> > +
> > /*
> > * Count the number of hardlinks for the pid_entry table, excluding the .
> > * and .. links.
> > @@ -825,9 +860,58 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
> > return 0;
> > }
> >
> > +static bool __mem_open_current_is_ptracer(struct file *file)
> > +{
> > + struct inode *inode = file_inode(file);
> > + struct task_struct *task = get_proc_task(inode);
> > + int ret = false;
> > +
> > + if (task) {
> > + rcu_read_lock();
> > + if (current == ptrace_parent(task))
> > + ret = true;
> > + rcu_read_unlock();
> > + put_task_struct(task);
> > + }
>
> This creates a ToCToU race between this check (which releases the task)
> and the later memopen which make get a different task (and mm).
>
> To deal with this, I think you need to add a new mode flag for
> proc_mem_open(), and add the checking there.
>
> > +
> > + return ret;
> > +}
> > +
> > +static int __mem_open_check_access_restriction(struct file *file)
> > +{
> > + if (file->f_mode & FMODE_WRITE) {
> > + /* Deny if writes are unconditionally disabled via param */
> > + if (static_branch_unlikely(&restrict_proc_mem[0]))
> > + return -EACCES;
> > +
> > + /* Deny if writes are allowed only for ptracers via param */
> > + if (static_branch_unlikely(&restrict_proc_mem[1]) &&
> > + !__mem_open_current_is_ptracer(file))
> > + return -EACCES;
> > +
> > + } else if (file->f_mode & FMODE_READ) {
>
> I think this "else" means that O_RDWR opens will only check the write
> flag, so drop the "else".
>
> > + /* Deny if reads are unconditionally disabled via param */
> > + if (static_branch_unlikely(&restrict_proc_mem[2]))
> > + return -EACCES;
> > +
> > + /* Deny if reads are allowed only for ptracers via param */
> > + if (static_branch_unlikely(&restrict_proc_mem[3]) &&
> > + !__mem_open_current_is_ptracer(file))
> > + return -EACCES;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mem_open(struct inode *inode, struct file *file)
> > {
> > - int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH);
> > + int ret;
> > +
> > + ret = __mem_open_check_access_restriction(file);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __mem_open(inode, file, PTRACE_MODE_ATTACH);
> >
> > /* OK to pass negative loff_t, we can catch out-of-range */
> > file->f_mode |= FMODE_UNSIGNED_OFFSET;
> > @@ -835,6 +919,20 @@ static int mem_open(struct inode *inode, struct file *file)
> > return ret;
> > }
> >
> > +static unsigned int __mem_rw_get_foll_force_flag(struct file *file)
> > +{
> > + /* Deny if FOLL_FORCE is disabled via param */
> > + if (static_branch_unlikely(&restrict_proc_mem[4]))
> > + return 0;
> > +
> > + /* Deny if FOLL_FORCE is allowed only for ptracers via param */
> > + if (static_branch_unlikely(&restrict_proc_mem[5]) &&
> > + !__mem_open_current_is_ptracer(file))
>
> This is like the ToCToU: the task may have changed out from under us
> between the open the read/write.
But why would you care? As long as the task is the ptracer it doesn't
really matter afaict.
next prev parent reply other threads:[~2024-05-03 9:58 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-09 17:57 [PATCH v3 1/2] proc: restrict /proc/pid/mem access via param knobs Adrian Ratiu
2024-04-09 17:57 ` [PATCH v3 2/2] proc: add Kconfigs to restrict /proc/pid/mem access Adrian Ratiu
2024-04-26 23:16 ` Kees Cook
2024-04-26 23:10 ` [PATCH v3 1/2] proc: restrict /proc/pid/mem access via param knobs Kees Cook
2024-05-03 9:57 ` Christian Brauner [this message]
2024-05-13 23:50 ` Kees Cook
2024-05-15 11:15 ` Adrian Ratiu
2024-05-15 11:13 ` Adrian Ratiu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240503-nulltarif-karten-82213463dedc@brauner \
--to=brauner@kernel.org \
--cc=adrian.ratiu@collabora.com \
--cc=ajordanr@google.com \
--cc=akpm@linux-foundation.org \
--cc=dianders@chromium.org \
--cc=gbiv@google.com \
--cc=groeck@chromium.org \
--cc=inglorion@google.com \
--cc=jannh@google.com \
--cc=jorgelo@chromium.org \
--cc=keescook@chromium.org \
--cc=kernel@collabora.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=rdunlap@infradead.org \
--cc=ryanbeltran@google.com \
--cc=vapier@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).