* [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference
@ 2026-05-02 16:33 Weiming Shi
2026-05-05 9:34 ` Herbert Xu
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Weiming Shi @ 2026-05-02 16:33 UTC (permalink / raw)
To: David Howells, Lukas Wunner, Ignat Korchagin, Herbert Xu,
David S . Miller
Cc: Jarkko Sakkinen, keyrings, linux-crypto, Xiang Mei, Weiming Shi
asymmetric_key_ids() returns key->payload.data[asym_key_ids], which can
be NULL for keys parsed by the PKCS#8 parser (pkcs8_parser.c explicitly
stores NULL in prep->payload.data[asym_key_ids]).
key_or_keyring_common() in restrict.c and find_asymmetric_key() in
asymmetric_type.c both dereference this return value without checking
for NULL. An unprivileged user can trigger a NULL pointer dereference
in key_or_keyring_common() by creating a PKCS#8 key, restricting a
keyring with key_or_keyring:<pkcs8_serial>, and adding an X.509 cert
to the restricted keyring. CONFIG_PKCS8_PRIVATE_KEY_PARSER=y is
required.
The following bash script can reproduce the issue:
#!/bin/bash
modprobe pkcs8_key_parser 2>/dev/null
openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:1024 \
-out /tmp/poc.pem 2>/dev/null
openssl pkcs8 -topk8 -nocrypt -in /tmp/poc.pem \
-outform DER -out /tmp/poc.p8
openssl req -new -x509 -key /tmp/poc.pem -outform DER \
-out /tmp/poc.der -days 365 -subj "/CN=Test" \
-addext "subjectKeyIdentifier=hash" \
-addext "authorityKeyIdentifier=keyid:always" 2>/dev/null
PKCS8_ID=$(keyctl padd asymmetric pkcs8key @s < /tmp/poc.p8)
KR=$(keyctl newring test_kr @s)
keyctl restrict_keyring $KR asymmetric "key_or_keyring:$PKCS8_ID"
keyctl padd asymmetric trigger $KR < /tmp/poc.der
rm -f /tmp/poc.pem /tmp/poc.p8 /tmp/poc.der
Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
RIP: 0010:key_or_keyring_common (crypto/asymmetric_keys/restrict.c:205 crypto/asymmetric_keys/restrict.c:279)
Call Trace:
<TASK>
__key_create_or_update (security/keys/key.c:884)
key_create_or_update (security/keys/key.c:1021)
__do_sys_add_key (security/keys/keyctl.c:134)
do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
</TASK>
Kernel panic - not syncing: Fatal exception
Add a NULL check in find_asymmetric_key(), mirroring the existing
pattern in asymmetric_match_key_ids() and asymmetric_key_describe().
In key_or_keyring_common(), skip the trusted key matching when it
has no key IDs and fall through to the check_dest path.
Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
v2: add bash reproducer to commit message (Ignat)
crypto/asymmetric_keys/asymmetric_type.c | 2 ++
crypto/asymmetric_keys/restrict.c | 9 +++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index 16a7ae16593c..22f04656d529 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -109,6 +109,8 @@ struct key *find_asymmetric_key(struct key *keyring,
if (id_0 && id_1) {
const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
+ if (!kids)
+ goto reject;
if (!kids->id[1]) {
pr_debug("First ID matches, but second is missing\n");
goto reject;
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 86292965f493..ccf1084f720e 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
if (IS_ERR(key))
key = NULL;
} else if (trusted->type == &key_type_asymmetric) {
+ const struct asymmetric_key_ids *kids;
const struct asymmetric_key_id **signer_ids;
- signer_ids = (const struct asymmetric_key_id **)
- asymmetric_key_ids(trusted)->id;
+ kids = asymmetric_key_ids(trusted);
+ if (!kids)
+ goto skip_trusted;
+
+ signer_ids = (const struct asymmetric_key_id **)kids->id;
/*
* The auth_ids come from the candidate key (the
@@ -290,6 +294,7 @@ static int key_or_keyring_common(struct key *dest_keyring,
}
}
+skip_trusted:
if (check_dest && !key) {
/* See if the destination has a key that signed this one. */
key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference
2026-05-02 16:33 [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference Weiming Shi
@ 2026-05-05 9:34 ` Herbert Xu
2026-06-22 4:16 ` Herbert Xu
2026-09-06 11:01 ` [PATCH v3] asymmetric_keys: reject trust keys without IDs Weiming Shi
2 siblings, 0 replies; 7+ messages in thread
From: Herbert Xu @ 2026-05-05 9:34 UTC (permalink / raw)
To: Weiming Shi
Cc: David Howells, Lukas Wunner, Ignat Korchagin, David S . Miller,
Jarkko Sakkinen, keyrings, linux-crypto, Xiang Mei
On Sat, May 02, 2026 at 09:33:29AM -0700, Weiming Shi wrote:
> asymmetric_key_ids() returns key->payload.data[asym_key_ids], which can
> be NULL for keys parsed by the PKCS#8 parser (pkcs8_parser.c explicitly
> stores NULL in prep->payload.data[asym_key_ids]).
>
> key_or_keyring_common() in restrict.c and find_asymmetric_key() in
> asymmetric_type.c both dereference this return value without checking
> for NULL. An unprivileged user can trigger a NULL pointer dereference
> in key_or_keyring_common() by creating a PKCS#8 key, restricting a
> keyring with key_or_keyring:<pkcs8_serial>, and adding an X.509 cert
> to the restricted keyring. CONFIG_PKCS8_PRIVATE_KEY_PARSER=y is
> required.
>
> The following bash script can reproduce the issue:
>
> #!/bin/bash
> modprobe pkcs8_key_parser 2>/dev/null
> openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:1024 \
> -out /tmp/poc.pem 2>/dev/null
> openssl pkcs8 -topk8 -nocrypt -in /tmp/poc.pem \
> -outform DER -out /tmp/poc.p8
> openssl req -new -x509 -key /tmp/poc.pem -outform DER \
> -out /tmp/poc.der -days 365 -subj "/CN=Test" \
> -addext "subjectKeyIdentifier=hash" \
> -addext "authorityKeyIdentifier=keyid:always" 2>/dev/null
> PKCS8_ID=$(keyctl padd asymmetric pkcs8key @s < /tmp/poc.p8)
> KR=$(keyctl newring test_kr @s)
> keyctl restrict_keyring $KR asymmetric "key_or_keyring:$PKCS8_ID"
> keyctl padd asymmetric trigger $KR < /tmp/poc.der
> rm -f /tmp/poc.pem /tmp/poc.p8 /tmp/poc.der
>
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000
> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> RIP: 0010:key_or_keyring_common (crypto/asymmetric_keys/restrict.c:205 crypto/asymmetric_keys/restrict.c:279)
> Call Trace:
> <TASK>
> __key_create_or_update (security/keys/key.c:884)
> key_create_or_update (security/keys/key.c:1021)
> __do_sys_add_key (security/keys/keyctl.c:134)
> do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> </TASK>
> Kernel panic - not syncing: Fatal exception
>
> Add a NULL check in find_asymmetric_key(), mirroring the existing
> pattern in asymmetric_match_key_ids() and asymmetric_key_describe().
> In key_or_keyring_common(), skip the trusted key matching when it
> has no key IDs and fall through to the check_dest path.
>
> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
> v2: add bash reproducer to commit message (Ignat)
>
> crypto/asymmetric_keys/asymmetric_type.c | 2 ++
> crypto/asymmetric_keys/restrict.c | 9 +++++++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference
2026-05-02 16:33 [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference Weiming Shi
2026-05-05 9:34 ` Herbert Xu
@ 2026-06-22 4:16 ` Herbert Xu
2026-06-22 14:56 ` Jarkko Sakkinen
2026-09-06 11:01 ` [PATCH v3] asymmetric_keys: reject trust keys without IDs Weiming Shi
2 siblings, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2026-06-22 4:16 UTC (permalink / raw)
To: Weiming Shi
Cc: David Howells, Lukas Wunner, Ignat Korchagin, David S . Miller,
Jarkko Sakkinen, keyrings, linux-crypto, Xiang Mei
On Sat, May 02, 2026 at 09:33:29AM -0700, Weiming Shi wrote:
>
> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> index 16a7ae16593c..22f04656d529 100644
> --- a/crypto/asymmetric_keys/asymmetric_type.c
> +++ b/crypto/asymmetric_keys/asymmetric_type.c
> @@ -109,6 +109,8 @@ struct key *find_asymmetric_key(struct key *keyring,
> if (id_0 && id_1) {
> const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
>
> + if (!kids)
> + goto reject;
This check is actually unnecessary because we've already matched
the key against the kid so it must be present.
I'd get rid of this check or perhaps add a comment instead.
> if (!kids->id[1]) {
> pr_debug("First ID matches, but second is missing\n");
> goto reject;
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index 86292965f493..ccf1084f720e 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
> if (IS_ERR(key))
> key = NULL;
> } else if (trusted->type == &key_type_asymmetric) {
> + const struct asymmetric_key_ids *kids;
> const struct asymmetric_key_id **signer_ids;
>
> - signer_ids = (const struct asymmetric_key_id **)
> - asymmetric_key_ids(trusted)->id;
> + kids = asymmetric_key_ids(trusted);
> + if (!kids)
> + goto skip_trusted;
Yes this is definitely buggy.
I think it was introduced by these two commits:
commit 3c58b2362ba828ee2970c66c6a6fd7b04fde4413
Author: David Howells <dhowells@redhat.com>
Date: Tue Oct 9 17:47:46 2018 +0100
KEYS: Implement PKCS#8 RSA Private Key parser [ver #2]
and
commit 7e3c4d22083f6e7316c5229b6197ca2d5335aa35
Author: Mat Martineau <martineau@kernel.org>
Date: Mon Jun 27 16:45:16 2016 -0700
KEYS: Restrict asymmetric key linkage using a specific keychain
So the Fixes header should point to them.
> @@ -290,6 +294,7 @@ static int key_or_keyring_common(struct key *dest_keyring,
> }
> }
>
> +skip_trusted:
> if (check_dest && !key) {
> /* See if the destination has a key that signed this one. */
> key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
I'm not sure continuing here is a good idea. Having a private key
here makes no sense whatsoever and we should just bail out right
away.
I would recommend returning an error of some sort if kids is NULL.
David/Lukas/Ignat, any opinions?
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference
2026-06-22 4:16 ` Herbert Xu
@ 2026-06-22 14:56 ` Jarkko Sakkinen
2026-06-22 20:21 ` Ignat Korchagin
0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2026-06-22 14:56 UTC (permalink / raw)
To: Herbert Xu
Cc: Weiming Shi, David Howells, Lukas Wunner, Ignat Korchagin,
David S . Miller, keyrings, linux-crypto, Xiang Mei
On Mon, Jun 22, 2026 at 12:16:07PM +0800, Herbert Xu wrote:
> On Sat, May 02, 2026 at 09:33:29AM -0700, Weiming Shi wrote:
> >
> > diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> > index 16a7ae16593c..22f04656d529 100644
> > --- a/crypto/asymmetric_keys/asymmetric_type.c
> > +++ b/crypto/asymmetric_keys/asymmetric_type.c
> > @@ -109,6 +109,8 @@ struct key *find_asymmetric_key(struct key *keyring,
> > if (id_0 && id_1) {
> > const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
> >
> > + if (!kids)
> > + goto reject;
>
> This check is actually unnecessary because we've already matched
> the key against the kid so it must be present.
>
> I'd get rid of this check or perhaps add a comment instead.
+1
>
> > if (!kids->id[1]) {
> > pr_debug("First ID matches, but second is missing\n");
> > goto reject;
> > diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> > index 86292965f493..ccf1084f720e 100644
> > --- a/crypto/asymmetric_keys/restrict.c
> > +++ b/crypto/asymmetric_keys/restrict.c
> > @@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
> > if (IS_ERR(key))
> > key = NULL;
> > } else if (trusted->type == &key_type_asymmetric) {
> > + const struct asymmetric_key_ids *kids;
> > const struct asymmetric_key_id **signer_ids;
> >
> > - signer_ids = (const struct asymmetric_key_id **)
> > - asymmetric_key_ids(trusted)->id;
> > + kids = asymmetric_key_ids(trusted);
> > + if (!kids)
> > + goto skip_trusted;
>
> Yes this is definitely buggy.
>
> I think it was introduced by these two commits:
>
> commit 3c58b2362ba828ee2970c66c6a6fd7b04fde4413
> Author: David Howells <dhowells@redhat.com>
> Date: Tue Oct 9 17:47:46 2018 +0100
>
> KEYS: Implement PKCS#8 RSA Private Key parser [ver #2]
>
> and
>
> commit 7e3c4d22083f6e7316c5229b6197ca2d5335aa35
> Author: Mat Martineau <martineau@kernel.org>
> Date: Mon Jun 27 16:45:16 2016 -0700
>
> KEYS: Restrict asymmetric key linkage using a specific keychain
>
> So the Fixes header should point to them.
+1
>
> > @@ -290,6 +294,7 @@ static int key_or_keyring_common(struct key *dest_keyring,
> > }
> > }
> >
> > +skip_trusted:
> > if (check_dest && !key) {
> > /* See if the destination has a key that signed this one. */
> > key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
>
> I'm not sure continuing here is a good idea. Having a private key
> here makes no sense whatsoever and we should just bail out right
> away.
>
> I would recommend returning an error of some sort if kids is NULL.
>
> David/Lukas/Ignat, any opinions?
I think with a quick skim that you are right. I'll work on this area
for the next version.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Thanks for the review!
BR, Jarkko
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference
2026-06-22 14:56 ` Jarkko Sakkinen
@ 2026-06-22 20:21 ` Ignat Korchagin
2026-06-24 22:34 ` Jarkko Sakkinen
0 siblings, 1 reply; 7+ messages in thread
From: Ignat Korchagin @ 2026-06-22 20:21 UTC (permalink / raw)
To: Herbert Xu, Weiming Shi
Cc: David Howells, Jarkko Sakkinen, Lukas Wunner, David S . Miller,
keyrings, linux-crypto, Xiang Mei
On Mon, Jun 22, 2026 at 3:56 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Mon, Jun 22, 2026 at 12:16:07PM +0800, Herbert Xu wrote:
> > On Sat, May 02, 2026 at 09:33:29AM -0700, Weiming Shi wrote:
> > >
> > > diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> > > index 16a7ae16593c..22f04656d529 100644
> > > --- a/crypto/asymmetric_keys/asymmetric_type.c
> > > +++ b/crypto/asymmetric_keys/asymmetric_type.c
> > > @@ -109,6 +109,8 @@ struct key *find_asymmetric_key(struct key *keyring,
> > > if (id_0 && id_1) {
> > > const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
> > >
> > > + if (!kids)
> > > + goto reject;
> >
> > This check is actually unnecessary because we've already matched
> > the key against the kid so it must be present.
> >
> > I'd get rid of this check or perhaps add a comment instead.
>
> +1
>
> >
> > > if (!kids->id[1]) {
> > > pr_debug("First ID matches, but second is missing\n");
> > > goto reject;
> > > diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> > > index 86292965f493..ccf1084f720e 100644
> > > --- a/crypto/asymmetric_keys/restrict.c
> > > +++ b/crypto/asymmetric_keys/restrict.c
> > > @@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
> > > if (IS_ERR(key))
> > > key = NULL;
> > > } else if (trusted->type == &key_type_asymmetric) {
> > > + const struct asymmetric_key_ids *kids;
> > > const struct asymmetric_key_id **signer_ids;
> > >
> > > - signer_ids = (const struct asymmetric_key_id **)
> > > - asymmetric_key_ids(trusted)->id;
> > > + kids = asymmetric_key_ids(trusted);
> > > + if (!kids)
> > > + goto skip_trusted;
> >
> > Yes this is definitely buggy.
> >
> > I think it was introduced by these two commits:
> >
> > commit 3c58b2362ba828ee2970c66c6a6fd7b04fde4413
> > Author: David Howells <dhowells@redhat.com>
> > Date: Tue Oct 9 17:47:46 2018 +0100
> >
> > KEYS: Implement PKCS#8 RSA Private Key parser [ver #2]
> >
> > and
> >
> > commit 7e3c4d22083f6e7316c5229b6197ca2d5335aa35
> > Author: Mat Martineau <martineau@kernel.org>
> > Date: Mon Jun 27 16:45:16 2016 -0700
> >
> > KEYS: Restrict asymmetric key linkage using a specific keychain
> >
> > So the Fixes header should point to them.
>
> +1
>
> >
> > > @@ -290,6 +294,7 @@ static int key_or_keyring_common(struct key *dest_keyring,
> > > }
> > > }
> > >
> > > +skip_trusted:
> > > if (check_dest && !key) {
> > > /* See if the destination has a key that signed this one. */
> > > key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
> >
> > I'm not sure continuing here is a good idea. Having a private key
> > here makes no sense whatsoever and we should just bail out right
> > away.
> >
> > I would recommend returning an error of some sort if kids is NULL.
> >
> > David/Lukas/Ignat, any opinions?
As I reread the original submission (somehow I never got the V2) it
seems we're restricting a keyring with a private key?! Which indeed
does not make sense.
> I think with a quick skim that you are right. I'll work on this area
> for the next version.
>
> >
> > Thanks,
> > --
> > Email: Herbert Xu <herbert@gondor.apana.org.au>
> > Home Page: http://gondor.apana.org.au/~herbert/
> > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
> Thanks for the review!
>
> BR, Jarkko
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference
2026-06-22 20:21 ` Ignat Korchagin
@ 2026-06-24 22:34 ` Jarkko Sakkinen
0 siblings, 0 replies; 7+ messages in thread
From: Jarkko Sakkinen @ 2026-06-24 22:34 UTC (permalink / raw)
To: Ignat Korchagin
Cc: Herbert Xu, Weiming Shi, David Howells, Lukas Wunner,
David S . Miller, keyrings, linux-crypto, Xiang Mei
On Mon, Jun 22, 2026 at 09:21:04PM +0100, Ignat Korchagin wrote:
> On Mon, Jun 22, 2026 at 3:56 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Mon, Jun 22, 2026 at 12:16:07PM +0800, Herbert Xu wrote:
> > > On Sat, May 02, 2026 at 09:33:29AM -0700, Weiming Shi wrote:
> > > >
> > > > diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> > > > index 16a7ae16593c..22f04656d529 100644
> > > > --- a/crypto/asymmetric_keys/asymmetric_type.c
> > > > +++ b/crypto/asymmetric_keys/asymmetric_type.c
> > > > @@ -109,6 +109,8 @@ struct key *find_asymmetric_key(struct key *keyring,
> > > > if (id_0 && id_1) {
> > > > const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
> > > >
> > > > + if (!kids)
> > > > + goto reject;
> > >
> > > This check is actually unnecessary because we've already matched
> > > the key against the kid so it must be present.
> > >
> > > I'd get rid of this check or perhaps add a comment instead.
> >
> > +1
> >
> > >
> > > > if (!kids->id[1]) {
> > > > pr_debug("First ID matches, but second is missing\n");
> > > > goto reject;
> > > > diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> > > > index 86292965f493..ccf1084f720e 100644
> > > > --- a/crypto/asymmetric_keys/restrict.c
> > > > +++ b/crypto/asymmetric_keys/restrict.c
> > > > @@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
> > > > if (IS_ERR(key))
> > > > key = NULL;
> > > > } else if (trusted->type == &key_type_asymmetric) {
> > > > + const struct asymmetric_key_ids *kids;
> > > > const struct asymmetric_key_id **signer_ids;
> > > >
> > > > - signer_ids = (const struct asymmetric_key_id **)
> > > > - asymmetric_key_ids(trusted)->id;
> > > > + kids = asymmetric_key_ids(trusted);
> > > > + if (!kids)
> > > > + goto skip_trusted;
> > >
> > > Yes this is definitely buggy.
> > >
> > > I think it was introduced by these two commits:
> > >
> > > commit 3c58b2362ba828ee2970c66c6a6fd7b04fde4413
> > > Author: David Howells <dhowells@redhat.com>
> > > Date: Tue Oct 9 17:47:46 2018 +0100
> > >
> > > KEYS: Implement PKCS#8 RSA Private Key parser [ver #2]
> > >
> > > and
> > >
> > > commit 7e3c4d22083f6e7316c5229b6197ca2d5335aa35
> > > Author: Mat Martineau <martineau@kernel.org>
> > > Date: Mon Jun 27 16:45:16 2016 -0700
> > >
> > > KEYS: Restrict asymmetric key linkage using a specific keychain
> > >
> > > So the Fixes header should point to them.
> >
> > +1
> >
> > >
> > > > @@ -290,6 +294,7 @@ static int key_or_keyring_common(struct key *dest_keyring,
> > > > }
> > > > }
> > > >
> > > > +skip_trusted:
> > > > if (check_dest && !key) {
> > > > /* See if the destination has a key that signed this one. */
> > > > key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
> > >
> > > I'm not sure continuing here is a good idea. Having a private key
> > > here makes no sense whatsoever and we should just bail out right
> > > away.
> > >
> > > I would recommend returning an error of some sort if kids is NULL.
> > >
> > > David/Lukas/Ignat, any opinions?
>
> As I reread the original submission (somehow I never got the V2) it
> seems we're restricting a keyring with a private key?! Which indeed
> does not make sense.
>
> > I think with a quick skim that you are right. I'll work on this area
> > for the next version.
BTW I was writing two emails simulatenously this last sentence was for
private email discussion.
> >
> > >
> > > Thanks,
> > > --
> > > Email: Herbert Xu <herbert@gondor.apana.org.au>
> > > Home Page: http://gondor.apana.org.au/~herbert/
> > > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> >
> > Thanks for the review!
And this.
> >
> > BR, Jarkko
> >
> >
+1's were for the discussion. Sorry, I should not multitask with
emails...
BR, Jarkko
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] asymmetric_keys: reject trust keys without IDs
2026-05-02 16:33 [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference Weiming Shi
2026-05-05 9:34 ` Herbert Xu
2026-06-22 4:16 ` Herbert Xu
@ 2026-09-06 11:01 ` Weiming Shi
2 siblings, 0 replies; 7+ messages in thread
From: Weiming Shi @ 2026-09-06 11:01 UTC (permalink / raw)
To: David Howells, Lukas Wunner, Ignat Korchagin, Herbert Xu,
David S . Miller
Cc: keyrings, linux-crypto, linux-kernel, Marcel Holtmann,
James Morris, Denis Kenzior, Xiang Mei, Weiming Shi, stable
The PKCS#8 parser deliberately leaves the asym_key_ids payload slot
empty. key_or_keyring_common() unconditionally dereferences that slot
when a PKCS#8 key is supplied to a key_or_keyring restriction. Both the
plain and :chain forms reach this branch, allowing an unprivileged caller
to trigger a NULL pointer dereference through KEYCTL_RESTRICT_KEYRING
followed by add_key().
Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
RIP: 0010:key_or_keyring_common (crypto/asymmetric_keys/restrict.c:205 crypto/asymmetric_keys/restrict.c:279)
Call Trace:
<TASK>
__key_create_or_update (security/keys/key.c:884)
key_create_or_update (security/keys/key.c:1021)
__do_sys_add_key (security/keys/keyctl.c:134)
do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
</TASK>
Kernel panic - not syncing: Fatal exception
Reject an asymmetric restriction key without key IDs. A PKCS#8 private
key cannot identify the signer of a candidate certificate and is not a
valid trust anchor.
Fixes: 3c58b2362ba8 ("KEYS: Implement PKCS#8 RSA Private Key parser [ver #2]")
Cc: stable@vger.kernel.org
Reported-by: Xiang Mei <xmei5@asu.edu>
Link: https://lore.kernel.org/r/20260429181629.110802-2-bestswngs@gmail.com
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
Changes in v3:
- Drop the unnecessary find_asymmetric_key() check.
- Reject a trust key without IDs instead of continuing the chain lookup.
- Use the PKCS#8 parser commit as the Fixes target.
crypto/asymmetric_keys/restrict.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 86292965f..0e4783ff7 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
if (IS_ERR(key))
key = NULL;
} else if (trusted->type == &key_type_asymmetric) {
+ const struct asymmetric_key_ids *kids;
const struct asymmetric_key_id **signer_ids;
- signer_ids = (const struct asymmetric_key_id **)
- asymmetric_key_ids(trusted)->id;
+ kids = asymmetric_key_ids(trusted);
+ if (!kids)
+ return -ENOKEY;
+
+ signer_ids = (const struct asymmetric_key_id **)kids->id;
/*
* The auth_ids come from the candidate key (the
--
2.55.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-06 11:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-02 16:33 [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference Weiming Shi
2026-05-05 9:34 ` Herbert Xu
2026-06-22 4:16 ` Herbert Xu
2026-06-22 14:56 ` Jarkko Sakkinen
2026-06-22 20:21 ` Ignat Korchagin
2026-06-24 22:34 ` Jarkko Sakkinen
2026-09-06 11:01 ` [PATCH v3] asymmetric_keys: reject trust keys without IDs Weiming Shi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.