diff mbox series

[1/2] KEYS: asymmetric: enforce that sig algo matches key algo

Message ID 20220201003414.55380-2-ebiggers@kernel.org (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series Fix bugs in public_key_verify_signature() | expand

Commit Message

Eric Biggers Feb. 1, 2022, 12:34 a.m. UTC 
From: Eric Biggers <ebiggers@google.com>

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.

Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 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(-)

Comments

Vitaly Chikunov Feb. 2, 2022, 2:52 a.m. UTC | #1 
Eric,

On Mon, Jan 31, 2022 at 04:34:13PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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.

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.


> 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  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 0b4d07aa88111..f94a1d1ad3a6c 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 4fefb219bfdc8..aba7113d86c76 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);
>  
> +	/*
> +	 * 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).

> +	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,

> +		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 fe14cae115b51..71cc1738fbfd2 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
Eric Biggers Feb. 2, 2022, 3:10 a.m. UTC | #2 
On Wed, Feb 02, 2022 at 05:52:30AM +0300, Vitaly Chikunov wrote:
> Eric,
> 
> On Mon, Jan 31, 2022 at 04:34:13PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > 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.
> 
> 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.

> 
> 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.

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.

> > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> > index 4fefb219bfdc8..aba7113d86c76 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);
> >  
> > +	/*
> > +	 * 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.

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
Eric Biggers Feb. 2, 2022, 3:22 a.m. UTC | #3 
On Tue, Feb 01, 2022 at 07:10:33PM -0800, Eric Biggers wrote:
> > This seem incorrect too, as sig->pkey_algo could be NULL for direct
> > signature verification calls. For example, for keyctl pkey_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
Vitaly Chikunov Feb. 2, 2022, 5:20 a.m. UTC | #4 
On Tue, Feb 01, 2022 at 07:10:33PM -0800, Eric Biggers wrote:
> On Wed, Feb 02, 2022 at 05:52:30AM +0300, Vitaly Chikunov wrote:
> > Eric,
> > 
> > On Mon, Jan 31, 2022 at 04:34:13PM -0800, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > 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.
> > 
> > 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.
> 
> > 
> > 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.
> 
> 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,

> 
> > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> > > index 4fefb219bfdc8..aba7113d86c76 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);
> > >  
> > > +	/*
> > > +	 * 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.
> 
> 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
Jarkko Sakkinen Feb. 21, 2022, 1:43 a.m. UTC | #5 
On Mon, Jan 31, 2022 at 04:34:13PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  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 0b4d07aa88111..f94a1d1ad3a6c 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 4fefb219bfdc8..aba7113d86c76 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);
>  
> +	/*
> +	 * 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);
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index fe14cae115b51..71cc1738fbfd2 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
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

David, do you want to pick this?

BR, Jarkko
Eric Biggers March 4, 2022, 7:26 p.m. UTC | #6 
On Mon, Feb 21, 2022 at 02:43:21AM +0100, Jarkko Sakkinen wrote:
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> David, do you want to pick this?
> 

No response from David, so I think you need to take these.

- Eric
Jarkko Sakkinen March 5, 2022, 5:51 a.m. UTC | #7 
On Fri, Mar 04, 2022 at 11:26:10AM -0800, Eric Biggers wrote:
> On Mon, Feb 21, 2022 at 02:43:21AM +0100, Jarkko Sakkinen wrote:
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > 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
diff mbox series

Patch

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 0b4d07aa88111..f94a1d1ad3a6c 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 4fefb219bfdc8..aba7113d86c76 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);
 
+	/*
+	 * 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);
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index fe14cae115b51..71cc1738fbfd2 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) {