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-rc3.
Changed v1 => v2:
- Changed patch 1 to continue allowing a NULL sig->pkey_algo.
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: dfd42facf1e4ada021b939b4e19c935dcdd55566
--
2.35.1
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 <[email protected]>
Cc: [email protected] # v5.10+
Signed-off-by: Tianjia Zhang <[email protected]>
---
v2:
- add Fixes tag to commit message
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;
--
2.34.1
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 (if given) matches the key's algorithm.
Also remove two checks of this done by callers, which are now redundant.
Cc: [email protected]
Tested-by: Stefan Berger <[email protected]>
Tested-by: Tianjia Zhang <[email protected]>
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(-)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 0b4d07aa8811..f94a1d1ad3a6 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -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;
}
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 4fefb219bfdc..e36213945686 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,
BUG_ON(!sig);
BUG_ON(!sig->s);
+ /*
+ * If the signature specifies a public key algorithm, it *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) {
+ 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);
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index fe14cae115b5..71cc1738fbfd 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -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) {
--
2.35.1
On Mon, Feb 07, 2022 at 09:24:47PM -0800, Eric Biggers wrote:
> 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 (if given) matches the key's algorithm.
>
> Also remove two checks of this done by callers, which are now redundant.
>
> Cc: [email protected]
> Tested-by: Stefan Berger <[email protected]>
> Tested-by: Tianjia Zhang <[email protected]>
> Signed-off-by: Eric Biggers <[email protected]>
Reviewed-by: Vitaly Chikunov <[email protected]>
Thanks,
> ---
> 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(-)
>
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
> index 0b4d07aa8811..f94a1d1ad3a6 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -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;
> }
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 4fefb219bfdc..e36213945686 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -325,6 +325,21 @@ int public_key_verify_signature(const struct public_key *pkey,
> BUG_ON(!sig);
> BUG_ON(!sig->s);
>
> + /*
> + * If the signature specifies a public key algorithm, it *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) {
> + 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);
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index fe14cae115b5..71cc1738fbfd 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -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) {
> --
> 2.35.1