On Wed, Jul 31, 2024 at 11:33:04PM +0200, Jann Horn wrote:

On Wed, Jul 31, 2024 at 11:27 PM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 31, 2024 at 4:54 PM Jann Horn [off-list ref] wrote:

quoted

On Wed, Jul 31, 2024 at 10:29 PM Paul Moore [off-list ref] wrote:

quoted

On Mon, Jul 29, 2024 at 11:17 AM Mickaël Salaün [off-list ref] wrote:

quoted

On Mon, Jul 29, 2024 at 05:06:10PM +0200, Jann Horn wrote:

quoted

On Mon, Jul 29, 2024 at 5:02 PM Mickaël Salaün [off-list ref] wrote:

quoted

On Mon, Jul 29, 2024 at 04:21:01PM +0200, Jann Horn wrote:

quoted

On Mon, Jul 29, 2024 at 4:09 PM Mickaël Salaün [off-list ref] wrote:

quoted

On Mon, Jul 29, 2024 at 03:49:29PM +0200, Jann Horn wrote:

quoted

On Mon, Jul 29, 2024 at 2:59 PM Mickaël Salaün [off-list ref] wrote:

quoted

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

quoted

quoted

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?

quoted

quoted

quoted

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.