linux-kernel - Re: [PATCH v1] keys: Restrict KEYCTL_SESSION_TO_PARENT according to ptrace_may

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG48ez2TVGzqS4RPSLJpLEsuqEPsxKfy+CoamGBD-1L8sWSAQQ@mail.gmail.com>
Date: Fri, 2 Aug 2024 15:12:43 +0200
From: Jann Horn <jannh@...gle.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Mickaël Salaün <mic@...ikod.net>, 
	David Howells <dhowells@...hat.com>, Jarkko Sakkinen <jarkko@...nel.org>, 
	Günther Noack <gnoack@...gle.com>, 
	James Morris <jmorris@...ei.org>, Kees Cook <kees@...nel.org>, keyrings@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v1] keys: Restrict KEYCTL_SESSION_TO_PARENT according to ptrace_may_access()

On Wed, Jul 31, 2024 at 11:33 PM Jann Horn <jannh@...gle.com> wrote:
> On Wed, Jul 31, 2024 at 11:27 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <jannh@...gle.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