2021-01-12 16:15:44

by Toke Høiland-Jørgensen

[permalink] [raw]
Subject: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

When public_key_verify_signature() is called from
asymmetric_key_verify_signature(), the pkey_algo field of struct
public_key_signature will be NULL, which causes a NULL pointer dereference
in the strcmp() check. Fix this by adding a NULL check.

One visible manifestation of this is that userspace programs (such as the
'iwd' WiFi daemon) will be killed when trying to verify a TLS key using the
keyctl(2) interface.

Cc: [email protected]
Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
---
crypto/asymmetric_keys/public_key.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 8892908ad58c..35b09e95a870 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -356,7 +356,7 @@ int public_key_verify_signature(const struct public_key *pkey,
if (ret)
goto error_free_key;

- if (strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
+ if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
ret = cert_sig_digest_update(sig, tfm);
if (ret)
goto error_free_key;
--
2.30.0


2021-01-13 03:43:12

by Tianjia Zhang

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

Hi,

I have fixed this problem last week. Still thanks for your fixing.
patch is here: https://lkml.org/lkml/2021/1/7/201

Best regards,
Tianjia

On 1/13/21 12:10 AM, Toke Høiland-Jørgensen wrote:
> When public_key_verify_signature() is called from
> asymmetric_key_verify_signature(), the pkey_algo field of struct
> public_key_signature will be NULL, which causes a NULL pointer dereference
> in the strcmp() check. Fix this by adding a NULL check.
>
> One visible manifestation of this is that userspace programs (such as the
> 'iwd' WiFi daemon) will be killed when trying to verify a TLS key using the
> keyctl(2) interface.
>
> Cc: [email protected]
> Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
> Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
> ---
> crypto/asymmetric_keys/public_key.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 8892908ad58c..35b09e95a870 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -356,7 +356,7 @@ int public_key_verify_signature(const struct public_key *pkey,
> if (ret)
> goto error_free_key;
>
> - if (strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
> + if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
> ret = cert_sig_digest_update(sig, tfm);
> if (ret)
> goto error_free_key;
>

2021-01-13 11:14:48

by David Howells

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

I'm intending to use Tianjia's patch. Would you like to add a Reviewed-by?

David
---
commit 11078a592e6dcea6b9f30e822d3d30e3defc99ca
Author: Tianjia Zhang <[email protected]>
Date: Thu Jan 7 17:28:55 2021 +0800

X.509: Fix crash caused by NULL pointer

On the following call path, `sig->pkey_algo` is not assigned
in asymmetric_key_verify_signature(), which causes runtime
crash in public_key_verify_signature().

keyctl_pkey_verify
asymmetric_key_verify_signature
verify_signature
public_key_verify_signature

This patch simply check this situation and fixes the crash
caused by NULL pointer.

Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
Cc: [email protected] # v5.10+
Reported-by: Tobias Markus <[email protected]>
Signed-off-by: Tianjia Zhang <[email protected]>
Signed-off-by: David Howells <[email protected]>

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 8892908ad58c..788a4ba1e2e7 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -356,7 +356,8 @@ int public_key_verify_signature(const struct public_key *pkey,
if (ret)
goto error_free_key;

- if (strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
+ if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 &&
+ sig->data_size) {
ret = cert_sig_digest_update(sig, tfm);
if (ret)
goto error_free_key;

2021-01-13 11:31:44

by Toke Høiland-Jørgensen

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

Tianjia Zhang <[email protected]> writes:

> Hi,
>
> I have fixed this problem last week. Still thanks for your fixing.
> patch is here: https://lkml.org/lkml/2021/1/7/201

Ah, awesome! I did look if this had already been fixed, but your patch
wasn't in the crypto tree and didn't think to go perusing the mailing
lists. So sorry for the duplicate, and thanks for fixing this :)

-Toke

2021-01-13 11:38:11

by Toke Høiland-Jørgensen

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

David Howells <[email protected]> writes:

> I'm intending to use Tianjia's patch.

Yeah, sorry for missing that.

> Would you like to add a Reviewed-by?

Sure:

Reviewed-by: Toke Høiland-Jørgensen <[email protected]>

and also, if you like:

Tested-by: Toke Høiland-Jørgensen <[email protected]>

2021-01-13 13:00:03

by David Howells

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

Toke Høiland-Jørgensen <[email protected]> wrote:

> Reviewed-by: Toke Høiland-Jørgensen <[email protected]>
>
> and also, if you like:
>
> Tested-by: Toke Høiland-Jørgensen <[email protected]>

Thanks!

David

2021-01-14 02:57:26

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

On Wed, Jan 13, 2021 at 11:11:13AM +0000, David Howells wrote:
> I'm intending to use Tianjia's patch. Would you like to add a Reviewed-by?
>
> David

I can give.

Reviewed-by: Jarkko Sakkinen <[email protected]>

/Jarkko

> ---
> commit 11078a592e6dcea6b9f30e822d3d30e3defc99ca
> Author: Tianjia Zhang <[email protected]>
> Date: Thu Jan 7 17:28:55 2021 +0800
>
> X.509: Fix crash caused by NULL pointer
>
> On the following call path, `sig->pkey_algo` is not assigned
> in asymmetric_key_verify_signature(), which causes runtime
> crash in public_key_verify_signature().
>
> keyctl_pkey_verify
> asymmetric_key_verify_signature
> verify_signature
> public_key_verify_signature
>
> This patch simply check this situation and fixes the crash
> caused by NULL pointer.
>
> Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
> Cc: [email protected] # v5.10+
> Reported-by: Tobias Markus <[email protected]>
> Signed-off-by: Tianjia Zhang <[email protected]>
> Signed-off-by: David Howells <[email protected]>
>
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 8892908ad58c..788a4ba1e2e7 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -356,7 +356,8 @@ int public_key_verify_signature(const struct public_key *pkey,
> if (ret)
> goto error_free_key;
>
> - if (strcmp(sig->pkey_algo, "sm2") == 0 && sig->data_size) {
> + if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 &&
> + sig->data_size) {
> ret = cert_sig_digest_update(sig, tfm);
> if (ret)
> goto error_free_key;
>
>

2021-01-18 17:20:27

by Toke Høiland-Jørgensen

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

David Howells <[email protected]> writes:

> Toke Høiland-Jørgensen <[email protected]> wrote:
>
>> Reviewed-by: Toke Høiland-Jørgensen <[email protected]>
>>
>> and also, if you like:
>>
>> Tested-by: Toke Høiland-Jørgensen <[email protected]>
>
> Thanks!

Any chance of that patch getting into -stable anytime soon? Would be
nice to have working WiFi without having to compile my own kernels ;)

-Toke

2021-01-18 21:12:49

by João Fonseca

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

Toke Høiland-Jørgensen <[email protected]> wrote:

> David Howells <[email protected]> writes:
>
>> Toke Høiland-Jørgensen <[email protected]> wrote:
>>
>>> Reviewed-by: Toke Høiland-Jørgensen <[email protected]>
>>>
>>> and also, if you like:
>>>
>>> Tested-by: Toke Høiland-Jørgensen <[email protected]>
>> Thanks!
> Any chance of that patch getting into -stable anytime soon? Would be
> nice to have working WiFi without having to compile my own kernels ;)
>
> -Toke
>
>
>
I have also just tested the patch and it seems to be working with the
PEAP method. I would also like to have this patch in the stable branch
as I normally don't have to compile my own kernels.

Also, if you want to add another tester:

Tested-by: João Fonseca <[email protected]>

Thanks for the patch everyone.

-João

2021-01-21 06:04:19

by Tee Hao Wei

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

On 19/1/21 5:09 am, João Fonseca wrote:
> Toke Høiland-Jørgensen <[email protected]> wrote:
>
>> David Howells <[email protected]> writes:
>>
>>> Toke Høiland-Jørgensen <[email protected]> wrote:
>>>
>>>> Reviewed-by: Toke Høiland-Jørgensen <[email protected]>
>>>>
>>>> and also, if you like:
>>>>
>>>> Tested-by: Toke Høiland-Jørgensen <[email protected]>
>>> Thanks!
>> Any chance of that patch getting into -stable anytime soon? Would be
>> nice to have working WiFi without having to compile my own kernels ;)
>>
>> -Toke
>>
>>
>>
> I have also just tested the patch and it seems to be working with the
> PEAP method. I would also like to have this patch in the stable branch
> as I normally don't have to compile my own kernels.
>
> Also, if you want to add another tester:
>
> Tested-by: João Fonseca <[email protected]>
>
> Thanks for the patch everyone.
>
> -João
>
>

The patch finally made it to Torvalds's tree a few hours ago, so it
should hopefully land in the next stable patch.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7178a107f5ea7bdb1cc23073234f0ded0ef90ec7

--
Hao Wei

2021-03-10 12:04:21

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

On Mon, Jan 18, 2021 at 06:13:02PM +0100, Toke H?iland-J?rgensen wrote:
> David Howells <[email protected]> writes:
>
> > Toke H?iland-J?rgensen <[email protected]> wrote:
> >
> >> Reviewed-by: Toke H?iland-J?rgensen <[email protected]>
> >>
> >> and also, if you like:
> >>
> >> Tested-by: Toke H?iland-J?rgensen <[email protected]>
> >
> > Thanks!
>
> Any chance of that patch getting into -stable anytime soon? Would be
> nice to have working WiFi without having to compile my own kernels ;)

What ever happened to this patch? I can't seem to find it in Linus's
tree anywhere :(

thanks,

greg k-h

2021-03-15 10:54:05

by Toke Høiland-Jørgensen

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

Greg KH <[email protected]> writes:

> On Mon, Jan 18, 2021 at 06:13:02PM +0100, Toke Høiland-Jørgensen wrote:
>> David Howells <[email protected]> writes:
>>
>> > Toke Høiland-Jørgensen <[email protected]> wrote:
>> >
>> >> Reviewed-by: Toke Høiland-Jørgensen <[email protected]>
>> >>
>> >> and also, if you like:
>> >>
>> >> Tested-by: Toke Høiland-Jørgensen <[email protected]>
>> >
>> > Thanks!
>>
>> Any chance of that patch getting into -stable anytime soon? Would be
>> nice to have working WiFi without having to compile my own kernels ;)
>
> What ever happened to this patch? I can't seem to find it in Linus's
> tree anywhere :(

This was a matter of crossed streams: Tianjia had already submitted an
identical fix, which went in as:

7178a107f5ea ("X.509: Fix crash caused by NULL pointer")

And that has made it into -stable, so all is well as far as I'm
concerned. Sorry for the confusion!

-Toke

2021-03-15 12:09:30

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp()

On Mon, Mar 15, 2021 at 11:52:52AM +0100, Toke H?iland-J?rgensen wrote:
> Greg KH <[email protected]> writes:
>
> > On Mon, Jan 18, 2021 at 06:13:02PM +0100, Toke H?iland-J?rgensen wrote:
> >> David Howells <[email protected]> writes:
> >>
> >> > Toke H?iland-J?rgensen <[email protected]> wrote:
> >> >
> >> >> Reviewed-by: Toke H?iland-J?rgensen <[email protected]>
> >> >>
> >> >> and also, if you like:
> >> >>
> >> >> Tested-by: Toke H?iland-J?rgensen <[email protected]>
> >> >
> >> > Thanks!
> >>
> >> Any chance of that patch getting into -stable anytime soon? Would be
> >> nice to have working WiFi without having to compile my own kernels ;)
> >
> > What ever happened to this patch? I can't seem to find it in Linus's
> > tree anywhere :(
>
> This was a matter of crossed streams: Tianjia had already submitted an
> identical fix, which went in as:
>
> 7178a107f5ea ("X.509: Fix crash caused by NULL pointer")
>
> And that has made it into -stable, so all is well as far as I'm
> concerned. Sorry for the confusion!

No worries, thanks for letting me know.

greg k-h