diff mbox series

KEYS: asymmetric: enforce SM2 signature use pkey algo

Message ID 20220207114327.7929-1-tianjia.zhang@linux.alibaba.com (mailing list archive)
State New, archived
Headers show
Series KEYS: asymmetric: enforce SM2 signature use pkey algo | expand

Commit Message

tianjia.zhang Feb. 7, 2022, 11:43 a.m. UTC
The signature verification of SM2 needs to add the Za value and
recalculate sig->digest, which requires the detection of the pkey_algo
in public_key_verify_signature(). As Eric Biggers said, the pkey_algo
field in sig is attacker-controlled and should be use pkey->pkey_algo
instead of sig->pkey_algo, and secondly, if sig->pkey_algo is NULL, it
will also cause signature verification failure.

The software_key_determine_akcipher() already forces the algorithms
are matched, so the SM3 algorithm is enforced in the SM2 signature,
although this has been checked, we still avoid using any algorithm
information in the signature as input.

Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
 crypto/asymmetric_keys/public_key.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Eric Biggers Feb. 8, 2022, 5:35 a.m. UTC | #1
On Mon, Feb 07, 2022 at 07:43:27PM +0800, Tianjia Zhang wrote:
> The signature verification of SM2 needs to add the Za value and
> recalculate sig->digest, which requires the detection of the pkey_algo
> in public_key_verify_signature(). As Eric Biggers said, the pkey_algo
> field in sig is attacker-controlled and should be use pkey->pkey_algo
> instead of sig->pkey_algo, and secondly, if sig->pkey_algo is NULL, it
> will also cause signature verification failure.
> 
> The software_key_determine_akcipher() already forces the algorithms
> are matched, so the SM3 algorithm is enforced in the SM2 signature,
> although this has been checked, we still avoid using any algorithm
> information in the signature as input.
> 
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>

Can you add a Fixes tag?

> ---
>  crypto/asymmetric_keys/public_key.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index a603ee8afdb8..ea9a5501f87e 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -309,7 +309,8 @@ static int cert_sig_digest_update(const struct public_key_signature *sig,
>  	if (ret)
>  		return ret;
>  
> -	tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
> +	/* SM2 signatures always use the SM3 hash algorithm */
> +	tfm = crypto_alloc_shash("sm3", 0, 0);
>  	if (IS_ERR(tfm))
>  		return PTR_ERR(tfm);
>  
> @@ -414,8 +415,7 @@ int public_key_verify_signature(const struct public_key *pkey,
>  	if (ret)
>  		goto error_free_key;
>  
> -	if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 &&
> -	    sig->data_size) {
> +	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
>  		ret = cert_sig_digest_update(sig, tfm);
>  		if (ret)
>  			goto error_free_key;
> -- 

This is an improvement, but do you also have a plan to address the problem where
the code allows the "Za" hash step to be skipped?  The definitions of SM2 that I
could find require that step.  So, it is unclear that the algorithm with that
step skipped is still SM2, and how its security relates to that of the SM2
algorithm as actually defined.

- Eric
tianjia.zhang Feb. 8, 2022, 9:45 a.m. UTC | #2
Hi Eric,

On 2/8/22 1:35 PM, Eric Biggers wrote:
> On Mon, Feb 07, 2022 at 07:43:27PM +0800, Tianjia Zhang wrote:
>> The signature verification of SM2 needs to add the Za value and
>> recalculate sig->digest, which requires the detection of the pkey_algo
>> in public_key_verify_signature(). As Eric Biggers said, the pkey_algo
>> field in sig is attacker-controlled and should be use pkey->pkey_algo
>> instead of sig->pkey_algo, and secondly, if sig->pkey_algo is NULL, it
>> will also cause signature verification failure.
>>
>> The software_key_determine_akcipher() already forces the algorithms
>> are matched, so the SM3 algorithm is enforced in the SM2 signature,
>> although this has been checked, we still avoid using any algorithm
>> information in the signature as input.
>>
>> Reported-by: Eric Biggers <ebiggers@google.com>
>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> 
> Can you add a Fixes tag?
> 

Thanks, the v2 patch with Fixes tag added has been appended to your
v2 series.

>> ---
>>   crypto/asymmetric_keys/public_key.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
>> index a603ee8afdb8..ea9a5501f87e 100644
>> --- a/crypto/asymmetric_keys/public_key.c
>> +++ b/crypto/asymmetric_keys/public_key.c
>> @@ -309,7 +309,8 @@ static int cert_sig_digest_update(const struct public_key_signature *sig,
>>   	if (ret)
>>   		return ret;
>>   
>> -	tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
>> +	/* SM2 signatures always use the SM3 hash algorithm */
>> +	tfm = crypto_alloc_shash("sm3", 0, 0);
>>   	if (IS_ERR(tfm))
>>   		return PTR_ERR(tfm);
>>   
>> @@ -414,8 +415,7 @@ int public_key_verify_signature(const struct public_key *pkey,
>>   	if (ret)
>>   		goto error_free_key;
>>   
>> -	if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 &&
>> -	    sig->data_size) {
>> +	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
>>   		ret = cert_sig_digest_update(sig, tfm);
>>   		if (ret)
>>   			goto error_free_key;
>> -- 
> 
> This is an improvement, but do you also have a plan to address the problem where
> the code allows the "Za" hash step to be skipped?  The definitions of SM2 that I
> could find require that step.  So, it is unclear that the algorithm with that
> step skipped is still SM2, and how its security relates to that of the SM2
> algorithm as actually defined.
> 
> - Eric
The design of this Za has indeed brought us a lot of trouble, which
makes the two separate steps of calculating the hash and signature
forced to be coupled together. At present, it is a better way to design
skipping Za as an option. I will try to do this, which of course also
includes application layer libraries, like openssl.

Best regards,
Tianjia
Jarkko Sakkinen Feb. 21, 2022, 1:49 a.m. UTC | #3
On Mon, Feb 07, 2022 at 07:43:27PM +0800, Tianjia Zhang wrote:
> The signature verification of SM2 needs to add the Za value and
> recalculate sig->digest, which requires the detection of the pkey_algo
> in public_key_verify_signature(). As Eric Biggers said, the pkey_algo
> field in sig is attacker-controlled and should be use pkey->pkey_algo
> instead of sig->pkey_algo, and secondly, if sig->pkey_algo is NULL, it
> will also cause signature verification failure.
> 
> The software_key_determine_akcipher() already forces the algorithms
> are matched, so the SM3 algorithm is enforced in the SM2 signature,
> although this has been checked, we still avoid using any algorithm
> information in the signature as input.
> 
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> ---
>  crypto/asymmetric_keys/public_key.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index a603ee8afdb8..ea9a5501f87e 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -309,7 +309,8 @@ static int cert_sig_digest_update(const struct public_key_signature *sig,
>  	if (ret)
>  		return ret;
>  
> -	tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
> +	/* SM2 signatures always use the SM3 hash algorithm */
> +	tfm = crypto_alloc_shash("sm3", 0, 0);

Why not simply fail when sig->hash_algo != "sm3"?

BR, Jarkko
tianjia.zhang Feb. 21, 2022, 2:43 a.m. UTC | #4
Hi Jarkko,

On 2/21/22 9:49 AM, Jarkko Sakkinen wrote:
> On Mon, Feb 07, 2022 at 07:43:27PM +0800, Tianjia Zhang wrote:
>> The signature verification of SM2 needs to add the Za value and
>> recalculate sig->digest, which requires the detection of the pkey_algo
>> in public_key_verify_signature(). As Eric Biggers said, the pkey_algo
>> field in sig is attacker-controlled and should be use pkey->pkey_algo
>> instead of sig->pkey_algo, and secondly, if sig->pkey_algo is NULL, it
>> will also cause signature verification failure.
>>
>> The software_key_determine_akcipher() already forces the algorithms
>> are matched, so the SM3 algorithm is enforced in the SM2 signature,
>> although this has been checked, we still avoid using any algorithm
>> information in the signature as input.
>>
>> Reported-by: Eric Biggers <ebiggers@google.com>
>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
>> ---
>>   crypto/asymmetric_keys/public_key.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
>> index a603ee8afdb8..ea9a5501f87e 100644
>> --- a/crypto/asymmetric_keys/public_key.c
>> +++ b/crypto/asymmetric_keys/public_key.c
>> @@ -309,7 +309,8 @@ static int cert_sig_digest_update(const struct public_key_signature *sig,
>>   	if (ret)
>>   		return ret;
>>   
>> -	tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
>> +	/* SM2 signatures always use the SM3 hash algorithm */
>> +	tfm = crypto_alloc_shash("sm3", 0, 0);
> 
> Why not simply fail when sig->hash_algo != "sm3"?
> 
> BR, Jarkko

This series of Eric's patch 2/2 has done this check.

Best regards,
Tianjia
Jarkko Sakkinen Feb. 21, 2022, 8:17 p.m. UTC | #5
On Mon, Feb 21, 2022 at 10:43:39AM +0800, Tianjia Zhang wrote:
> Hi Jarkko,
> 
> On 2/21/22 9:49 AM, Jarkko Sakkinen wrote:
> > On Mon, Feb 07, 2022 at 07:43:27PM +0800, Tianjia Zhang wrote:
> > > The signature verification of SM2 needs to add the Za value and
> > > recalculate sig->digest, which requires the detection of the pkey_algo
> > > in public_key_verify_signature(). As Eric Biggers said, the pkey_algo
> > > field in sig is attacker-controlled and should be use pkey->pkey_algo
> > > instead of sig->pkey_algo, and secondly, if sig->pkey_algo is NULL, it
> > > will also cause signature verification failure.
> > > 
> > > The software_key_determine_akcipher() already forces the algorithms
> > > are matched, so the SM3 algorithm is enforced in the SM2 signature,
> > > although this has been checked, we still avoid using any algorithm
> > > information in the signature as input.
> > > 
> > > Reported-by: Eric Biggers <ebiggers@google.com>
> > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> > > ---
> > >   crypto/asymmetric_keys/public_key.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> > > index a603ee8afdb8..ea9a5501f87e 100644
> > > --- a/crypto/asymmetric_keys/public_key.c
> > > +++ b/crypto/asymmetric_keys/public_key.c
> > > @@ -309,7 +309,8 @@ static int cert_sig_digest_update(const struct public_key_signature *sig,
> > >   	if (ret)
> > >   		return ret;
> > > -	tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
> > > +	/* SM2 signatures always use the SM3 hash algorithm */
> > > +	tfm = crypto_alloc_shash("sm3", 0, 0);
> > 
> > Why not simply fail when sig->hash_algo != "sm3"?
> > 
> > BR, Jarkko
> 
> This series of Eric's patch 2/2 has done this check.
> 
> Best regards,
> Tianjia

Hmm... So how does that make this legit?

BR, Jarkko
diff mbox series

Patch

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index a603ee8afdb8..ea9a5501f87e 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -309,7 +309,8 @@  static int cert_sig_digest_update(const struct public_key_signature *sig,
 	if (ret)
 		return ret;
 
-	tfm = crypto_alloc_shash(sig->hash_algo, 0, 0);
+	/* SM2 signatures always use the SM3 hash algorithm */
+	tfm = crypto_alloc_shash("sm3", 0, 0);
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 
@@ -414,8 +415,7 @@  int public_key_verify_signature(const struct public_key *pkey,
 	if (ret)
 		goto error_free_key;
 
-	if (sig->pkey_algo && strcmp(sig->pkey_algo, "sm2") == 0 &&
-	    sig->data_size) {
+	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
 		ret = cert_sig_digest_update(sig, tfm);
 		if (ret)
 			goto error_free_key;