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
