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

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

[Toke Høiland-Jørgensen]

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: stable@vger.kernel.org
Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 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

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

[Tianjia Zhang]

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: stable@vger.kernel.org
> Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>   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;
> 

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

[David Howells]

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

David
---
commit 11078a592e6dcea6b9f30e822d3d30e3defc99ca
Author: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
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: stable@vger.kernel.org # v5.10+
    Reported-by: Tobias Markus <tobias@markus-regensburg.de>
    Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

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;

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

[Toke Høiland-Jørgensen]

Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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

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

[Toke Høiland-Jørgensen]

David Howells <dhowells@redhat.com> 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 <toke@redhat.com>

and also, if you like:

Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>

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

[David Howells]

Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> and also, if you like:
> 
> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>

Thanks!

David

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

[Jarkko Sakkinen]

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 <jarkko@kernel.org>

/Jarkko

> ---
> commit 11078a592e6dcea6b9f30e822d3d30e3defc99ca
> Author: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> 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: stable@vger.kernel.org # v5.10+
>     Reported-by: Tobias Markus <tobias@markus-regensburg.de>
>     Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
> 
> 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;
> 
> 

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

[Toke Høiland-Jørgensen]

David Howells <dhowells@redhat.com> writes:

> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> and also, if you like:
>> 
>> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> 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

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

[João Fonseca]

Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> David Howells <dhowells@redhat.com> writes:
>
>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>
>>> and also, if you like:
>>>
>>> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> 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 <jpedrofonseca@ua.pt>

Thanks for the patch everyone.

-João 

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

[Tee Hao Wei]

On 19/1/21 5:09 am, João Fonseca wrote:
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
>> David Howells <dhowells@redhat.com> writes:
>>
>>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>>
>>>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>
>>>> and also, if you like:
>>>>
>>>> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>> 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 <jpedrofonseca@ua.pt>
> 
> 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

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

[Greg KH]

On Mon, Jan 18, 2021 at 06:13:02PM +0100, Toke Høiland-Jørgensen wrote:
> David Howells <dhowells@redhat.com> writes:
> 
> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> >> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> 
> >> and also, if you like:
> >> 
> >> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >
> > 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

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

[Toke Høiland-Jørgensen]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Mon, Jan 18, 2021 at 06:13:02PM +0100, Toke Høiland-Jørgensen wrote:
>> David Howells <dhowells@redhat.com> writes:
>> 
>> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> >> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> 
>> >> and also, if you like:
>> >> 
>> >> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> > 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

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

[Greg KH]

On Mon, Mar 15, 2021 at 11:52:52AM +0100, Toke Høiland-Jørgensen wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> 
> > On Mon, Jan 18, 2021 at 06:13:02PM +0100, Toke Høiland-Jørgensen wrote:
> >> David Howells <dhowells@redhat.com> writes:
> >> 
> >> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >
> >> >> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> >> 
> >> >> and also, if you like:
> >> >> 
> >> >> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> >
> >> > 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