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

Commit Message

Mickaël Salaün July 29, 2024, 12:58 p.m. UTC

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.

Cc: David Howells <[e-mail]>
Cc: Günther Noack <[e-mail]>
Cc: James Morris <[e-mail]>
Cc: Jann Horn <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: Kees Cook <[e-mail]>
Cc: Paul Moore <[e-mail]>
Fixes: ee18d64c1f63 ("KEYS: Add a keyctl to install a process's session keyring on its parent [try #6]")
Signed-off-by: Mickaël Salaün <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
---
 security/keys/keyctl.c | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b

Comments

Jann Horn July 29, 2024, 1:49 p.m. UTC | #1

On Mon, Jul 29, 2024 at 2:59 PM Mickaël Salaün <[e-mail]> 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.

I think KEYCTL_SESSION_TO_PARENT is not a great design, but I'm not
sure if we can improve it much without risking some breakage.

Jarkko Sakkinen July 29, 2024, 2:06 p.m. UTC | #2

On Mon Jul 29, 2024 at 3:58 PM EEST, Mickaël Salaün 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.

Add a smoke test transcript here, which demonstrates the above
for A/B testing sake so that there is no need to invent one by
the reviewer.

Otherwise, it is too involved to give tested-by tag to this patch.

>
> 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.
>
> Cc: David Howells <[e-mail]>
> Cc: Günther Noack <[e-mail]>
> Cc: James Morris <[e-mail]>
> Cc: Jann Horn <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Kees Cook <[e-mail]>
> Cc: Paul Moore <[e-mail]>
> Fixes: ee18d64c1f63 ("KEYS: Add a keyctl to install a process's session keyring on its parent [try #6]")
> Signed-off-by: Mickaël Salaün <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]
> ---
>  security/keys/keyctl.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index ab927a142f51..511bf79fa14c 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -21,6 +21,7 @@
>  #include <linux/security.h>
>  #include <linux/uio.h>
>  #include <linux/uaccess.h>
> +#include <linux/ptrace.h>
>  #include <keys/request_key_auth-type.h>
>  #include "internal.h"
>  
> @@ -1687,6 +1688,10 @@ long keyctl_session_to_parent(void)
>  	    !gid_eq(pcred->sgid, mycred->egid))
>  		goto unlock;
>  
> +	/* The child must be allowed to impersonate its parent process. */
> +	if (!ptrace_may_access(parent, PTRACE_MODE_ATTACH_REALCREDS))
> +		goto unlock;
> +
>  	/* the keyrings must have the same UID */
>  	if ((pcred->session_keyring &&
>  	     !uid_eq(pcred->session_keyring->uid, mycred->euid)) ||
>
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b


BR, Jarkko

Mickaël Salaün July 29, 2024, 2:09 p.m. UTC | #3

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 <[e-mail]> 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.  So this change should be OK with
Yama and most use cases.

> 
> I think KEYCTL_SESSION_TO_PARENT is not a great design, but I'm not
> sure if we can improve it much without risking some breakage.
> 

I think this is a security issue that a process can change another
process's credentials.  If the main use cases is for shell commands, it
should be OK.

The alternative would be to restore the key_session_to_parent LSM hook
[1], and update most LSMs to block this kind of credential tampering,
which will lead to the same result but with only partial users being
protected.

[1] commit 3011a344cdcd ("security: remove dead hook key_session_to_parent")

Jann Horn July 29, 2024, 2:21 p.m. UTC | #4

On Mon, Jul 29, 2024 at 4:09 PM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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.

Mickaël Salaün July 29, 2024, 3:02 p.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]> 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.

Jann Horn July 29, 2024, 3:06 p.m. UTC | #6

On Mon, Jul 29, 2024 at 5:02 PM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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?

Mickaël Salaün July 29, 2024, 3:17 p.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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")

Paul Moore July 31, 2024, 8:29 p.m. UTC | #8

On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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.

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

Jann Horn July 31, 2024, 8:53 p.m. UTC | #9

On Wed, Jul 31, 2024 at 10:29 PM Paul Moore <[e-mail]> wrote:
> On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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.

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...

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

Paul Moore July 31, 2024, 9:27 p.m. UTC | #10

On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <[e-mail]> wrote:
> On Wed, Jul 31, 2024 at 10:29 PM Paul Moore <[e-mail]> wrote:
> > On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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?

Jann Horn July 31, 2024, 9:33 p.m. UTC | #11

On Wed, Jul 31, 2024 at 11:27 PM Paul Moore <[e-mail]> wrote:
> On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <[e-mail]> wrote:
> > On Wed, Jul 31, 2024 at 10:29 PM Paul Moore <[e-mail]> wrote:
> > > On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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...

> > > Mickaël, is this something you want to work on?
>
> --
> paul-moore.com

Mickaël Salaün Aug. 1, 2024, 3:34 p.m. UTC | #12

On Wed, Jul 31, 2024 at 11:33:04PM +0200, Jann Horn wrote:
> On Wed, Jul 31, 2024 at 11:27 PM Paul Moore <[e-mail]> wrote:
> > On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <[e-mail]> wrote:
> > > On Wed, Jul 31, 2024 at 10:29 PM Paul Moore <[e-mail]> wrote:
> > > > On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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.

Jann Horn Aug. 2, 2024, 1:12 p.m. UTC | #13

On Wed, Jul 31, 2024 at 11:33 PM Jann Horn <[e-mail]> wrote:
> On Wed, Jul 31, 2024 at 11:27 PM Paul Moore <[e-mail]> wrote:
> > On Wed, Jul 31, 2024 at 4:54 PM Jann Horn <[e-mail]> 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/[e-mail]

Patch

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ab927a142f51..511bf79fa14c 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -21,6 +21,7 @@ 
 #include <linux/security.h>
 #include <linux/uio.h>
 #include <linux/uaccess.h>
+#include <linux/ptrace.h>
 #include <keys/request_key_auth-type.h>
 #include "internal.h"
 
@@ -1687,6 +1688,10 @@  long keyctl_session_to_parent(void)
 	    !gid_eq(pcred->sgid, mycred->egid))
 		goto unlock;
 
+	/* The child must be allowed to impersonate its parent process. */
+	if (!ptrace_may_access(parent, PTRACE_MODE_ATTACH_REALCREDS))
+		goto unlock;
+
 	/* the keyrings must have the same UID */
 	if ((pcred->session_keyring &&
 	     !uid_eq(pcred->session_keyring->uid, mycred->euid)) ||