Fix bugs in public_key_verify_signature()
Subsystems (5)
cover Fix bugs in public_key_verify_signature()
This patchset fixes some bugs in public_key_verify_signature() where it could be tricked into using the wrong algorithm, as was discussed at
https://lore.kernel.org/linux-integrity/[email protected]/T/#t
I'd appreciate it if the people who care about each of the supported public key algorithms (RSA, ECDSA, ECRDSA, and SM2) would test this patchset to make sure it still works for their use case(s). I've tested that X.509 and PKCS#7 with RSA still work.
Note, I have *not* included a fix for SM2 being implemented incorrectly. That is another bug that I pointed out in the above thread. I think that bug is for the people who actually care about SM2.
This applies to v5.17-rc2.
Eric Biggers (2):
KEYS: asymmetric: enforce that sig algo matches key algo
KEYS: asymmetric: properly validate hash_algo and encoding
crypto/asymmetric_keys/pkcs7_verify.c | 6 -- crypto/asymmetric_keys/public_key.c | 126 ++++++++++++++++------- crypto/asymmetric_keys/x509_public_key.c | 6 -- 3 files changed, 91 insertions(+), 47 deletions(-)
base-commit: 26291c54e111ff6ba87a164d85d4a4e134b7315c
Signature · 1 line
2.35.1
PATCH 1/2 KEYS: asymmetric: enforce that sig algo matches key algo
From: Eric Biggers <[email protected]>
Most callers of public_key_verify_signature(), including most indirect callers via verify_signature() as well as pkcs7_verify_sig_chain(), don't check that public_key_signature::pkey_algo matches public_key::pkey_algo. These should always match. However, a malicious signature could intentionally declare an unintended algorithm. It is essential that such signatures be rejected outright, or that the algorithm of the *key* be used -- not the algorithm of the signature as that would allow attackers to choose the algorithm used.
Currently, public_key_verify_signature() correctly uses the key's algorithm when deciding which akcipher to allocate. That's good. However, it uses the signature's algorithm when deciding whether to do the first step of SM2, which is incorrect. Also, v4.19 and older kernels used the signature's algorithm for the entire process.
Prevent such errors by making public_key_verify_signature() enforce that the signature's algorithm matches the key's algorithm.
Also remove two checks of this done by callers, which are now redundant.
--- crypto/asymmetric_keys/pkcs7_verify.c | 6 ------ crypto/asymmetric_keys/public_key.c | 15 +++++++++++++++ crypto/asymmetric_keys/x509_public_key.c | 6 ------ 3 files changed, 15 insertions(+), 12 deletions(-)
crypto/asymmetric_keys/pkcs7_verify.c +0 -6
@@ -174,12 +174,6 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,pr_devel ("Sig %u: Found cert serial match X.509[%u]\n" , sinfo->index, certix); -if (strcmp (x509->pub->pkey_algo, sinfo->sig->pkey_algo) !=0 ) { -pr_warn ("Sig %u: X.509 algo and PKCS#7 sig algo don't match\n" , - sinfo->index); -continue ; - } - sinfo->signer = x509;return 0 ; }
crypto/asymmetric_keys/public_key.c +15 -0
@@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,BUG_ON (!sig);BUG_ON (!sig->s); +/* +* The signature's claimed public key algorithm *must* match the key's +* actual public key algorithm. +* +* Small exception: ECDSA signatures don't specify the curve, but ECDSA +* keys do. So the strings can mismatch slightly in that case: +* "ecdsa-nist-*" for the key, but "ecdsa" for the signature. +*/ +if (!sig->pkey_algo) +return -EINVAL ; +if (strcmp (pkey->pkey_algo, sig->pkey_algo) !=0 && + (strncmp (pkey->pkey_algo,"ecdsa-" ,6 ) !=0 || +strcmp (sig->pkey_algo,"ecdsa" ) !=0 )) +return -EKEYREJECTED ; + ret =software_key_determine_akcipher (sig->encoding, sig->hash_algo, pkey, alg_name);
crypto/asymmetric_keys/x509_public_key.c +0 -6
@@ -128,12 +128,6 @@ int x509_check_for_self_signed(struct x509_certificate *cert) goto out; } - ret = -EKEYREJECTED ; -if (strcmp (cert->pub->pkey_algo, cert->sig->pkey_algo) !=0 && - (strncmp (cert->pub->pkey_algo,"ecdsa-" ,6 ) !=0 || -strcmp (cert->sig->pkey_algo,"ecdsa" ) !=0 )) - goto out; - ret =public_key_verify_signature (cert->pub, cert->sig);if (ret <0 ) {if (ret == -ENOPKG ) {
Signature · 1 line
2.35.1
Re: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
Eric,
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
Most callers of
public_key_verify_signature(), including most indirect callers viaverify_signature()as well aspkcs7_verify_sig_chain(), don't check thatpublic_key_signature::pkey_algomatchespublic_key::pkey_algo. These should always match.
Why should they match?
public_key_signature is the data prepared to verify the cert's signature. The cert's signature algorithm could be different from the public key algorithm defined in the cert itself. They should match only for self-signed certs. For example, you should be able to sign RSA public key with ECDSA signature and vice versa. Or 256-bit EC-RDSA with 512-bit EC-RDSA. This check will prevent this.
Quoted 50 lines
However, a malicious
signature could intentionally declare an unintended algorithm. It is essential that such signatures be rejected outright, or that the algorithm of the *key* be used -- not the algorithm of the signature as that would allow attackers to choose the algorithm used.Currently,
public_key_verify_signature()correctly uses the key's algorithm when deciding which akcipher to allocate. That's good. However, it uses the signature's algorithm when deciding whether to do the first step of SM2, which is incorrect. Also, v4.19 and older kernels used the signature's algorithm for the entire process.Prevent such errors by making
public_key_verify_signature()enforce that the signature's algorithm matches the key's algorithm.Also remove two checks of this done by callers, which are now redundant.
Signed-off-by Eric Biggers <[email protected]>--- crypto/asymmetric_keys/pkcs7_verify.c | 6 ------ crypto/asymmetric_keys/public_key.c | 15 +++++++++++++++ crypto/asymmetric_keys/x509_public_key.c | 6 ------ 3 files changed, 15 insertions(+), 12 deletions(-)crypto/asymmetric_keys/pkcs7_verify.c +0
-6@@ -174,12 +174,6 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,pr_devel ("Sig %u: Found cert serial match X.509[%u]\n" , sinfo->index, certix); -if (strcmp (x509->pub->pkey_algo, sinfo->sig->pkey_algo) !=0 ) { -pr_warn ("Sig %u: X.509 algo and PKCS#7 sig algo don't match\n" , - sinfo->index); -continue ; - } - sinfo->signer = x509;return 0 ; }crypto/asymmetric_keys/public_key.c +0
-0@@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,BUG_ON (!sig);BUG_ON (!sig->s);
crypto/asymmetric_keys/public_key.c int public_key_verify_signature(const struct public_key *pkey, · line 337
Quoted 10 lines
crypto/asymmetric_keys/public_key.c +10
-0+/* +* The signature's claimed public key algorithm *must* match the key's +* actual public key algorithm. +* +* Small exception: ECDSA signatures don't specify the curve, but ECDSA +* keys do. So the strings can mismatch slightly in that case: +* "ecdsa-nist-*" for the key, but "ecdsa" for the signature. +*/ +if (!sig->pkey_algo) +return -EINVAL ;
This seem incorrect too, as sig->pkey_algo could be NULL for direct signature verification calls. For example, for keyctl pkey_verify.
(Side note: keyctl pkey_verify will not work for non-RSA signatures, though, due to other bug - because signature size is twice key size for them, but akcipher_alg::max_size cannot distinguish this, and max_data_size, key_size, and max_sig_size are set from it).
crypto/asymmetric_keys/public_key.c int public_key_verify_signature(const struct public_key *pkey, · line 340
Quoted 3 lines
crypto/asymmetric_keys/public_key.c +3
-0+if (strcmp (pkey->pkey_algo, sig->pkey_algo) !=0 && + (strncmp (pkey->pkey_algo,"ecdsa-" ,6 ) !=0 || +strcmp (sig->pkey_algo,"ecdsa" ) !=0 ))
This seems to be taken from x509_check_for_self_signed, that's why this should not work for non-self-signed certs.
Thanks,
Quoted 24 lines
crypto/asymmetric_keys/public_key.c +2
-0+return -EKEYREJECTED ; + ret =software_key_determine_akcipher (sig->encoding, sig->hash_algo, pkey, alg_name);crypto/asymmetric_keys/x509_public_key.c +0
-6@@ -128,12 +128,6 @@ int x509_check_for_self_signed(struct x509_certificate *cert) goto out; } - ret = -EKEYREJECTED ; -if (strcmp (cert->pub->pkey_algo, cert->sig->pkey_algo) !=0 && - (strncmp (cert->pub->pkey_algo,"ecdsa-" ,6 ) !=0 || -strcmp (cert->sig->pkey_algo,"ecdsa" ) !=0 )) - goto out; - ret =public_key_verify_signature (cert->pub, cert->sig);if (ret <0 ) {if (ret == -ENOPKG ) {Signature · 1 line
2.35.1
Re: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
Quoted 11 lines · Vitaly Chikunov
Eric,
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
Most callers of
public_key_verify_signature(), including most indirect callers viaverify_signature()as well aspkcs7_verify_sig_chain(), don't check thatpublic_key_signature::pkey_algomatchespublic_key::pkey_algo. These should always match.Why should they match?
For the reasons I explain in the rest of the commit message. To summarize: to have a valid signature verification scheme the algorithm must be fixed by the key, and not attacker-controlled.
Quoted 7 lines
public_key_signatureis the data prepared to verify the cert's signature. The cert's signature algorithm could be different from the public key algorithm defined in the cert itself. They should match only for self-signed certs. For example, you should be able to sign RSA public key with ECDSA signature and vice versa. Or 256-bit EC-RDSA with 512-bit EC-RDSA. This check will prevent this.
That has nothing to do with this patch, as this patch is only dealing with the signature. A cert's public key algorithm can be different, and that is fine.
Quoted 8 lines
Quoted 8 lines
crypto/asymmetric_keys/public_key.c +0
-0@@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,BUG_ON (!sig);BUG_ON (!sig->s);
Quoted 13 lines
crypto/asymmetric_keys/public_key.c int public_key_verify_signature(const struct public_key *pkey, · line 337
Quoted 10 lines
crypto/asymmetric_keys/public_key.c +10
-0+/* +* The signature's claimed public key algorithm *must* match the key's +* actual public key algorithm. +* +* Small exception: ECDSA signatures don't specify the curve, but ECDSA +* keys do. So the strings can mismatch slightly in that case: +* "ecdsa-nist-*" for the key, but "ecdsa" for the signature. +*/ +if (!sig->pkey_algo) +return -EINVAL ;This seem incorrect too, as
sig->pkey_algocould be NULL for direct signature verification calls. For example, for keyctlpkey_verify.
We can make it optional if some callers aren't providing it. Of course, such callers wouldn't be able to verify ECDSA signatures.
- Eric
Re: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
Quoted 5 lines · Eric Biggers
Quoted 2 lines
This seem incorrect too, as
sig->pkey_algocould be NULL for direct signature verification calls. For example, for keyctlpkey_verify.We can make it optional if some callers aren't providing it. Of course, such callers wouldn't be able to verify ECDSA signatures.
Sorry, I got that backwards. ECDSA signatures don't specify the curve, but the keys do (as I noted in a comment). So ECDSA wouldn't require sig->pkey_algo.
Since it appears that KEYCTL_PKEY_VERIFY does in fact have no way to specify a pkey_algo, I'll allow NULL pkey_algo in v2.
Note that SM2 isn't implemented correctly when sig->pkey_algo is NULL, as the following code incorrectly uses the signature's pkey_algo rather than the key's:
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;
}I'm not sure whether I should even bother fixing that, given how broken the SM2 stuff is anyway.
- Eric
Re: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
In reply to Eric BiggersQuoted 25 lines · Eric Biggers
Quoted 11 lines · Vitaly Chikunov
Eric,
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
Most callers of
public_key_verify_signature(), including most indirect callers viaverify_signature()as well aspkcs7_verify_sig_chain(), don't check thatpublic_key_signature::pkey_algomatchespublic_key::pkey_algo. These should always match.Why should they match?
For the reasons I explain in the rest of the commit message. To summarize: to have a valid signature verification scheme the algorithm must be fixed by the key, and not attacker-controlled.
Quoted 7 lines
public_key_signatureis the data prepared to verify the cert's signature. The cert's signature algorithm could be different from the public key algorithm defined in the cert itself. They should match only for self-signed certs. For example, you should be able to sign RSA public key with ECDSA signature and vice versa. Or 256-bit EC-RDSA with 512-bit EC-RDSA. This check will prevent this.
Quoted 2 lines
That has nothing to do with this patch, as this patch is only dealing with the signature. A cert's public key algorithm can be different, and that is fine.
You are right and I was mistaken about that (obscured by keyctl pkey_verify error and self-signed keys verification). Then it's all good!
I also tested these patches to work well with rsa-ecdsa and ecrdsa certificates using keyctl restrict_keyring.
Thanks,
Quoted 27 lines
Quoted 9 lines
Quoted 9 lines
crypto/asymmetric_keys/public_key.c +0
-0@@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,BUG_ON (!sig);BUG_ON (!sig->s);Quoted 13 lines
crypto/asymmetric_keys/public_key.c int public_key_verify_signature(const struct public_key *pkey, · line 337
Quoted 10 lines
crypto/asymmetric_keys/public_key.c +10
-0+/* +* The signature's claimed public key algorithm *must* match the key's +* actual public key algorithm. +* +* Small exception: ECDSA signatures don't specify the curve, but ECDSA +* keys do. So the strings can mismatch slightly in that case: +* "ecdsa-nist-*" for the key, but "ecdsa" for the signature. +*/ +if (!sig->pkey_algo) +return -EINVAL ;This seem incorrect too, as
sig->pkey_algocould be NULL for direct signature verification calls. For example, for keyctlpkey_verify.We can make it optional if some callers aren't providing it. Of course, such callers wouldn't be able to verify ECDSA signatures.
- Eric
Re: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
In reply to Eric BiggersQuoted 93 lines · Eric Biggers
From: Eric Biggers <[email protected]>
Most callers of
public_key_verify_signature(), including most indirect callers viaverify_signature()as well aspkcs7_verify_sig_chain(), don't check thatpublic_key_signature::pkey_algomatchespublic_key::pkey_algo. These should always match. However, a malicious signature could intentionally declare an unintended algorithm. It is essential that such signatures be rejected outright, or that the algorithm of the *key* be used -- not the algorithm of the signature as that would allow attackers to choose the algorithm used.Currently,
public_key_verify_signature()correctly uses the key's algorithm when deciding which akcipher to allocate. That's good. However, it uses the signature's algorithm when deciding whether to do the first step of SM2, which is incorrect. Also, v4.19 and older kernels used the signature's algorithm for the entire process.Prevent such errors by making
public_key_verify_signature()enforce that the signature's algorithm matches the key's algorithm.Also remove two checks of this done by callers, which are now redundant.
Signed-off-by Eric Biggers <[email protected]>--- crypto/asymmetric_keys/pkcs7_verify.c | 6 ------ crypto/asymmetric_keys/public_key.c | 15 +++++++++++++++ crypto/asymmetric_keys/x509_public_key.c | 6 ------ 3 files changed, 15 insertions(+), 12 deletions(-)crypto/asymmetric_keys/pkcs7_verify.c +0
-6@@ -174,12 +174,6 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,pr_devel ("Sig %u: Found cert serial match X.509[%u]\n" , sinfo->index, certix); -if (strcmp (x509->pub->pkey_algo, sinfo->sig->pkey_algo) !=0 ) { -pr_warn ("Sig %u: X.509 algo and PKCS#7 sig algo don't match\n" , - sinfo->index); -continue ; - } - sinfo->signer = x509;return 0 ; }crypto/asymmetric_keys/public_key.c +15
-0@@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,BUG_ON (!sig);BUG_ON (!sig->s); +/* +* The signature's claimed public key algorithm *must* match the key's +* actual public key algorithm. +* +* Small exception: ECDSA signatures don't specify the curve, but ECDSA +* keys do. So the strings can mismatch slightly in that case: +* "ecdsa-nist-*" for the key, but "ecdsa" for the signature. +*/ +if (!sig->pkey_algo) +return -EINVAL ; +if (strcmp (pkey->pkey_algo, sig->pkey_algo) !=0 && + (strncmp (pkey->pkey_algo,"ecdsa-" ,6 ) !=0 || +strcmp (sig->pkey_algo,"ecdsa" ) !=0 )) +return -EKEYREJECTED ; + ret =software_key_determine_akcipher (sig->encoding, sig->hash_algo, pkey, alg_name);crypto/asymmetric_keys/x509_public_key.c +0
-6@@ -128,12 +128,6 @@ int x509_check_for_self_signed(struct x509_certificate *cert) goto out; } - ret = -EKEYREJECTED ; -if (strcmp (cert->pub->pkey_algo, cert->sig->pkey_algo) !=0 && - (strncmp (cert->pub->pkey_algo,"ecdsa-" ,6 ) !=0 || -strcmp (cert->sig->pkey_algo,"ecdsa" ) !=0 )) - goto out; - ret =public_key_verify_signature (cert->pub, cert->sig);if (ret <0 ) {if (ret == -ENOPKG ) {Signature · 1 line
2.35.1
David, do you want to pick this?
BR, Jarkko
Re: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
Quoted 5 lines · Jarkko Sakkinen
Reviewed-by Jarkko Sakkinen <[email protected]>David, do you want to pick this?
No response from David, so I think you need to take these.
- Eric
Jarkko Sakkinen
maintainerRe: [PATCH 1/2] KEYS: asymmetric: enforce that sig algo matches key algo
Quoted 10 lines · Eric Biggers
Quoted 5 lines · Jarkko Sakkinen
Reviewed-by Jarkko Sakkinen <[email protected]>David, do you want to pick this?
No response from David, so I think you need to take these.
- Eric
I did, they're applied now, thank you.
BR, Jarkko
PATCH 2/2 KEYS: asymmetric: properly validate hash_algo and encoding
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
It is insecure to allow arbitrary hash algorithms and signature encodings to be used with arbitrary signature algorithms. Notably, ECDSA, ECRDSA, and SM2 all sign/verify raw hash values and don't disambiguate between different hash algorithms like RSA PKCS#1 v1.5 padding does. Therefore, they need to be restricted to certain sets of hash algorithms (ideally just one, but in practice small sets are used). Additionally, the encoding is an integral part of modern signature algorithms, and is not supposed to vary.
Therefore, tighten the checks of hash_algo and encoding done by software_key_determine_akcipher().
Also rearrange the parameters to software_key_determine_akcipher() to put the public_key first, as this is the most important parameter and it often determines everything else.
--- crypto/asymmetric_keys/public_key.c | 111 +++++++++++++++++++--------- 1 file changed, 76 insertions(+), 35 deletions(-)
crypto/asymmetric_keys/public_key.c +76 -35
@@ -60,39 +60,83 @@ static void public_key_destroy(void *payload0, void *payload3) }/* -* Determine the crypto algorithm name. +* Given a public_key, and an encoding and hash_algo to be used for signing +* and/or verification with that key, determine the name of the corresponding +* akcipher algorithm. Also check that encoding and hash_algo are allowed. */ -static -int software_key_determine_akcipher (const char *encoding, -const char *hash_algo, -const struct public_key *pkey, -char alg_name[CRYPTO_MAX_ALG_NAME ]) +static int +software_key_determine_akcipher (const struct public_key *pkey, +const char *encoding,const char *hash_algo, +char alg_name[CRYPTO_MAX_ALG_NAME ]) {int n; -if (strcmp (encoding,"pkcs1" ) ==0 ) { -/* The data wangled by the RSA algorithm is typically padded -* and encoded in some manner, such as EMSA-PKCS1-1_5 [RFC3447 -* sec 8.2]. +if (!encoding) +return -EINVAL ; + +if (strcmp (pkey->pkey_algo,"rsa" ) ==0 ) { +/* +* RSA signatures usually use EMSA-PKCS1-1_5 [RFC3447 sec 8.2]. +*/ +if (strcmp (encoding,"pkcs1" ) ==0 ) { +if (!hash_algo) + n =snprintf (alg_name,CRYPTO_MAX_ALG_NAME , +"pkcs1pad(%s)" , + pkey->pkey_algo); +else + n =snprintf (alg_name,CRYPTO_MAX_ALG_NAME , +"pkcs1pad(%s,%s)" , + pkey->pkey_algo, hash_algo); +return n >=CRYPTO_MAX_ALG_NAME ? -EINVAL :0 ; + } +if (strcmp (encoding,"raw" ) !=0 ) +return -EINVAL ; +/* +* Raw RSA cannot differentiate between different hash +* algorithms. +*/ +if (hash_algo) +return -EINVAL ; + }else if (strncmp (pkey->pkey_algo,"ecdsa" ,5 ) ==0 ) { +if (strcmp (encoding,"x962" ) !=0 ) +return -EINVAL ; +/* +* ECDSA signatures are taken over a raw hash, so they don't +* differentiate between different hash algorithms. That means +* that the verifier should hard-code a specific hash algorithm. +* Unfortunately, in practice ECDSA is used with multiple SHAs, +* so we have to allow all of them and not just one. */ if (!hash_algo) - n =snprintf (alg_name,CRYPTO_MAX_ALG_NAME , -"pkcs1pad(%s)" , - pkey->pkey_algo); -else - n =snprintf (alg_name,CRYPTO_MAX_ALG_NAME , -"pkcs1pad(%s,%s)" , - pkey->pkey_algo, hash_algo); -return n >=CRYPTO_MAX_ALG_NAME ? -EINVAL :0 ; - } - -if (strcmp (encoding,"raw" ) ==0 || -strcmp (encoding,"x962" ) ==0 ) { -strcpy (alg_name, pkey->pkey_algo); -return 0 ; +return -EINVAL ; +if (strcmp (hash_algo,"sha1" ) !=0 && +strcmp (hash_algo,"sha224" ) !=0 && +strcmp (hash_algo,"sha256" ) !=0 && +strcmp (hash_algo,"sha384" ) !=0 && +strcmp (hash_algo,"sha512" ) !=0 ) +return -EINVAL ; + }else if (strcmp (pkey->pkey_algo,"sm2" ) ==0 ) { +if (strcmp (encoding,"raw" ) !=0 ) +return -EINVAL ; +if (!hash_algo) +return -EINVAL ; +if (strcmp (hash_algo,"sm3" ) !=0 ) +return -EINVAL ; + }else if (strcmp (pkey->pkey_algo,"ecrdsa" ) ==0 ) { +if (strcmp (encoding,"raw" ) !=0 ) +return -EINVAL ; +if (!hash_algo) +return -EINVAL ; +if (strcmp (hash_algo,"streebog256" ) !=0 && +strcmp (hash_algo,"streebog512" ) !=0 ) +return -EINVAL ; + }else { +/* Unknown public key algorithm */ +return -ENOPKG ; } - -return -ENOPKG ; +if (strscpy (alg_name, pkey->pkey_algo,CRYPTO_MAX_ALG_NAME ) <0 ) +return -EINVAL ; +return 0 ; }static u8 *pkey_pack_u32 (u8 *dst,u32 val)
@@ -113,9 +157,8 @@ static int software_key_query(const struct kernel_pkey_params *params,u8 *key, *ptr;int ret, len; - ret =software_key_determine_akcipher (params->encoding, - params->hash_algo, - pkey, alg_name); + ret =software_key_determine_akcipher (pkey, params->encoding, + params->hash_algo, alg_name);if (ret <0 )return ret;
@@ -179,9 +222,8 @@ static int software_key_eds_op(struct kernel_pkey_params *params,pr_devel ("==>%s()\n" , __func__); - ret =software_key_determine_akcipher (params->encoding, - params->hash_algo, - pkey, alg_name); + ret =software_key_determine_akcipher (pkey, params->encoding, + params->hash_algo, alg_name);if (ret <0 )return ret;
@@ -340,9 +382,8 @@ int public_key_verify_signature(const struct public_key *pkey, strcmp(sig->pkey_algo, "ecdsa") != 0))return -EKEYREJECTED ; - ret =software_key_determine_akcipher (sig->encoding, - sig->hash_algo, - pkey, alg_name); + ret =software_key_determine_akcipher (pkey, sig->encoding, + sig->hash_algo, alg_name);if (ret <0 )return ret;
Signature · 1 line
2.35.1
Jarkko Sakkinen
maintainerRe: [PATCH 2/2] KEYS: asymmetric: properly validate hash_algo and encoding
Quoted 39 lines · Eric Biggers
From: Eric Biggers <[email protected]>
It is insecure to allow arbitrary hash algorithms and signature encodings to be used with arbitrary signature algorithms. Notably, ECDSA, ECRDSA, and SM2 all sign/verify raw hash values and don't disambiguate between different hash algorithms like RSA PKCS#1 v1.5 padding does. Therefore, they need to be restricted to certain sets of hash algorithms (ideally just one, but in practice small sets are used). Additionally, the encoding is an integral part of modern signature algorithms, and is not supposed to vary.
Therefore, tighten the checks of
hash_algoand encoding done bysoftware_key_determine_akcipher().Also rearrange the parameters to
software_key_determine_akcipher()to put thepublic_keyfirst, as this is the most important parameter and it often determines everything else.Fixes 299f561a6693 ("x509: Add support for parsing x509 certs with ECDSA keys")Fixes 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")Fixes 0d7a78643f69 ("crypto: ecrdsa - add EC-RDSA (GOST 34.10) algorithm")Signed-off-by Eric Biggers <[email protected]>--- crypto/asymmetric_keys/public_key.c | 111 +++++++++++++++++++--------- 1 file changed, 76 insertions(+), 35 deletions(-)crypto/asymmetric_keys/public_key.c +3
-1@@ -60,39 +60,83 @@ static void public_key_destroy(void *payload0, void *payload3) }/* -* Determine the crypto algorithm name. +* Given a public_key, and an encoding and hash_algo to be used for signing +* and/or verification with that key, determine the name of the corresponding +* akcipher algorithm. Also check that encoding and hash_algo are allowed.
crypto/asymmetric_keys/public_key.c static void public_key_destroy(void *payload0, void *payload3) · line 70
Quoted 10 lines
crypto/asymmetric_keys/public_key.c +4
-5*/ -static -int software_key_determine_akcipher (const char *encoding, -const char *hash_algo, -const struct public_key *pkey, -char alg_name[CRYPTO_MAX_ALG_NAME ]) +static int +software_key_determine_akcipher (const struct public_key *pkey, +const char *encoding,const char *hash_algo, +char alg_name[CRYPTO_MAX_ALG_NAME ])
Why is changing parameter order necessary?
BR, Jarkko
Re: [PATCH 2/2] KEYS: asymmetric: properly validate hash_algo and encoding
Quoted 44 lines · Jarkko Sakkinen
Quoted 43 lines · Eric Biggers
From: Eric Biggers <[email protected]>
It is insecure to allow arbitrary hash algorithms and signature encodings to be used with arbitrary signature algorithms. Notably, ECDSA, ECRDSA, and SM2 all sign/verify raw hash values and don't disambiguate between different hash algorithms like RSA PKCS#1 v1.5 padding does. Therefore, they need to be restricted to certain sets of hash algorithms (ideally just one, but in practice small sets are used). Additionally, the encoding is an integral part of modern signature algorithms, and is not supposed to vary.
Therefore, tighten the checks of
hash_algoand encoding done bysoftware_key_determine_akcipher().Also rearrange the parameters to
software_key_determine_akcipher()to put thepublic_keyfirst, as this is the most important parameter and it often determines everything else.Fixes 299f561a6693 ("x509: Add support for parsing x509 certs with ECDSA keys")Fixes 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")Fixes 0d7a78643f69 ("crypto: ecrdsa - add EC-RDSA (GOST 34.10) algorithm")Signed-off-by Eric Biggers <[email protected]>--- crypto/asymmetric_keys/public_key.c | 111 +++++++++++++++++++--------- 1 file changed, 76 insertions(+), 35 deletions(-)crypto/asymmetric_keys/public_key.c +3
-4@@ -60,39 +60,83 @@ static void public_key_destroy(void *payload0, void *payload3) }/* -* Determine the crypto algorithm name. +* Given a public_key, and an encoding and hash_algo to be used for signing +* and/or verification with that key, determine the name of the corresponding +* akcipher algorithm. Also check that encoding and hash_algo are allowed. */ -static -int software_key_determine_akcipher(const char *encoding, - const char *hash_algo,
Quoted 9 lines
crypto/asymmetric_keys/public_key.c static void public_key_destroy(void *payload0, void *payload3) · line 70
Quoted 6 lines
crypto/asymmetric_keys/public_key.c +4
-2-const struct public_key *pkey, - char alg_name[CRYPTO_MAX_ALG_NAME]) +static int +software_key_determine_akcipher (const struct public_key *pkey, +const char *encoding,const char *hash_algo, +char alg_name[CRYPTO_MAX_ALG_NAME ])Why is changing parameter order necessary?
It's mentioned in the commit message. It's obviously not necessary but this way makes much more sense IMO.
- Eric
Re: [PATCH 2/2] KEYS: asymmetric: properly validate hash_algo and encoding
Quoted 55 lines · Eric Biggers
Quoted 44 lines · Jarkko Sakkinen
Quoted 43 lines · Eric Biggers
From: Eric Biggers <[email protected]>
It is insecure to allow arbitrary hash algorithms and signature encodings to be used with arbitrary signature algorithms. Notably, ECDSA, ECRDSA, and SM2 all sign/verify raw hash values and don't disambiguate between different hash algorithms like RSA PKCS#1 v1.5 padding does. Therefore, they need to be restricted to certain sets of hash algorithms (ideally just one, but in practice small sets are used). Additionally, the encoding is an integral part of modern signature algorithms, and is not supposed to vary.
Therefore, tighten the checks of
hash_algoand encoding done bysoftware_key_determine_akcipher().Also rearrange the parameters to
software_key_determine_akcipher()to put thepublic_keyfirst, as this is the most important parameter and it often determines everything else.Fixes 299f561a6693 ("x509: Add support for parsing x509 certs with ECDSA keys")Fixes 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")Fixes 0d7a78643f69 ("crypto: ecrdsa - add EC-RDSA (GOST 34.10) algorithm")Signed-off-by Eric Biggers <[email protected]>--- crypto/asymmetric_keys/public_key.c | 111 +++++++++++++++++++--------- 1 file changed, 76 insertions(+), 35 deletions(-)crypto/asymmetric_keys/public_key.c +3
-4@@ -60,39 +60,83 @@ static void public_key_destroy(void *payload0, void *payload3) }/* -* Determine the crypto algorithm name. +* Given a public_key, and an encoding and hash_algo to be used for signing +* and/or verification with that key, determine the name of the corresponding +* akcipher algorithm. Also check that encoding and hash_algo are allowed. */ -static -int software_key_determine_akcipher(const char *encoding, - const char *hash_algo,Quoted 9 lines
crypto/asymmetric_keys/public_key.c static void public_key_destroy(void *payload0, void *payload3) · line 70
Quoted 6 lines
crypto/asymmetric_keys/public_key.c +4
-2-const struct public_key *pkey, - char alg_name[CRYPTO_MAX_ALG_NAME]) +static int +software_key_determine_akcipher (const struct public_key *pkey, +const char *encoding,const char *hash_algo, +char alg_name[CRYPTO_MAX_ALG_NAME ])Why is changing parameter order necessary?
Quoted 2 lines
It's mentioned in the commit message. It's obviously not necessary but this way makes much more sense IMO.
Ah, so it is.
BR, Jarkko
Quoted 8 lines · Eric Biggers
This patchset fixes some bugs in
public_key_verify_signature()where it could be tricked into using the wrong algorithm, as was discussed at
https://lore.kernel.org/linux-integrity/[email protected]/T/#tI'd appreciate it if the people who care about each of the supported public key algorithms (RSA, ECDSA, ECRDSA, and SM2) would test this patchset to make sure it still works for their use case(s). I've tested that X.509 and PKCS#7 with RSA still work.
I have tested that self-ECDSA-signed x.509 certs can still be loaded and ECDSA-signed files are still verified by IMA. It works for NIST P256 and P384.
Quoted 18 lines
Note, I have *not* included a fix for SM2 being implemented incorrectly. That is another bug that I pointed out in the above thread. I think that bug is for the people who actually care about SM2.
This applies to v5.17-rc2.
Eric Biggers (2):
KEYS: asymmetric: enforce that sig algo matches key algo
KEYS: asymmetric: properly validatehash_algoand encodingcrypto/asymmetric_keys/pkcs7_verify.c | 6 -- crypto/asymmetric_keys/public_key.c | 126 ++++++++++++++++------- crypto/asymmetric_keys/x509_public_key.c | 6 -- 3 files changed, 91 insertions(+), 47 deletions(-)base-commit:
26291c54e111ff6ba87a164d85d4a4e134b7315c
Hi Eric,
Quoted 9 lines · Eric Biggers
This patchset fixes some bugs in
public_key_verify_signature()where it could be tricked into using the wrong algorithm, as was discussed at
https://lore.kernel.org/linux-integrity/[email protected]/T/#tI'd appreciate it if the people who care about each of the supported public key algorithms (RSA, ECDSA, ECRDSA, and SM2) would test this patchset to make sure it still works for their use case(s). I've tested that X.509 and PKCS#7 with RSA still work.
Quoted 6 lines
Note, I have *not* included a fix for SM2 being implemented incorrectly. That is another bug that I pointed out in the above thread. I think that bug is for the people who actually care about SM2.
This applies to v5.17-rc2.
Sorry for the late reply, thanks for your work.
I did the test and the x509 certificate for SM2-with-SM3 is working fine.
Regarding the algorithm information in the signature data used by SM2, I will add a patch to fix this issue, thanks for pointing it out.
Best regards,
Tianjia
Quoted 11 lines
Eric Biggers (2):
KEYS: asymmetric: enforce that sig algo matches key algo
KEYS: asymmetric: properly validatehash_algoand encodingcrypto/asymmetric_keys/pkcs7_verify.c | 6 -- crypto/asymmetric_keys/public_key.c | 126 ++++++++++++++++------- crypto/asymmetric_keys/x509_public_key.c | 6 -- 3 files changed, 91 insertions(+), 47 deletions(-)base-commit:
26291c54e111ff6ba87a164d85d4a4e134b7315c
PATCH 1/1 KEYS: asymmetric: enforce SM2 signature use pkey algo
In reply to Eric BiggersThe 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.
--- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
crypto/asymmetric_keys/public_key.c +3 -3
@@ -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;
Signature · 1 line
2.34.1
Re: [PATCH] KEYS: asymmetric: enforce SM2 signature use pkey algo
Quoted 7 lines · Tianjia Zhang
The signature verification of SM2 needs to add the Za value and recalculate
sig->digest, which requires the detection of thepkey_algoinpublic_key_verify_signature(). As Eric Biggers said, thepkey_algofield in sig is attacker-controlled and should be usepkey->pkey_algoinstead ofsig->pkey_algo, and secondly, ifsig->pkey_algois NULL, it will also cause signature verification failure.
Quoted 7 lines
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 <[email protected]>Signed-off-by Tianjia Zhang <[email protected]>
Can you add a Fixes tag?
Quoted 20 lines
--- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)crypto/asymmetric_keys/public_key.c +2
-1@@ -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);
crypto/asymmetric_keys/public_key.c int public_key_verify_signature(const struct public_key *pkey, · line 421
Quoted 9 lines
crypto/asymmetric_keys/public_key.c +1
-2@@ -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
Re: [PATCH] KEYS: asymmetric: enforce SM2 signature use pkey algo
Hi Eric,
Quoted 13 lines · Eric Biggers
Quoted 12 lines · Tianjia Zhang
The signature verification of SM2 needs to add the Za value and recalculate
sig->digest, which requires the detection of thepkey_algoinpublic_key_verify_signature(). As Eric Biggers said, thepkey_algofield in sig is attacker-controlled and should be usepkey->pkey_algoinstead ofsig->pkey_algo, and secondly, ifsig->pkey_algois 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.
Quoted 5 lines
Quoted 2 lines
Reported-by Eric Biggers <[email protected]>Signed-off-by Tianjia Zhang <[email protected]>Can you add a Fixes tag?
Thanks, the v2 patch with Fixes tag added has been appended to your v2 series.
Quoted 36 lines
Quoted 20 lines
--- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)crypto/asymmetric_keys/public_key.c +2
-1@@ -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);crypto/asymmetric_keys/public_key.c int public_key_verify_signature(const struct public_key *pkey, · line 421
Quoted 9 lines
crypto/asymmetric_keys/public_key.c +1
-2@@ -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.
Quoted 1 line
- 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
maintainerRe: [PATCH] KEYS: asymmetric: enforce SM2 signature use pkey algo
In reply to Tianjia ZhangQuoted 23 lines · Tianjia Zhang
The signature verification of SM2 needs to add the Za value and recalculate
sig->digest, which requires the detection of thepkey_algoinpublic_key_verify_signature(). As Eric Biggers said, thepkey_algofield in sig is attacker-controlled and should be usepkey->pkey_algoinstead ofsig->pkey_algo, and secondly, ifsig->pkey_algois 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 <[email protected]>Signed-off-by Tianjia Zhang <[email protected]>--- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)crypto/asymmetric_keys/public_key.c +0
-0
crypto/asymmetric_keys/public_key.c static int cert_sig_digest_update(const struct public_key_signature *sig, · line 313
Quoted 6 lines
crypto/asymmetric_keys/public_key.c +2
-1@@ -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
Re: [PATCH] KEYS: asymmetric: enforce SM2 signature use pkey algo
Hi Jarkko,
Quoted 27 lines · Jarkko Sakkinen
Quoted 26 lines · Tianjia Zhang
The signature verification of SM2 needs to add the Za value and recalculate
sig->digest, which requires the detection of thepkey_algoinpublic_key_verify_signature(). As Eric Biggers said, thepkey_algofield in sig is attacker-controlled and should be usepkey->pkey_algoinstead ofsig->pkey_algo, and secondly, ifsig->pkey_algois 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 <[email protected]>Signed-off-by Tianjia Zhang <[email protected]>--- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)crypto/asymmetric_keys/public_key.c +0
-0@@ -309,7 +309,8 @@ static int cert_sig_digest_update(const struct public_key_signature *sig,if (ret)return ret;
Quoted 7 lines
crypto/asymmetric_keys/public_key.c static int cert_sig_digest_update(const struct public_key_signature *sig, · line 313
Quoted 3 lines
crypto/asymmetric_keys/public_key.c +2
-1- 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
maintainerRe: [PATCH] KEYS: asymmetric: enforce SM2 signature use pkey algo
Quoted 35 lines · Tianjia Zhang
Hi Jarkko,
Quoted 32 lines · Jarkko Sakkinen
Quoted 23 lines · Tianjia Zhang
The signature verification of SM2 needs to add the Za value and recalculate
sig->digest, which requires the detection of thepkey_algoinpublic_key_verify_signature(). As Eric Biggers said, thepkey_algofield in sig is attacker-controlled and should be usepkey->pkey_algoinstead ofsig->pkey_algo, and secondly, ifsig->pkey_algois 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 <[email protected]>Signed-off-by Tianjia Zhang <[email protected]>--- crypto/asymmetric_keys/public_key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)crypto/asymmetric_keys/public_key.c +0
-0crypto/asymmetric_keys/public_key.c static int cert_sig_digest_update(const struct public_key_signature *sig, · line 312
Quoted 5 lines
crypto/asymmetric_keys/public_key.c +2
-1@@ -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"?
Quoted 6 lines
Quoted 1 line
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