On Sun, Mar 31, 2019 at 01:04:17PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebigg...@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: sta...@vger.kernel.org
> Signed-off-by: Eric Biggers <ebigg...@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

Reply via email to