Messages in this thread | | | Date | Mon, 30 Jun 2008 13:53:18 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: [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
| |