Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

[Jann Horn <jann@thejh.net>]

[-- Attachment #1: Type: text/plain, Size: 5604 bytes --]

On Mon, Nov 09, 2015 at 12:55:54PM -0800, Andrew Morton wrote:
> On Sun,  8 Nov 2015 13:08:36 +0100 Jann Horn <jann@thejh.net> wrote:
> 
> > By checking the effective credentials instead of the real UID /
> > permitted capabilities, ensure that the calling process actually
> > intended to use its credentials.
> > 
> > To ensure that all ptrace checks use the correct caller
> > credentials (e.g. in case out-of-tree code or newly added code
> > omits the PTRACE_MODE_*CREDS flag), use two new flags and
> > require one of them to be set.
> > 
> > The problem was that when a privileged task had temporarily dropped
> > its privileges, e.g. by calling setreuid(0, user_uid), with the
> > intent to perform following syscalls with the credentials of
> > a user, it still passed ptrace access checks that the user would
> > not be able to pass.
> > 
> > While an attacker should not be able to convince the privileged
> > task to perform a ptrace() syscall, this is a problem because the
> > ptrace access check is reused for things in procfs.
> > 
> > In particular, the following somewhat interesting procfs entries
> > only rely on ptrace access checks:
> > 
> >  /proc/$pid/stat - uses the check for determining whether pointers
> >      should be visible, useful for bypassing ASLR
> >  /proc/$pid/maps - also useful for bypassing ASLR
> >  /proc/$pid/cwd - useful for gaining access to restricted
> >      directories that contain files with lax permissions, e.g. in
> >      this scenario:
> >      lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
> >      drwx------ root root /root
> >      drwxr-xr-x root root /root/foobar
> >      -rw-r--r-- root root /root/foobar/secret
> > 
> > Therefore, on a system where a root-owned mode 6755 binary
> > changes its effective credentials as described and then dumps a
> > user-specified file, this could be used by an attacker to reveal
> > the memory layout of root's processes or reveal the contents of
> > files he is not allowed to access (through /proc/$pid/cwd).
> 
> I'll await reviewer input on this one.  Meanwhile, a bunch of
> minor(ish) things...
> 
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -395,7 +395,8 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> >  
> >  	state = *get_task_state(task);
> >  	vsize = eip = esp = 0;
> > -	permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> > +	permitted = ptrace_may_access(task,
> > +		PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT | PTRACE_MODE_FSCREDS);
> 
> There's lots of ugliness in the patch to do with fitting code into 80 cols.

I agree.


> Can we do
> 
> #define PTRACE_foo (PTRACE_MODE_READ|PTRACE_MODE_FSCREDS)
> 
> to avoid all that?

Hm. All combinations of the PTRACE_MODE_*CREDS flags with
PTRACE_MODE_{READ,ATTACH} plus optionally PTRACE_MODE_NOAUDIT
make sense, I think. So your suggestion would be to create
four new #defines
PTRACE_MODE_{READ,ATTACH}_{FSCREDS,REALCREDS} and then let
callers OR in the PTRACE_MODE_NOAUDIT flag if needed?


> > --- a/include/linux/ptrace.h
> > +++ b/include/linux/ptrace.h
> > @@ -57,7 +57,22 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
> >  #define PTRACE_MODE_READ	0x01
> >  #define PTRACE_MODE_ATTACH	0x02
> >  #define PTRACE_MODE_NOAUDIT	0x04
> > -/* Returns true on success, false on denial. */
> > +#define PTRACE_MODE_FSCREDS 0x08
> > +#define PTRACE_MODE_REALCREDS 0x10
> > +/**
> > + * ptrace_may_access - check whether the caller is permitted to access
> > + * a target task.
> > + * @task: target task
> > + * @mode: selects type of access and caller credentials
> > + *
> > + * Returns true on success, false on denial.
> > + *
> > + * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
> > + * be set in @mode to specify whether the access was requested through
> > + * a filesystem syscall (should use effective capabilities and fsuid
> > + * of the caller) or through an explicit syscall such as
> > + * process_vm_writev or ptrace (and should use the real credentials).
> > + */
> >  extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
> 
> It is unconventional to put the kernedoc in the header - people have
> been trained to look for it in the .c file.

OK, will fix that. I thought it would be appropriate to put it in the
header since that one-line comment was already there.


> > +++ b/kernel/ptrace.c
> > @@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> >  static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> >  {
> >  	const struct cred *cred = current_cred(), *tcred;
> > +	kuid_t caller_uid;
> > +	kgid_t caller_gid;
> > +
> > +	if (!(mode & PTRACE_MODE_FSCREDS) != !(mode & PTRACE_MODE_REALCREDS)) {
> 
> So setting either one of these and not the other is an error.  How
> come?

Oh. Sorry about that. I only added PTRACE_MODE_REALCREDS in this iteration
of the patch and forgot to re-test afterwards. It is supposed to be the
other way around, so that you need to set exactly one. s/!=/==/


> > +		WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
> 
> This warning cannot be triggered by malicious userspace, I trust?

Yeah, the ptrace access check flags should come from kernelspace only.
My patch modifies all callers of mm_access / ptrace_may_access so that
exactly one of the new flags is added, and the mode argument is always
a constant.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]