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

["Jarkko Sakkinen" <jarkko.sakkinen@iki.fi>]

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 <dhowells@redhat.com>
> Cc: Günther Noack <gnoack@google.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Kees Cook <kees@kernel.org>
> Cc: Paul Moore <paul@paul-moore.com>
> 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 <mic@digikod.net>
> Link: https://lore.kernel.org/r/20240729125846.1043211-1-mic@digikod.net
> ---
>  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