[+Cc linux-crypto]
On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> This patch is to introduce ECDH into keyctl syscall for
> userspace usage, containing public key generation and
> shared secret computation.
>
> It is mainly based on dh code, so it has the same condition
> to the input which only user keys is supported. The output
> result is storing into the buffer with the provided length.
>
> Signed-off-by: Zhang Yiqun <[email protected]>
> ---
> Documentation/security/keys/core.rst | 62 ++++++
> include/linux/compat.h | 4 +
> include/uapi/linux/keyctl.h | 11 +
> security/keys/Kconfig | 12 +
> security/keys/Makefile | 2 +
> security/keys/compat_ecdh.c | 50 +++++
> security/keys/ecdh.c | 318 +++++++++++++++++++++++++++
> security/keys/internal.h | 44 ++++
> security/keys/keyctl.c | 10 +
> 9 files changed, 513 insertions(+)
> create mode 100644 security/keys/compat_ecdh.c
> create mode 100644 security/keys/ecdh.c
Nacked-by: Eric Biggers <[email protected]>
The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing enough
problems. We do not need any more UAPIs like this. They are hard to maintain,
break often, not properly documented, increase the kernel's attack surface, and
what they do is better done in userspace.
Please refer to the recent thread
https://lore.kernel.org/linux-crypto/[email protected]/T/#u
where these issues were discussed in detail.
- Eric
On Sat, 2024-03-30 at 00:04 -0700, Eric Biggers wrote:
> [+Cc linux-crypto]
>
> On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> > This patch is to introduce ECDH into keyctl syscall for
> > userspace usage, containing public key generation and
> > shared secret computation.
> >
> > It is mainly based on dh code, so it has the same condition
> > to the input which only user keys is supported. The output
> > result is storing into the buffer with the provided length.
> >
> > Signed-off-by: Zhang Yiqun <[email protected]>
> > ---
> > Documentation/security/keys/core.rst | 62 ++++++
> > include/linux/compat.h | 4 +
> > include/uapi/linux/keyctl.h | 11 +
> > security/keys/Kconfig | 12 +
> > security/keys/Makefile | 2 +
> > security/keys/compat_ecdh.c | 50 +++++
> > security/keys/ecdh.c | 318
> > +++++++++++++++++++++++++++
> > security/keys/internal.h | 44 ++++
> > security/keys/keyctl.c | 10 +
> > 9 files changed, 513 insertions(+)
> > create mode 100644 security/keys/compat_ecdh.c
> > create mode 100644 security/keys/ecdh.c
>
> Nacked-by: Eric Biggers <[email protected]>
>
> The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing
> enough problems. We do not need any more UAPIs like this. They are
> hard to maintain, break often, not properly documented, increase the
> kernel's attack surface, and what they do is better done in
> userspace.
Actually that's not entirely true. There is a use case for keys which
is where you'd like to harden unwrapped key handling and don't have the
ability to use a device. The kernel provides a harder exfiltration
environment than user space, so there is a use case for getting the
kernel to handle operations on unwrapped keys for the protection it
affords the crytpographic key material.
For instance there are people who use the kernel keyring to replace
ssh-agent and thus *reduce* the attack surface they have for storing
ssh keys:
https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
The same thing could be done with gpg keys or the gnome keyring.
> Please refer to the recent thread
> https://lore.kernel.org/linux-crypto/[email protected]/T/#u
> where these issues were discussed in detail.
This thread was talking about using the kernel for handling the
algorithms themselves (which is probably best done in userspace) and
didn't address using the kernel to harden the key protection
environment.
James
On Sat, Mar 30, 2024 at 09:09:51AM -0400, James Bottomley wrote:
> On Sat, 2024-03-30 at 00:04 -0700, Eric Biggers wrote:
> > [+Cc linux-crypto]
> >
> > On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> > > This patch is to introduce ECDH into keyctl syscall for
> > > userspace usage, containing public key generation and
> > > shared secret computation.
> > >
> > > It is mainly based on dh code, so it has the same condition
> > > to the input which only user keys is supported. The output
> > > result is storing into the buffer with the provided length.
> > >
> > > Signed-off-by: Zhang Yiqun <[email protected]>
> > > ---
> > > ?Documentation/security/keys/core.rst |? 62 ++++++
> > > ?include/linux/compat.h?????????????? |?? 4 +
> > > ?include/uapi/linux/keyctl.h????????? |? 11 +
> > > ?security/keys/Kconfig??????????????? |? 12 +
> > > ?security/keys/Makefile?????????????? |?? 2 +
> > > ?security/keys/compat_ecdh.c????????? |? 50 +++++
> > > ?security/keys/ecdh.c???????????????? | 318
> > > +++++++++++++++++++++++++++
> > > ?security/keys/internal.h???????????? |? 44 ++++
> > > ?security/keys/keyctl.c?????????????? |? 10 +
> > > ?9 files changed, 513 insertions(+)
> > > ?create mode 100644 security/keys/compat_ecdh.c
> > > ?create mode 100644 security/keys/ecdh.c
> >
> > Nacked-by: Eric Biggers <[email protected]>
> >
> > The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing
> > enough problems.? We do not need any more UAPIs like this.? They are
> > hard to maintain, break often, not properly documented, increase the
> > kernel's attack surface, and what they do is better done in
> > userspace.
>
> Actually that's not entirely true. There is a use case for keys which
> is where you'd like to harden unwrapped key handling and don't have the
> ability to use a device. The kernel provides a harder exfiltration
> environment than user space, so there is a use case for getting the
> kernel to handle operations on unwrapped keys for the protection it
> affords the crytpographic key material.
>
> For instance there are people who use the kernel keyring to replace
> ssh-agent and thus *reduce* the attack surface they have for storing
> ssh keys:
>
> https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
>
> The same thing could be done with gpg keys or the gnome keyring.
First, that blog post never actually said that the "replace ssh-agent with
kernel keyrings" idea was deployed. It sounds like a proof of concept idea that
someone thought was interesting and decided to blog about. Upstream OpenSSH has
no support for Linux keyrings. It seems unlikely it would get added, especially
given the OpenSSH developers' healthy skepticism of using broken Linux-isms.
You're welcome to bring it up on openssh-unix-dev and get their buy-in first.
Second, as mentioned by the blog post, the kernel also does not support private
keys in the default OpenSSH format. That sort of thing is an example of the
fundamental problem with trying to make the kernel support every cryptographic
protocol and format in existence. Userspace simply has much more flexibility to
implement whatever it happens to need.
Third, ssh-agent is already a separate process, and like any other process the
kernel enforces isolation of its address space. The potential loopholes are
ptrace and coredumps, which ssh-agent already disables, except for ptrace by
root which it can't do alone, but the system administrator can do that by
setting the ptrace_scope sysctl to 3 or by using SELinux.
Amusingly, the existing KEYCTL_DH_* APIs, and the KEYCTL_ECDH_* APIs proposed by
this patch, only operate on user keys that the process has READ access to. This
means that the keys can be trivially extracted by a shell script running in your
user session. That's *less* secure than using an isolated process...
That's not to mention that merging this will likely add vulnerabilities
reachable by unprivileged users, just as the original KEYCTL_DH_* did. I had to
fix some of them last time around (e.g. commits 12d41a023efb, bbe240454d86,
3619dec5103d, 1d9ddde12e3c, ccd9888f14a8, 199512b1234f). I don't really feel
like doing it again. (Wait, was this supposed to *improve* security?)
> > Please refer to the recent thread
> > https://lore.kernel.org/linux-crypto/[email protected]/T/#u
> > where these issues were discussed in detail.
>
> This thread was talking about using the kernel for handling the
> algorithms themselves (which is probably best done in userspace) and
> didn't address using the kernel to harden the key protection
> environment.
This patch is about using the kernel to handle a class of algorithm,
Elliptic-Curve Diffie-Hellman. Which specific algorithm in that class (i.e.
which elliptic curve), who knows. Just like the existing APIs like this, it's
undocumented which algorithm(s) are actually supported.
- Eric
Hi Eric,
>
> Amusingly, the existing KEYCTL_DH_* APIs, and the KEYCTL_ECDH_* APIs proposed by
> this patch, only operate on user keys that the process has READ access to. This
> means that the keys can be trivially extracted by a shell script running in your
> user session. That's *less* secure than using an isolated process...
>
I can see this being true for user or session keys, but I don't think this is
true of process or thread specific keys. At least I couldn't read any keys out
of a test app when I tried.
Regards,
-Denis
On Sat, 2024-03-30 at 17:48 -0700, Eric Biggers wrote:
> On Sat, Mar 30, 2024 at 09:09:51AM -0400, James Bottomley wrote:
[...]
> > For instance there are people who use the kernel keyring to replace
> > ssh-agent and thus *reduce* the attack surface they have for
> > storing
> > ssh keys:
> >
> > https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
> >
> > The same thing could be done with gpg keys or the gnome keyring.
>
> First, that blog post never actually said that the "replace ssh-agent
> with kernel keyrings" idea was deployed. It sounds like a proof of
> concept idea that someone thought was interesting and decided to blog
> about. Upstream OpenSSH has no support for Linux keyrings.
The openssh community is incredibly resistant to out of house
innovation. It has no support for engine or provider keys, for TPM
keys, or for that systemd start patch xz just exploited ...
> It seems unlikely it would get added, especially given the OpenSSH
> developers' healthy skepticism of using broken Linux-isms.
> You're welcome to bring it up on openssh-unix-dev and get their buy-
> in first.
I also didn't say just openssh. You picked the one you apparently know
hardly ever accepts anyone else's ideas. I don't disagree that finding
implementors is reasonable ... I just wouldn't pick openssh as the
first upstream target.
> Second, as mentioned by the blog post, the kernel also does not
> support private keys in the default OpenSSH format. That sort of
> thing is an example of the fundamental problem with trying to make
> the kernel support every cryptographic protocol and format in
> existence. Userspace simply has much more flexibility to implement
> whatever it happens to need.
That's a complete red herring. You don't need the kernel keyrings to
support every format, you just need a user space converter to import to
the kernel keyring format. Every device or token that can replace key
handling has their own internal format and they all come with importers
that do conversion.
> Third, ssh-agent is already a separate process, and like any other
> process the kernel enforces isolation of its address space. The
> potential loopholes are ptrace and coredumps, which ssh-agent already
> disables, except for ptrace by root which it can't do alone, but the
> system administrator can do that by setting the ptrace_scope sysctl
> to 3 or by using SELinux.
Well, a) this doesn't survive privilege escalation and b) I don't think
many people would buy into the notion that we should remove security
functions from the kernel and give them to userspace daemons because
it's safer.
James
On Sat Mar 30, 2024 at 3:09 PM EET, James Bottomley wrote:
> On Sat, 2024-03-30 at 00:04 -0700, Eric Biggers wrote:
> > [+Cc linux-crypto]
> >
> > On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> > > This patch is to introduce ECDH into keyctl syscall for
> > > userspace usage, containing public key generation and
> > > shared secret computation.
> > >
> > > It is mainly based on dh code, so it has the same condition
> > > to the input which only user keys is supported. The output
> > > result is storing into the buffer with the provided length.
> > >
> > > Signed-off-by: Zhang Yiqun <[email protected]>
> > > ---
> > > Documentation/security/keys/core.rst | 62 ++++++
> > > include/linux/compat.h | 4 +
> > > include/uapi/linux/keyctl.h | 11 +
> > > security/keys/Kconfig | 12 +
> > > security/keys/Makefile | 2 +
> > > security/keys/compat_ecdh.c | 50 +++++
> > > security/keys/ecdh.c | 318
> > > +++++++++++++++++++++++++++
> > > security/keys/internal.h | 44 ++++
> > > security/keys/keyctl.c | 10 +
> > > 9 files changed, 513 insertions(+)
> > > create mode 100644 security/keys/compat_ecdh.c
> > > create mode 100644 security/keys/ecdh.c
> >
> > Nacked-by: Eric Biggers <[email protected]>
> >
> > The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing
> > enough problems. We do not need any more UAPIs like this. They are
> > hard to maintain, break often, not properly documented, increase the
> > kernel's attack surface, and what they do is better done in
> > userspace.
>
> Actually that's not entirely true. There is a use case for keys which
> is where you'd like to harden unwrapped key handling and don't have the
> ability to use a device. The kernel provides a harder exfiltration
> environment than user space, so there is a use case for getting the
> kernel to handle operations on unwrapped keys for the protection it
> affords the crytpographic key material.
>
> For instance there are people who use the kernel keyring to replace
> ssh-agent and thus *reduce* the attack surface they have for storing
> ssh keys:
>
> https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
>
> The same thing could be done with gpg keys or the gnome keyring.
Eric has a correct standing given that the commit message does not have
motivation part at all.
With a description of the problem that this patch is supposed to solve
this would be more meaningful to review.
BR, Jarkko