diff mbox series

[v2,5/7] crypto: gcm - fix incompatibility between "gcm" and "gcm_base"

Message ID 20190410064635.11813-6-ebiggers@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto: fixes in preparation of new fuzz tests | expand

Commit Message

Eric Biggers April 10, 2019, 6:46 a.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.  It also requires starting to verify that the algorithms are
really ctr and ghash, not something else entirely.  But it would be
bizarre if anyone were actually using non-gcm-compatible algorithms with
gcm_base, so this shouldn't break anyone in practice.

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

Comments

Sasha Levin April 10, 2019, 7:18 p.m. UTC | #1 
Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: d00aa19b507b [CRYPTO] gcm: Allow block cipher parameter.

The bot has tested the following trees: v5.0.7, v4.19.34, v4.14.111, v4.9.168, v4.4.178, v3.18.138.

v5.0.7: Build OK!
v4.19.34: Build OK!
v4.14.111: Build OK!
v4.9.168: Failed to apply! Possible dependencies:
    9b40f79c08e8 ("crypto: gcm - Fix error return code in crypto_gcm_create_common()")

v4.4.178: Failed to apply! Possible dependencies:
    16f37ecdd068 ("crypto: gcm - Use skcipher")
    9b40f79c08e8 ("crypto: gcm - Fix error return code in crypto_gcm_create_common()")

v3.18.138: Failed to apply! Possible dependencies:
    16f37ecdd068 ("crypto: gcm - Use skcipher")
    17db85469970 ("crypto: gcm - Use default null skcipher")
    5d72336f1b2c ("crypto: gcm - Use crypto_aead_set_reqsize helper")
    9b40f79c08e8 ("crypto: gcm - Fix error return code in crypto_gcm_create_common()")
    adcbc688fe2f ("crypto: gcm - Convert to new AEAD interface")


How should we proceed with this patch?

--
Thanks,
Sasha
Herbert Xu April 18, 2019, 2 p.m. UTC | #2 
Eric Biggers <ebiggers@kernel.org> wrote:
>
> @@ -638,7 +637,7 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
>                goto err_free_inst;
> 
>        err = -EINVAL;
> -       if (ghash->digestsize != 16)
> +       if (strcmp(ghash->base.cra_name, "ghash") != 0)
>                goto err_drop_ghash;

We should keep both tests because the self-tests can be compiled
out so there is no guarantee that something claiming to be ghash
actually is ghash.

Cheers,
Eric Biggers April 18, 2019, 6:41 p.m. UTC | #3 
On Thu, Apr 18, 2019 at 10:00:06PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > @@ -638,7 +637,7 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
> >                goto err_free_inst;
> > 
> >        err = -EINVAL;
> > -       if (ghash->digestsize != 16)
> > +       if (strcmp(ghash->base.cra_name, "ghash") != 0)
> >                goto err_drop_ghash;
> 
> We should keep both tests because the self-tests can be compiled
> out so there is no guarantee that something claiming to be ghash
> actually is ghash.
> 

I'm not necessarily opposed to doing this, but if we're assuming that untested,
arbitrarily broken algorithms may be registered with the crypto API under any
name, "ghash" could easily still be broken even if it declares a 16-byte digest
size.  Verifying the digest size is just an extra sanity check; we're really
still relying on the implementation to be correct.  (Same for the ccm patch.)

- Eric
Herbert Xu April 19, 2019, 5:52 a.m. UTC | #4 
On Thu, Apr 18, 2019 at 11:41:46AM -0700, Eric Biggers wrote:
>
> I'm not necessarily opposed to doing this, but if we're assuming that untested,
> arbitrarily broken algorithms may be registered with the crypto API under any
> name, "ghash" could easily still be broken even if it declares a 16-byte digest
> size.  Verifying the digest size is just an extra sanity check; we're really
> still relying on the implementation to be correct.  (Same for the ccm patch.)

The check is not so much for the purpose of producing the correct
output for the given algorithm, since as you correctly stated that
would rely on the correctness of the underlying algorithm.  It is
more about not crashing when the underlying algorithm has a geometry
that is completely different (e.g., by overwriting the digest
buffer).

Come to think of it, perhaps we should check the geometry of
algorithms even when self-tests are compiled out.  That way these
checks could be replaced by a simple name check.

This would be good anyway because the self-tests may indeed crash
if an algorithm with an incorrect geometry is used.

Cheers,
diff mbox series

Patch

diff --git a/crypto/gcm.c b/crypto/gcm.c
index e1a11f529d257..c7354ed1fa4d4 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)
 {
@@ -638,7 +637,7 @@  static int crypto_gcm_create_common(struct crypto_template *tmpl,
 		goto err_free_inst;
 
 	err = -EINVAL;
-	if (ghash->digestsize != 16)
+	if (strcmp(ghash->base.cra_name, "ghash") != 0)
 		goto err_drop_ghash;
 
 	crypto_set_skcipher_spawn(&ctx->ctr, aead_crypto_instance(inst));
@@ -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. */
+	/* The skcipher algorithm must be CTR mode, using 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,