| 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
| ||
|
Date: Wed, 12 Jun 2024 19:13:41 +0100
From: "Adrian Ratiu" <adrian.ratiu@...labora.com>
To: "Kees Cook" <kees@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, linux-doc@...r.kernel.org, kernel@...labora.com, gbiv@...gle.com, ryanbeltran@...gle.com, inglorion@...gle.com, ajordanr@...gle.com, jorgelo@...omium.org, "Guenter Roeck" <groeck@...omium.org>, "Doug Anderson" <dianders@...omium.org>, "Jann Horn" <jannh@...gle.com>, "Andrew Morton" <akpm@...ux-foundation.org>, "Randy Dunlap" <rdunlap@...radead.org>, "Christian Brauner" <brauner@...nel.org>, "Jeff Xu" <jeffxu@...gle.com>, "Mike Frysinger" <vapier@...omium.org>
Subject: Re: [PATCH v5 2/2] proc: restrict /proc/pid/mem
On Thursday, June 06, 2024 20:45 EEST, Kees Cook <kees@...nel.org> wrote:
> On Wed, Jun 05, 2024 at 07:49:31PM +0300, Adrian Ratiu wrote:
> > + proc_mem.restrict_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.
>
> It dawns on me that we likely need an "off" setting for these in case it
> was CONFIG-enabled...
>
> > +static int __init early_proc_mem_restrict_##name(char *buf) \
> > +{ \
> > + if (!buf) \
> > + return -EINVAL; \
> > + \
> > + if (strcmp(buf, "all") == 0) \
> > + static_key_slow_inc(&proc_mem_restrict_##name##_all.key); \
> > + else if (strcmp(buf, "ptracer") == 0) \
> > + static_key_slow_inc(&proc_mem_restrict_##name##_ptracer.key); \
> > + return 0; \
> > +} \
> > +early_param("proc_mem.restrict_" #name, early_proc_mem_restrict_##name)
>
> Why slow_inc here instead of the normal static_key_enable/disable?
>
> And we should report misparsing too, so perhaps:
>
> static int __init early_proc_mem_restrict_##name(char *buf) \
> { \
> if (!buf) \
> return -EINVAL; \
> \
> if (strcmp(buf, "all") == 0) { \
> static_key_enable(&proc_mem_restrict_##name##_all.key); \
> static_key_disable(&proc_mem_restrict_##name##_ptracer.key); \
> } else if (strcmp(buf, "ptracer") == 0) { \
> static_key_disable(&proc_mem_restrict_##name##_all.key); \
> static_key_enable(&proc_mem_restrict_##name##_ptracer.key); \
> } else if (strcmp(buf, "off") == 0) { \
> static_key_disable(&proc_mem_restrict_##name##_all.key); \
> static_key_disable(&proc_mem_restrict_##name##_ptracer.key); \
> } else \
> pr_warn("%s: ignoring unknown option '%s'\n", \
> "proc_mem.restrict_" #name, buf); \
> return 0; \
> } \
> early_param("proc_mem.restrict_" #name, early_proc_mem_restrict_##name)
>
> > +static int __mem_open_access_permitted(struct file *file, struct task_struct *task)
> > +{
> > + bool is_ptracer;
> > +
> > + rcu_read_lock();
> > + is_ptracer = current == ptrace_parent(task);
> > + rcu_read_unlock();
> > +
> > + if (file->f_mode & FMODE_WRITE) {
> > + /* Deny if writes are unconditionally disabled via param */
> > + if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_DEFAULT,
> > + &proc_mem_restrict_open_write_all))
> > + return -EACCES;
> > +
> > + /* Deny if writes are allowed only for ptracers via param */
> > + if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_PTRACE_DEFAULT,
> > + &proc_mem_restrict_open_write_ptracer) &&
> > + !is_ptracer)
> > + return -EACCES;
> > + }
> > +
> > + if (file->f_mode & FMODE_READ) {
> > + /* Deny if reads are unconditionally disabled via param */
> > + if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_DEFAULT,
> > + &proc_mem_restrict_open_read_all))
> > + return -EACCES;
> > +
> > + /* Deny if reads are allowed only for ptracers via param */
> > + if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_PTRACE_DEFAULT,
> > + &proc_mem_restrict_open_read_ptracer) &&
> > + !is_ptracer)
> > + return -EACCES;
> > + }
> > +
> > + return 0; /* R/W are not restricted */
> > +}
>
> Given how deeply some of these behaviors may be in userspace, it might
> be more friendly to report the new restrictions with a pr_notice() so
> problems can be more easily tracked down. For example:
>
> static void report_mem_rw_rejection(const char *action, struct task_struct *task)
> {
> pr_warn_ratelimited("Denied %s of /proc/%d/mem (%s) by pid %d (%s)\n",
> action, task_pid_nr(task), task->comm,
> task_pid_nr(current), current->comm);
> }
>
> ...
>
> if (file->f_mode & FMODE_WRITE) {
> /* Deny if writes are unconditionally disabled via param */
> if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_DEFAULT,
> &proc_mem_restrict_open_write_all)) {
> report_mem_rw_reject("all open-for-write");
> return -EACCES;
> }
>
> /* Deny if writes are allowed only for ptracers via param */
> if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_PTRACE_DEFAULT,
> &proc_mem_restrict_open_write_ptracer) &&
> !is_ptracer)
> report_mem_rw_reject("non-ptracer open-for-write");
> return -EACCES;
> }
>
> etc
>
> > +static bool __mem_rw_current_is_ptracer(struct file *file)
> > +{
> > + struct inode *inode = file_inode(file);
> > + struct task_struct *task = get_proc_task(inode);
> > + struct mm_struct *mm = NULL;
> > + int is_ptracer = false, has_mm_access = false;
> > +
> > + if (task) {
> > + rcu_read_lock();
> > + is_ptracer = current == ptrace_parent(task);
> > + rcu_read_unlock();
> > +
> > + mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> > + if (mm && file->private_data == mm) {
> > + has_mm_access = true;
> > + mmput(mm);
> > + }
> > +
> > + put_task_struct(task);
> > + }
> > +
> > + return is_ptracer && has_mm_access;
> > +}
>
> Thanks; this looks right to me now!
>
> > +menu "Procfs mem restriction options"
> > +
> > +config PROC_MEM_RESTRICT_FOLL_FORCE_DEFAULT
> > + bool "Restrict all FOLL_FORCE flag usage"
> > + default n
> > + help
> > + Restrict all FOLL_FORCE usage during /proc/*/mem RW.
> > + Debuggers like GDB require using FOLL_FORCE for basic
> > + functionality.
> > +
> > +config PROC_MEM_RESTRICT_FOLL_FORCE_PTRACE_DEFAULT
> > + bool "Restrict FOLL_FORCE usage except for ptracers"
> > + default n
> > + help
> > + Restrict FOLL_FORCE usage during /proc/*/mem RW, except
> > + for ptracer processes. Debuggers like GDB require using
> > + FOLL_FORCE for basic functionality.
>
> Can we adjust the Kconfigs to match the bootparam arguments? i.e.
> instead of two for each mode, how about one with 3 settings ("all",
> "ptrace", or "off")
>
> choice
> prompt "Restrict /proc/pid/mem FOLL_FORCE usage"
> default PROC_MEM_RESTRICT_FOLL_FORCE_OFF
> help
> Reading and writing of /proc/pid/mem bypasses memory permission
> checks due to the internal use of the FOLL_FORCE flag. This can be
> used by attackers to manipulate process memory contents that
> would have been otherwise protected. However, debuggers, like GDB,
> use this to set breakpoints, etc. To force debuggers to fall back
> to PEEK/POKE, see PROC_MEM_RESTRICT_OPEN_WRITE_ALL.
>
> config PROC_MEM_RESTRICT_FOLL_FORCE_OFF
> bool "Do not restrict FOLL_FORCE usage with /proc/pid/mem (regular)"
> help
> Regular behavior: continue to use the FOLL_FORCE flag for
> /proc/pid/mem access.
>
> config PROC_MEM_RESTRICT_FOLL_FORCE_PTRACE
> bool "Only allow ptracers to use FOLL_FORCE with /proc/pid/mem (safer)"
> help
> Only use the FOLL_FORCE flag for /proc/pid/mem access when the
> current task is the active ptracer of the target task. (Safer,
> least disruptive to most usage patterns.)
>
> config PROC_MEM_RESTRICT_FOLL_FORCE_ALL
> bool "Do not use FOLL_FORCE with /proc/pid/mem (safest)"
> help
> Remove the FOLL_FORCE flag for all /proc/pid/mem accesses.
> (Safest, but may be disruptive to some usage patterns.)
> endchoice
>
> Then the static_keys can be defined like this mess (I couldn't find a
> cleaner way to do it):
>
> #define DEFINE_STATIC_KEY_PROC_MEM_ALL(name) \
> DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_##name##_all); \
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_ptracer);
> #define DEFINE_STATIC_KEY_PROC_MEM_PTRACE(name) \
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_all); \
> DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_##name##_ptracer);
> #define DEFINE_STATIC_KEY_PROC_MEM_OFF(name) \
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_all); \
> DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_ptracer);
>
> #define DEFINE_STATIC_KEY_PROC_MEM_0(level, name)
> #define DEFINE_STATIC_KEY_PROC_MEM_1(level, name) \
> DEFINE_STATIC_KEY_PROC_MEM_##level(name)
>
> #define _DEFINE_STATIC_KEY_PROC_MEM_PICK(enabled, level, name) \
> DEFINE_STATIC_KEY_PROC_MEM_##enabled(level, name)
>
> #define DEFINE_STATIC_KEY_PROC_MEM_PICK(enabled, level, name) \
> _DEFINE_STATIC_KEY_PROC_MEM_PICK(enabled, level, name)
>
> #define DEFINE_STATIC_KEY_PROC_MEM(CFG, name) \
> DEFINE_STATIC_KEY_PROC_MEM_PICK(IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_##CFG##_ALL), ALL, name)
> DEFINE_STATIC_KEY_PROC_MEM_PICK(IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_##CFG##_PTRACE), PTRACE, name)
> DEFINE_STATIC_KEY_PROC_MEM_PICK(IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_##CFG##_OFF), OFF, name)
>
> #define DEFINE_EARLY_PROC_MEM_RESTRICT(CFG, name) \
> DEFINE_STATIC_KEY_PROC_MEM(CFG, name) \
> static int __init early_proc_mem_restrict_##name(char *buf) \
> { \
> if (!buf) \
> return -EINVAL; \
> \
> if (strcmp(buf, "all") == 0) { \
> static_key_enable(&proc_mem_restrict_##name##_all.key); \
> static_key_disable(&proc_mem_restrict_##name##_ptracer.key); \
> } else if (strcmp(buf, "ptracer") == 0) { \
> static_key_disable(&proc_mem_restrict_##name##_all.key); \
> static_key_enable(&proc_mem_restrict_##name##_ptracer.key); \
> } else if (strcmp(buf, "off") == 0) { \
> static_key_disable(&proc_mem_restrict_##name##_all.key); \
> static_key_disable(&proc_mem_restrict_##name##_ptracer.key); \
> } else \
> pr_warn("%s: ignoring unknown option '%s'\n", \
> "proc_mem.restrict_" #name, buf); \
> return 0; \
> } \
> early_param("proc_mem.restrict_" #name, early_proc_mem_restrict_##name)
>
> DEFINE_EARLY_PROC_MEM_RESTRICT(OPEN_READ, open_read);
> DEFINE_EARLY_PROC_MEM_RESTRICT(OPEN_WRITE, open_write);
> DEFINE_EARLY_PROC_MEM_RESTRICT(WRITE, write);
> DEFINE_EARLY_PROC_MEM_RESTRICT(FOLL_FORCE, foll_force);
Hello again,
I tried very hard to make the above work these past few days and gave up.
Couldn't find a way to get it to compile.
Tried to also debug the compiler preprocess output and my head hurts. :)
Would macros like the following be acceptable?
I know it's more verbose but also much easier to understand and it works.
#if IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_ALL)
DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_open_read_all);
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_ptracer);
#elif IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_PTRACE)
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_all);
DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_open_read_ptracer);
#else
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_all);
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_ptracer);
#endif
Powered by blists - more mailing lists