diff mbox series

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

Message ID 20210112161044.3101-1-toke@redhat.com (mailing list archive)
State New
Headers show
Series crypto: public_key: check that pkey_algo is non-NULL before passing it to strcmp() | expand

Commit Message

Toke Høiland-Jørgensen Jan. 12, 2021, 4:10 p.m. UTC
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(-)

Comments

Tianjia Zhang Jan. 13, 2021, 2:39 a.m. UTC | #1
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;
>
David Howells Jan. 13, 2021, 11:11 a.m. UTC | #2
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;
Toke Høiland-Jørgensen Jan. 13, 2021, 11:29 a.m. UTC | #3
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
Toke Høiland-Jørgensen Jan. 13, 2021, 11:36 a.m. UTC | #4
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>
David Howells Jan. 13, 2021, 12:57 p.m. UTC | #5
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
Jarkko Sakkinen Jan. 14, 2021, 2:55 a.m. UTC | #6
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;
> 
>
Toke Høiland-Jørgensen Jan. 18, 2021, 5:13 p.m. UTC | #7
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
João Fonseca Jan. 18, 2021, 9:09 p.m. UTC | #8
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
Greg Kroah-Hartman March 10, 2021, 12:02 p.m. UTC | #9
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
Toke Høiland-Jørgensen March 15, 2021, 10:52 a.m. UTC | #10
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
Greg Kroah-Hartman March 15, 2021, 12:07 p.m. UTC | #11
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
diff mbox series

Patch

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;