diff mbox series

[RFC/RFT,07/18] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

Message ID 20190331200428.26597-8-ebiggers@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto: fuzz algorithms against their generic implementation | expand

Commit Message

Eric Biggers March 31, 2019, 8:04 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

GCM instances can be created by either the "gcm" template, which only
allows choosing the block cipher, e.g. "gcm(aes)"; or by "gcm_base",
which allows choosing the ctr and ghash implementations, e.g.
"gcm_base(ctr(aes-generic),ghash-generic)".

However, a "gcm_base" instance prevents a "gcm" instance from being
registered using the same implementations.  Nor will the instance be
found by lookups of "gcm".  This can be used as a denial of service.
Moreover, "gcm_base" instances are never tested by the crypto
self-tests, even if there are compatible "gcm" tests.

The root cause of these problems is that instances of the two templates
use different cra_names.  Therefore, fix these problems by making
"gcm_base" instances set the same cra_name as "gcm" instances, e.g.
"gcm(aes)" instead of "gcm_base(ctr(aes-generic),ghash-generic)".

This requires extracting the block cipher name from the name of the ctr
algorithm, which means starting to require that the stream cipher really
be ctr and not something else.  But it would be pretty bizarre if anyone
was actually relying on being able to use a non-ctr stream cipher here.

Fixes: d00aa19b507b ("[CRYPTO] gcm: Allow block cipher parameter")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/gcm.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

Comments

Eric Biggers April 1, 2019, 3:56 p.m. UTC | #1 
On Sun, Mar 31, 2019 at 01:04:17PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> GCM instances can be created by either the "gcm" template, which only
> allows choosing the block cipher, e.g. "gcm(aes)"; or by "gcm_base",
> which allows choosing the ctr and ghash implementations, e.g.
> "gcm_base(ctr(aes-generic),ghash-generic)".
> 
> However, a "gcm_base" instance prevents a "gcm" instance from being
> registered using the same implementations.  Nor will the instance be
> found by lookups of "gcm".  This can be used as a denial of service.
> Moreover, "gcm_base" instances are never tested by the crypto
> self-tests, even if there are compatible "gcm" tests.
> 
> The root cause of these problems is that instances of the two templates
> use different cra_names.  Therefore, fix these problems by making
> "gcm_base" instances set the same cra_name as "gcm" instances, e.g.
> "gcm(aes)" instead of "gcm_base(ctr(aes-generic),ghash-generic)".
> 
> This requires extracting the block cipher name from the name of the ctr
> algorithm, which means starting to require that the stream cipher really
> be ctr and not something else.  But it would be pretty bizarre if anyone
> was actually relying on being able to use a non-ctr stream cipher here.
> 
> Fixes: d00aa19b507b ("[CRYPTO] gcm: Allow block cipher parameter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/gcm.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/crypto/gcm.c b/crypto/gcm.c
> index e1a11f529d257..12ff5ed569272 100644
> --- a/crypto/gcm.c
> +++ b/crypto/gcm.c
> @@ -597,7 +597,6 @@ static void crypto_gcm_free(struct aead_instance *inst)
>  
>  static int crypto_gcm_create_common(struct crypto_template *tmpl,
>  				    struct rtattr **tb,
> -				    const char *full_name,
>  				    const char *ctr_name,
>  				    const char *ghash_name)
>  {
> @@ -650,24 +649,23 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
>  
>  	ctr = crypto_spawn_skcipher_alg(&ctx->ctr);
>  
> -	/* We only support 16-byte blocks. */
> +	/* Must be CTR mode with 16-byte blocks. */
>  	err = -EINVAL;
> -	if (crypto_skcipher_alg_ivsize(ctr) != 16)
> +	if (strncmp(ctr->base.cra_name, "ctr(", 4) != 0 ||
> +	    crypto_skcipher_alg_ivsize(ctr) != 16)
>  		goto out_put_ctr;
>  
> -	/* Not a stream cipher? */
> -	if (ctr->base.cra_blocksize != 1)
> +	err = -ENAMETOOLONG;
> +	if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
> +		     "gcm(%s", ctr->base.cra_name + 4) >= CRYPTO_MAX_ALG_NAME)
>  		goto out_put_ctr;
>  
> -	err = -ENAMETOOLONG;
>  	if (snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
>  		     "gcm_base(%s,%s)", ctr->base.cra_driver_name,
>  		     ghash_alg->cra_driver_name) >=
>  	    CRYPTO_MAX_ALG_NAME)
>  		goto out_put_ctr;
>  
> -	memcpy(inst->alg.base.cra_name, full_name, CRYPTO_MAX_ALG_NAME);
> -
>  	inst->alg.base.cra_flags = (ghash->base.cra_flags |
>  				    ctr->base.cra_flags) & CRYPTO_ALG_ASYNC;
>  	inst->alg.base.cra_priority = (ghash->base.cra_priority +
> @@ -709,7 +707,6 @@ static int crypto_gcm_create(struct crypto_template *tmpl, struct rtattr **tb)
>  {
>  	const char *cipher_name;
>  	char ctr_name[CRYPTO_MAX_ALG_NAME];
> -	char full_name[CRYPTO_MAX_ALG_NAME];
>  
>  	cipher_name = crypto_attr_alg_name(tb[1]);
>  	if (IS_ERR(cipher_name))
> @@ -719,12 +716,7 @@ static int crypto_gcm_create(struct crypto_template *tmpl, struct rtattr **tb)
>  	    CRYPTO_MAX_ALG_NAME)
>  		return -ENAMETOOLONG;
>  
> -	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "gcm(%s)", cipher_name) >=
> -	    CRYPTO_MAX_ALG_NAME)
> -		return -ENAMETOOLONG;
> -
> -	return crypto_gcm_create_common(tmpl, tb, full_name,
> -					ctr_name, "ghash");
> +	return crypto_gcm_create_common(tmpl, tb, ctr_name, "ghash");
>  }
>  
>  static int crypto_gcm_base_create(struct crypto_template *tmpl,
> @@ -732,7 +724,6 @@ static int crypto_gcm_base_create(struct crypto_template *tmpl,
>  {
>  	const char *ctr_name;
>  	const char *ghash_name;
> -	char full_name[CRYPTO_MAX_ALG_NAME];
>  
>  	ctr_name = crypto_attr_alg_name(tb[1]);
>  	if (IS_ERR(ctr_name))
> @@ -742,12 +733,7 @@ static int crypto_gcm_base_create(struct crypto_template *tmpl,
>  	if (IS_ERR(ghash_name))
>  		return PTR_ERR(ghash_name);
>  
> -	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "gcm_base(%s,%s)",
> -		     ctr_name, ghash_name) >= CRYPTO_MAX_ALG_NAME)
> -		return -ENAMETOOLONG;
> -
> -	return crypto_gcm_create_common(tmpl, tb, full_name,
> -					ctr_name, ghash_name);
> +	return crypto_gcm_create_common(tmpl, tb, ctr_name, ghash_name);
>  }
>  
>  static int crypto_rfc4106_setkey(struct crypto_aead *parent, const u8 *key,
> -- 
> 2.21.0
> 

Oops, I think there's a bug here: we also need to check that the hash algorithm
passed to gcm_base is really "ghash".  Similarly, in the next patch, ccm_base
requires "cbcmac".

- Eric
diff mbox series

Patch

diff --git a/crypto/gcm.c b/crypto/gcm.c
index e1a11f529d257..12ff5ed569272 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -597,7 +597,6 @@  static void crypto_gcm_free(struct aead_instance *inst)
 
 static int crypto_gcm_create_common(struct crypto_template *tmpl,
 				    struct rtattr **tb,
-				    const char *full_name,
 				    const char *ctr_name,
 				    const char *ghash_name)
 {
@@ -650,24 +649,23 @@  static int crypto_gcm_create_common(struct crypto_template *tmpl,
 
 	ctr = crypto_spawn_skcipher_alg(&ctx->ctr);
 
-	/* We only support 16-byte blocks. */
+	/* Must be CTR mode with 16-byte blocks. */
 	err = -EINVAL;
-	if (crypto_skcipher_alg_ivsize(ctr) != 16)
+	if (strncmp(ctr->base.cra_name, "ctr(", 4) != 0 ||
+	    crypto_skcipher_alg_ivsize(ctr) != 16)
 		goto out_put_ctr;
 
-	/* Not a stream cipher? */
-	if (ctr->base.cra_blocksize != 1)
+	err = -ENAMETOOLONG;
+	if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
+		     "gcm(%s", ctr->base.cra_name + 4) >= CRYPTO_MAX_ALG_NAME)
 		goto out_put_ctr;
 
-	err = -ENAMETOOLONG;
 	if (snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
 		     "gcm_base(%s,%s)", ctr->base.cra_driver_name,
 		     ghash_alg->cra_driver_name) >=
 	    CRYPTO_MAX_ALG_NAME)
 		goto out_put_ctr;
 
-	memcpy(inst->alg.base.cra_name, full_name, CRYPTO_MAX_ALG_NAME);
-
 	inst->alg.base.cra_flags = (ghash->base.cra_flags |
 				    ctr->base.cra_flags) & CRYPTO_ALG_ASYNC;
 	inst->alg.base.cra_priority = (ghash->base.cra_priority +
@@ -709,7 +707,6 @@  static int crypto_gcm_create(struct crypto_template *tmpl, struct rtattr **tb)
 {
 	const char *cipher_name;
 	char ctr_name[CRYPTO_MAX_ALG_NAME];
-	char full_name[CRYPTO_MAX_ALG_NAME];
 
 	cipher_name = crypto_attr_alg_name(tb[1]);
 	if (IS_ERR(cipher_name))
@@ -719,12 +716,7 @@  static int crypto_gcm_create(struct crypto_template *tmpl, struct rtattr **tb)
 	    CRYPTO_MAX_ALG_NAME)
 		return -ENAMETOOLONG;
 
-	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "gcm(%s)", cipher_name) >=
-	    CRYPTO_MAX_ALG_NAME)
-		return -ENAMETOOLONG;
-
-	return crypto_gcm_create_common(tmpl, tb, full_name,
-					ctr_name, "ghash");
+	return crypto_gcm_create_common(tmpl, tb, ctr_name, "ghash");
 }
 
 static int crypto_gcm_base_create(struct crypto_template *tmpl,
@@ -732,7 +724,6 @@  static int crypto_gcm_base_create(struct crypto_template *tmpl,
 {
 	const char *ctr_name;
 	const char *ghash_name;
-	char full_name[CRYPTO_MAX_ALG_NAME];
 
 	ctr_name = crypto_attr_alg_name(tb[1]);
 	if (IS_ERR(ctr_name))
@@ -742,12 +733,7 @@  static int crypto_gcm_base_create(struct crypto_template *tmpl,
 	if (IS_ERR(ghash_name))
 		return PTR_ERR(ghash_name);
 
-	if (snprintf(full_name, CRYPTO_MAX_ALG_NAME, "gcm_base(%s,%s)",
-		     ctr_name, ghash_name) >= CRYPTO_MAX_ALG_NAME)
-		return -ENAMETOOLONG;
-
-	return crypto_gcm_create_common(tmpl, tb, full_name,
-					ctr_name, ghash_name);
+	return crypto_gcm_create_common(tmpl, tb, ctr_name, ghash_name);
 }
 
 static int crypto_rfc4106_setkey(struct crypto_aead *parent, const u8 *key,