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

[Jann Horn]

On Wed, Jul 31, 2024 at 11:33 PM Jann Horn <jannh at google.com> 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:
> > > 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...

I hacked up an RFC patch for this approach:
https://lore.kernel.org/r/20240802-remove-cred-transfer-v1-1-b3fef1ef2ade@google.com