lkml.org 
[lkml]   [2008]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/4] security: filesystem capabilities bugfix2
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


\
 
 \ /
  Last update: 2008-06-30 20:55    [W:0.434 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site