Re: [PATCH 2/4] security: filesystem capabilities bugfix2

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > If I understand this right, then LSM_UNSAFE_PTRACE_CAP will only be set
> > if the tracer didn't have CAP_SYS_PTRACE.  So this seems sane to me.
> 
> Erm...  Firstly:

Yeah, inverse of what I said...

> 	int ptrace_attach(struct task_struct *task)
> 	{
> 	...
> 		if (capable(CAP_SYS_PTRACE))
> 			task->ptrace |= PT_PTRACE_CAP;
> 	...
> 	}
> 
> Then:
> 
> 	static int unsafe_exec(struct task_struct *p)
> 	{
> 		int unsafe = 0;
> 		if (p->ptrace & PT_PTRACED) {
> 			if (p->ptrace & PT_PTRACE_CAP)
> 				unsafe |= LSM_UNSAFE_PTRACE_CAP;
> 			else
> 				unsafe |= LSM_UNSAFE_PTRACE;
> 		}
> 		if (atomic_read(&p->fs->count) > 1 ||
> 		    atomic_read(&p->files->count) > 1 ||
> 		    atomic_read(&p->sighand->count) > 1)
> 			unsafe |= LSM_UNSAFE_SHARE;
> 
> 		return unsafe;
> 	}
> 
> So LSM_UNSAFE_PTRACE_CAP will only be set if the tracer _does_ have
> CAP_SYS_PTRACE.  That will be irrelevant, however, if any of fs, files or
> sighand are shared.
> 
> And finally:
> 
> 	void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
> 	{
> 	...
> 		if (bprm->e_uid != current->uid ||
> 		    bprm->e_gid != current->gid ||
> 		    !cap_issubset (new_permitted, current->cap_permitted)) {
> 	...
> 			if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
> 				if (!capable(CAP_SETUID)) {
> 					bprm->e_uid = current->uid;
> 					bprm->e_gid = current->gid;
> 				}
> 				if (!capable (CAP_SETPCAP)) {
> 					new_permitted = cap_intersect(
> 						new_permitted,
> 						current->cap_permitted);
> 				}
> 			}
> 	...
> 	}
> 
> So if it's a 'set-privilege' binary, then if the tracer _doesn't_ have
> CAP_SYS_PTRACE, we look at downgrading the privileges of the process.
> 
> Without Andrew's patch, we only downgrade the capabilities if we don't have
> CAP_SETPCAP (and aren't sharing inheritables).
> 
> With Andrew's patch, capabilities are downgraded regardless of whether we have
> CAP_SETPCAP or not.  I guess that means that if you're tracing a binary whose
> filecaps say that it wants CAP_SETPCAP, then it retains CAP_SETPCAP.

I don't understand where that last sentence comes from.  Why would it
retain CAP_SETPCAP?

> I wonder if the tracing task should be examined here, and any capability the
> tracer isn't permitted should be denied the process doing the exec.

That sounds reasonable on its own, but it opens up a dangerous ability
for the partially-privileged tracer to manipulate the capability set for
the traced task.

Note that (as of recently) we do not allow the execution of a file with
partial privileges in its pE', precisely because it is dangerous to
allow pick-and-choosing of capabilities in a capability-unaware binary.

So frankly I wonder whether the existing downgrade is really safe...

-serge

> Anyway, in my commoncap.c prettification patch, I've dressed the limiter
> function up as follows:
> 
> 	/*
> 	 * Determine whether a exec'ing process's new permitted capabilities
> 	 * should be limited to just what it already has.
> 	 *
> 	 * This prevents processes that are being ptraced from gaining access
> 	 * to CAP_SETPCAP, unless the process they're tracing already has it,
> 	 * and the binary they're executing has filecaps that elevate it.
> 	 *
> 	 *  Returns 1 if they should be limited, 0 if they are not.
> 	 */
> 	static inline int cap_limit_ptraced_target(void)
> 	{
> 	#ifndef CONFIG_SECURITY_FILE_CAPABILITIES
> 		if (capable(CAP_SETPCAP))
> 			return 0;
> 	#endif
> 		return 1;
> 	}
> 
> David