Re: [PATCH v1] keys: Restrict KEYCTL_SESSION_TO_PARENT according to ptrace_may_access()

[Paul Moore]

On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
> On Wed, Jul 31, 2024 at 10:29 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > On Mon, Jul 29, 2024 at 05:06:10PM +0200, Jann Horn wrote:
> > > > On Mon, Jul 29, 2024 at 5:02 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > > On Mon, Jul 29, 2024 at 04:21:01PM +0200, Jann Horn wrote:
> > > > > > On Mon, Jul 29, 2024 at 4:09 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > > > > On Mon, Jul 29, 2024 at 03:49:29PM +0200, Jann Horn wrote:
> > > > > > > > On Mon, Jul 29, 2024 at 2:59 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > > > > > > A process can modify its parent's credentials with
> > > > > > > > > KEYCTL_SESSION_TO_PARENT when their EUID and EGID are the same.  This
> > > > > > > > > doesn't take into account all possible access controls.
> > > > > > > > >
> > > > > > > > > Enforce the same access checks as for impersonating a process.
> > > > > > > > >
> > > > > > > > > The current credentials checks are untouch because they check against
> > > > > > > > > EUID and EGID, whereas ptrace_may_access() checks against UID and GID.
> > > > > > > >
> > > > > > > > FWIW, my understanding is that the intended usecase of
> > > > > > > > KEYCTL_SESSION_TO_PARENT is that command-line tools (like "keyctl
> > > > > > > > new_session" and "e4crypt new_session") want to be able to change the
> > > > > > > > keyring of the parent process that spawned them (which I think is
> > > > > > > > usually a shell?); and Yama LSM, which I think is fairly widely used
> > > > > > > > at this point, by default prevents a child process from using
> > > > > > > > PTRACE_MODE_ATTACH on its parent.
> > > > > > >
> > > > > > > About Yama, the patched keyctl_session_to_parent() function already
> > > > > > > check if the current's and the parent's credentials are the same before
> > > > > > > this new ptrace_may_access() check.
> > > > > >
> > > > > > prepare_exec_creds() in execve() always creates new credentials which
> > > > > > are stored in bprm->cred and then later committed in begin_new_exec().
> > > > > > Also, fork() always copies the credentials in copy_creds().
> > > > > > So the "mycred == pcred" condition in keyctl_session_to_parent()
> > > > > > basically never applies, I think.
> > > > > > Also: When that condition is true, the whole operation is a no-op,
> > > > > > since if the credentials are the same, then the session keyring that
> > > > > > the credentials point to must also be the same.
> > > > >
> > > > > Correct, it's not a content comparison.  We could compare the
> > > > > credential's data for this specific KEYCTL_SESSION_TO_PARENT call, I
> > > > > guess this should not be performance sensitive.
> > > >
> > > > Yeah, though I guess keyctl_session_to_parent() is already kind of
> > > > doing that for the UID information; and for LSMs that would mean
> > > > adding an extra LSM hook?
> > >
> > > I'm wondering why security_key_session_to_parent() was never used: see
> > > commit 3011a344cdcd ("security: remove dead hook key_session_to_parent")
> >
> > While I was looking at this in another off-list thread I think I came
> > around to the same conclusion: I think we want the
> > security_key_session_to_parent() hook back, and while I'm wearing my
> > SELinux hat, I think we want a SELinux implementation.
>
> FYI: Those checks, including the hook that formerly existed there, are
> (somewhat necessarily) racy wrt concurrent security context changes of
> the parent because they come before asynchronous work is posted to the
> parent to do the keyring update.

I was wondering about something similar while looking at
keyctl_session_to_parent(), aren't all of the parent's cred checks
here racy?

> In theory we could make them synchronous if we have the child wait for
> the parent to enter task work... actually, with that we could probably
> get rid of the whole cred_transfer hack and have the parent do
> prepare_creds() and commit_creds() normally, and propagate any errors
> back to the child, as long as we don't create any deadlocks with
> this...

Assuming that no problems are caused by waiting on the parent, this
might be the best approach.  Should we also move, or duplicate, the
cred checks into the parent's context to avoid any races?

> > Mickaël, is this something you want to work on?

-- 
paul-moore.com