| Message ID | 20220627092027.20858-1-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2,RESEND] KEYS: asymmetric: enforce SM2 signature use pkey algo | expand |
On Mon, Jun 27, 2022 at 05:20: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. > > Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification") > Reported-by: Eric Biggers <ebiggers@google.com> > Cc: stable@vger.kernel.org # v5.10+ > 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 7c9e6be35c30..3f17ee860f89 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); So, why this should not validate sig->hash_alog *to be* "sm3"? I.e. add instead guard before crypto_alloc_hash: if (strncmp(sig->hash_algo, "sm3") != 0) { /* error */ } /* continue */ > 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; > -- > 2.24.3 (Apple Git-128) > BR, Jarkko
Hi Jarkko, On 6/28/22 7:14 AM, Jarkko Sakkinen wrote: > On Mon, Jun 27, 2022 at 05:20: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. >> >> Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification") >> Reported-by: Eric Biggers <ebiggers@google.com> >> Cc: stable@vger.kernel.org # v5.10+ >> 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 7c9e6be35c30..3f17ee860f89 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); > > So, why this should not validate sig->hash_alog *to be* "sm3"? > > I.e. add instead guard before crypto_alloc_hash: > > if (strncmp(sig->hash_algo, "sm3") != 0) { > /* error */ > } > /* continue */ > Thanks, it's reasonable and I'll take your advice. Best regards, Tianjia
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index 7c9e6be35c30..3f17ee860f89 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;
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. Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification") Reported-by: Eric Biggers <ebiggers@google.com> Cc: stable@vger.kernel.org # v5.10+ Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> --- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)