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

[Mickaël Salaün]

On Wed, Jul 31, 2024 at 11:33:04PM +0200, Jann Horn wrote:
> On Wed, Jul 31, 2024 at 11:27 PM Paul Moore <paul at paul-moore.com> wrote:
> > On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <jannh at google.com> wrote:
> > > On Wed, Jul 31, 2024 at 10:29 PM Paul Moore <paul at paul-moore.com> wrote:
> > > > On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <mic at digikod.net> 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 at digikod.net> 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 at digikod.net> 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 at digikod.net> 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?
> 
> Yeah...
> 
> > > 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?
> 
> Yeah, I think that'd probably be a reasonable way to do it. Post task
> work to the parent, wait for the task work to finish (with an
> interruptible sleep that cancels the work item on EINTR), and then do
> the checks and stuff in the parent. I guess whether we should also do
> racy checks in the child before that depends on whether we're worried
> about a child without the necessary permissions being able to cause
> spurious syscall restarts in the parent...

Why doing the check only in the parent and reporting back the result to
the child could be a security issue?  I guess duplicating the check
would just avoid executing useless code in the parent side if the child
doesn't have enough privileges right?

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

I'll let you handle the new design of the hook, but I'll review it. :)

I guess we're not OK to tie the KEYCTL_SESSION_TO_PARENT call to a
ptrace_may_access() mainly because of the Yama case?  I'm wondering if
we should add an exception for Yama here, or if each LSM should
implement its own new hook with the related new bit of security policy.
I guess some systems with a fine-tuned SELinux policy could be an issue
too.

Anyway, I wondering what was the motivation to only/mainly check
EUID/EGID for keyring change.