Skip to contentSkip to conversation

crypto: prevent unkeyed use of keyed algorithms

RFC PATCH In review Eric Biggers 2 participants 11 messages Jan 3, 2018 – Jan 12, 2018

Subsystems (10)
Arm (32-bit) x86 PowerPC Staging +6 more subsystems
lore
Share & tools
lore

Eric Biggers

author
#0 lore raw

cover crypto: prevent unkeyed use of keyed algorithms

From: Eric Biggers <[email protected]>

This series updates the crypto API to consistently prevent using keyed algorithms without setting the key. Currently this is prevented for AF_ALG but not for other crypto API users, which is very problematic for other places in the kernel where userspace can specify a hash algorithm by name, e.g. KEYCTL_DH_COMPUTE as demonstrated by syzbot
(https://marc.info/?l=linux-crypto-vger&m=151395810921850).

This series fixes the bug for all users by adding a flag CRYPTO_ALG_NEED_KEY to crypto_tfm.crt_flags. This flag is set if needed when the tfm is created, is cleared when the key is set, and is checked when doing an operation that would require the key.

Patches 1-6 update the hash API, which is the primary fix. I've marked all those patches for stable, which is kind of a pain, but it seems the alternative would be very messy -- we'd have to patch at least 5 different crypto API users (probably missing some), then revert those patches upstream once we have the proper fix at the API level.

The last two patches also extend the fix to the skcipher and AEAD APIs, primarily as a sanity check since users should be less likely to try to use skciphers or AEADs without setting a key.

Eric Biggers (9):
crypto: hash - introduce crypto_hash_alg_has_setkey()
crypto: cryptd - pass through absence of ->setkey()
crypto: mcryptd - pass through absence of ->setkey()
crypto: poly1305 - remove ->setkey() method
crypto: hash - annotate algorithms taking optional key
crypto: hash - prevent using keyed hashes without setting key
crypto: ghash - remove checks for key being set
crypto: skcipher - prevent using skciphers without setting key
crypto: aead - prevent using AEADs without setting key

 arch/arm/crypto/crc32-ce-glue.c                    |  2 +
 arch/arm64/crypto/crc32-ce-glue.c                  |  2 +
 arch/powerpc/crypto/crc32c-vpmsum_glue.c           |  1 +
 arch/s390/crypto/crc32-vx.c                        |  3 ++
 arch/sparc/crypto/crc32c_glue.c                    |  1 +
 arch/x86/crypto/crc32-pclmul_glue.c                |  1 +
 arch/x86/crypto/crc32c-intel_glue.c                |  1 +
 arch/x86/crypto/poly1305_glue.c                    |  1 -
 crypto/aead.c                                      | 13 ++++-
 crypto/ahash.c                                     | 33 ++++++++++--
 crypto/algif_aead.c                                | 11 ++--
 crypto/algif_hash.c                                | 52 ++++---------------
 crypto/algif_skcipher.c                            | 59 +++++-----------------
 crypto/crc32_generic.c                             |  1 +
 crypto/crc32c_generic.c                            |  1 +
 crypto/cryptd.c                                    | 10 ++--
 crypto/ghash-generic.c                             |  6 ---
 crypto/mcryptd.c                                   | 10 ++--
 crypto/poly1305_generic.c                          | 17 ++-----
 crypto/shash.c                                     | 25 +++++++--
 crypto/skcipher.c                                  | 30 +++++++++--
 drivers/crypto/bfin_crc.c                          |  3 +-
 drivers/crypto/stm32/stm32_crc32.c                 |  2 +
 .../lustre/lnet/libcfs/linux/linux-crypto-adler.c  |  1 +
 include/crypto/aead.h                              | 10 +++-
 include/crypto/hash.h                              | 34 +++++++++----
 include/crypto/internal/hash.h                     |  2 +
 include/crypto/poly1305.h                          |  2 -
 include/crypto/skcipher.h                          | 11 ++--
 include/linux/crypto.h                             |  8 +++
 30 files changed, 195 insertions(+), 158 deletions(-)
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#1 lore raw

RFC PATCH 1/9 crypto: hash - introduce crypto_hash_alg_has_setkey()

From: Eric Biggers <[email protected]>

Templates that use an shash spawn can use crypto_shash_alg_has_setkey() to determine whether the underlying algorithm requires a key or not. But there was no corresponding function for ahash spawns. Add it.

Note that the new function actually has to support both shash and ahash algorithms, since the ahash API can be used with either.

Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 crypto/ahash.c                 | 11 +++++++++++
 include/crypto/internal/hash.h |  2 ++
 2 files changed, 13 insertions(+)
crypto/ahash.c +11 -0
index 3a35d67de7d9..d2c8895bb2fe 100644
@@ -649,5 +649,16 @@ struct hash_alg_common *ahash_attr_alg(struct rtattr *rta, u32 type, u32 mask)
 }
 EXPORT_SYMBOL_GPL(ahash_attr_alg);
 
+bool crypto_hash_alg_has_setkey(struct hash_alg_common *halg)
+{
+	struct crypto_alg *alg = &halg->base;
+
+	if (alg->cra_type != &crypto_ahash_type)
+		return crypto_shash_alg_has_setkey(__crypto_shash_alg(alg));
+
+	return __crypto_ahash_alg(alg)->setkey != NULL;
+}
+EXPORT_SYMBOL_GPL(crypto_hash_alg_has_setkey);
+
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Asynchronous cryptographic hash type");
include/crypto/internal/hash.h +2 -0
index c2bae8da642c..27040a46d50a 100644
@@ -90,6 +90,8 @@ static inline bool crypto_shash_alg_has_setkey(struct shash_alg *alg)
 	return alg->setkey != shash_no_setkey;
 }
 
+bool crypto_hash_alg_has_setkey(struct hash_alg_common *halg);
+
 int crypto_init_ahash_spawn(struct crypto_ahash_spawn *spawn,
 			    struct hash_alg_common *alg,
 			    struct crypto_instance *inst);
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#2 lore raw

RFC PATCH 2/9 crypto: cryptd - pass through absence of ->setkey()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

When the cryptd template is used to wrap an unkeyed hash algorithm, don't install a ->setkey() method to the cryptd instance. This change is necessary for cryptd to keep working with unkeyed hash algorithms once we start enforcing that ->setkey() is called when present.

Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 crypto/cryptd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
crypto/cryptd.c +2 -1
index 552e3a86e829..457ae3e66a41 100644
@@ -914,7 +914,8 @@ static int cryptd_create_hash(struct crypto_template *tmpl, struct rtattr **tb,
 	inst->alg.finup  = cryptd_hash_finup_enqueue;
 	inst->alg.export = cryptd_hash_export;
 	inst->alg.import = cryptd_hash_import;
-	inst->alg.setkey = cryptd_hash_setkey;
+	if (crypto_shash_alg_has_setkey(salg))
+		inst->alg.setkey = cryptd_hash_setkey;
 	inst->alg.digest = cryptd_hash_digest_enqueue;
 
 	err = ahash_register_instance(tmpl, inst);
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#3 lore raw

RFC PATCH 3/9 crypto: mcryptd - pass through absence of ->setkey()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

When the mcryptd template is used to wrap an unkeyed hash algorithm, don't install a ->setkey() method to the mcryptd instance. This change is necessary for mcryptd to keep working with unkeyed hash algorithms once we start enforcing that ->setkey() is called when present.

Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 crypto/mcryptd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
crypto/mcryptd.c +2 -1
index 2908382861ee..ace346b976b3 100644
@@ -534,7 +534,8 @@ static int mcryptd_create_hash(struct crypto_template *tmpl, struct rtattr **tb,
 	inst->alg.finup  = mcryptd_hash_finup_enqueue;
 	inst->alg.export = mcryptd_hash_export;
 	inst->alg.import = mcryptd_hash_import;
-	inst->alg.setkey = mcryptd_hash_setkey;
+	if (crypto_hash_alg_has_setkey(halg))
+		inst->alg.setkey = mcryptd_hash_setkey;
 	inst->alg.digest = mcryptd_hash_digest_enqueue;
 
 	err = ahash_register_instance(tmpl, inst);
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#4 lore raw

RFC PATCH 4/9 crypto: poly1305 - remove ->setkey() method

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Since Poly1305 requires a nonce per invocation, the Linux kernel implementations of Poly1305 don't use the crypto API's keying mechanism and instead expect the key and nonce as the first 32 bytes of the data. But ->setkey() is still defined as a stub returning an error code. This prevents Poly1305 from being used through AF_ALG and will also break it completely once we start enforcing that all crypto API users (not just AF_ALG) call ->setkey() if present.

Fix it by removing crypto_poly1305_setkey(), leaving ->setkey as NULL.

Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 arch/x86/crypto/poly1305_glue.c |  1 -
 crypto/poly1305_generic.c       | 17 +++++------------
 include/crypto/poly1305.h       |  2 --
 3 files changed, 5 insertions(+), 15 deletions(-)
arch/x86/crypto/poly1305_glue.c +0 -1
index e32142bc071d..28c372003e44 100644
@@ -164,7 +164,6 @@ static struct shash_alg alg = {
 	.init		= poly1305_simd_init,
 	.update		= poly1305_simd_update,
 	.final		= crypto_poly1305_final,
-	.setkey		= crypto_poly1305_setkey,
 	.descsize	= sizeof(struct poly1305_simd_desc_ctx),
 	.base		= {
 		.cra_name		= "poly1305",
crypto/poly1305_generic.c +5 -12
index b1c2d57dc734..ba39eb308c79 100644
@@ -47,17 +47,6 @@ int crypto_poly1305_init(struct shash_desc *desc)
 }
 EXPORT_SYMBOL_GPL(crypto_poly1305_init);
 
-int crypto_poly1305_setkey(struct crypto_shash *tfm,
-			   const u8 *key, unsigned int keylen)
-{
-	/* Poly1305 requires a unique key for each tag, which implies that
-	 * we can't set it on the tfm that gets accessed by multiple users
-	 * simultaneously. Instead we expect the key as the first 32 bytes in
-	 * the update() call. */
-	return -ENOTSUPP;
-}
-EXPORT_SYMBOL_GPL(crypto_poly1305_setkey);
-
 static void poly1305_setrkey(struct poly1305_desc_ctx *dctx, const u8 *key)
 {
 	/* r &= 0xffffffc0ffffffc0ffffffc0fffffff */

@@ -76,6 +65,11 @@ static void poly1305_setskey(struct poly1305_desc_ctx *dctx, const u8 *key)
 	dctx->s[3] = get_unaligned_le32(key + 12);
 }
 
+/*
+ * Poly1305 requires a unique key for each tag, which implies that we can't set
+ * it on the tfm that gets accessed by multiple users simultaneously. Instead we
+ * expect the key as the first 32 bytes in the update() call.
+ */
 unsigned int crypto_poly1305_setdesckey(struct poly1305_desc_ctx *dctx,
 					const u8 *src, unsigned int srclen)
 {

@@ -281,7 +275,6 @@ static struct shash_alg poly1305_alg = {
 	.init		= crypto_poly1305_init,
 	.update		= crypto_poly1305_update,
 	.final		= crypto_poly1305_final,
-	.setkey		= crypto_poly1305_setkey,
 	.descsize	= sizeof(struct poly1305_desc_ctx),
 	.base		= {
 		.cra_name		= "poly1305",
include/crypto/poly1305.h +0 -2
index c65567d01e8e..f718a19da82f 100644
@@ -31,8 +31,6 @@ struct poly1305_desc_ctx {
 };
 
 int crypto_poly1305_init(struct shash_desc *desc);
-int crypto_poly1305_setkey(struct crypto_shash *tfm,
-			   const u8 *key, unsigned int keylen);
 unsigned int crypto_poly1305_setdesckey(struct poly1305_desc_ctx *dctx,
 					const u8 *src, unsigned int srclen);
 int crypto_poly1305_update(struct shash_desc *desc,
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#5 lore raw

RFC PATCH 5/9 crypto: hash - annotate algorithms taking optional key

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

We need to consistently enforce that keyed hashes cannot be used without setting the key. To do this we need a reliable way to determine whether a given hash algorithm is keyed or not. AF_ALG currently does this by checking for the presence of a ->setkey() method. However, this is actually slightly broken because the CRC-32 algorithms implement ->setkey() but can also be used without a key. (The CRC-32 "key" is not actually a cryptographic key but rather represents the initial state. If not overridden, then a default initial state is used.)

Prepare to fix this by introducing a flag CRYPTO_ALG_OPTIONAL_KEY which indicates that the algorithm has a ->setkey() method, but it is not required to be called. Then set it on all the CRC-32 algorithms.

The same also applies to the Adler-32 implementation in Lustre.

Also, the cryptd and mcryptd templates have to pass through the flag from their underlying algorithm.

Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 arch/arm/crypto/crc32-ce-glue.c                               | 2 ++
 arch/arm64/crypto/crc32-ce-glue.c                             | 2 ++
 arch/powerpc/crypto/crc32c-vpmsum_glue.c                      | 1 +
 arch/s390/crypto/crc32-vx.c                                   | 3 +++
 arch/sparc/crypto/crc32c_glue.c                               | 1 +
 arch/x86/crypto/crc32-pclmul_glue.c                           | 1 +
 arch/x86/crypto/crc32c-intel_glue.c                           | 1 +
 crypto/crc32_generic.c                                        | 1 +
 crypto/crc32c_generic.c                                       | 1 +
 crypto/cryptd.c                                               | 7 +++----
 crypto/mcryptd.c                                              | 7 +++----
 drivers/crypto/bfin_crc.c                                     | 3 ++-
 drivers/crypto/stm32/stm32_crc32.c                            | 2 ++
 drivers/staging/lustre/lnet/libcfs/linux/linux-crypto-adler.c | 1 +
 include/linux/crypto.h                                        | 6 ++++++
 15 files changed, 30 insertions(+), 9 deletions(-)
arch/arm/crypto/crc32-ce-glue.c +2 -0
index 1b0e0e86ee9c..96e62ec105d0 100644
@@ -188,6 +188,7 @@ static struct shash_alg crc32_pmull_algs[] = { {
 	.base.cra_name		= "crc32",
 	.base.cra_driver_name	= "crc32-arm-ce",
 	.base.cra_priority	= 200,
+	.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 	.base.cra_blocksize	= 1,
 	.base.cra_module	= THIS_MODULE,
 }, {

@@ -203,6 +204,7 @@ static struct shash_alg crc32_pmull_algs[] = { {
 	.base.cra_name		= "crc32c",
 	.base.cra_driver_name	= "crc32c-arm-ce",
 	.base.cra_priority	= 200,
+	.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 	.base.cra_blocksize	= 1,
 	.base.cra_module	= THIS_MODULE,
 } };
arch/arm64/crypto/crc32-ce-glue.c +2 -0
index 624f4137918c..34b4e3d46aab 100644
@@ -185,6 +185,7 @@ static struct shash_alg crc32_pmull_algs[] = { {
 	.base.cra_name		= "crc32",
 	.base.cra_driver_name	= "crc32-arm64-ce",
 	.base.cra_priority	= 200,
+	.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 	.base.cra_blocksize	= 1,
 	.base.cra_module	= THIS_MODULE,
 }, {

@@ -200,6 +201,7 @@ static struct shash_alg crc32_pmull_algs[] = { {
 	.base.cra_name		= "crc32c",
 	.base.cra_driver_name	= "crc32c-arm64-ce",
 	.base.cra_priority	= 200,
+	.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 	.base.cra_blocksize	= 1,
 	.base.cra_module	= THIS_MODULE,
 } };
arch/powerpc/crypto/crc32c-vpmsum_glue.c +1 -0
index f058e0c3e4d4..fd1d6c83f0c0 100644
@@ -141,6 +141,7 @@ static struct shash_alg alg = {
 		.cra_name		= "crc32c",
 		.cra_driver_name	= "crc32c-vpmsum",
 		.cra_priority		= 200,
+		.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		= CHKSUM_BLOCK_SIZE,
 		.cra_ctxsize		= sizeof(u32),
 		.cra_module		= THIS_MODULE,
arch/s390/crypto/crc32-vx.c +3 -0
index 436865926c26..423ee05887e6 100644
@@ -239,6 +239,7 @@ static struct shash_alg crc32_vx_algs[] = {
 			.cra_name	 = "crc32",
 			.cra_driver_name = "crc32-vx",
 			.cra_priority	 = 200,
+			.cra_flags	 = CRYPTO_ALG_OPTIONAL_KEY,
 			.cra_blocksize	 = CRC32_BLOCK_SIZE,
 			.cra_ctxsize	 = sizeof(struct crc_ctx),
 			.cra_module	 = THIS_MODULE,

@@ -259,6 +260,7 @@ static struct shash_alg crc32_vx_algs[] = {
 			.cra_name	 = "crc32be",
 			.cra_driver_name = "crc32be-vx",
 			.cra_priority	 = 200,
+			.cra_flags	 = CRYPTO_ALG_OPTIONAL_KEY,
 			.cra_blocksize	 = CRC32_BLOCK_SIZE,
 			.cra_ctxsize	 = sizeof(struct crc_ctx),
 			.cra_module	 = THIS_MODULE,

@@ -279,6 +281,7 @@ static struct shash_alg crc32_vx_algs[] = {
 			.cra_name	 = "crc32c",
 			.cra_driver_name = "crc32c-vx",
 			.cra_priority	 = 200,
+			.cra_flags	 = CRYPTO_ALG_OPTIONAL_KEY,
 			.cra_blocksize	 = CRC32_BLOCK_SIZE,
 			.cra_ctxsize	 = sizeof(struct crc_ctx),
 			.cra_module	 = THIS_MODULE,
arch/sparc/crypto/crc32c_glue.c +1 -0
index d1064e46efe8..8aa664638c3c 100644
@@ -133,6 +133,7 @@ static struct shash_alg alg = {
 		.cra_name		=	"crc32c",
 		.cra_driver_name	=	"crc32c-sparc64",
 		.cra_priority		=	SPARC_CR_OPCODE_PRIORITY,
+		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		=	CHKSUM_BLOCK_SIZE,
 		.cra_ctxsize		=	sizeof(u32),
 		.cra_alignmask		=	7,
arch/x86/crypto/crc32-pclmul_glue.c +1 -0
index 27226df3f7d8..c8d9cdacbf10 100644
@@ -162,6 +162,7 @@ static struct shash_alg alg = {
 			.cra_name		= "crc32",
 			.cra_driver_name	= "crc32-pclmul",
 			.cra_priority		= 200,
+			.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 			.cra_blocksize		= CHKSUM_BLOCK_SIZE,
 			.cra_ctxsize		= sizeof(u32),
 			.cra_module		= THIS_MODULE,
arch/x86/crypto/crc32c-intel_glue.c +1 -0
index c194d5717ae5..5773e1161072 100644
@@ -226,6 +226,7 @@ static struct shash_alg alg = {
 		.cra_name		=	"crc32c",
 		.cra_driver_name	=	"crc32c-intel",
 		.cra_priority		=	200,
+		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		=	CHKSUM_BLOCK_SIZE,
 		.cra_ctxsize		=	sizeof(u32),
 		.cra_module		=	THIS_MODULE,
crypto/crc32_generic.c +1 -0
index aa2a25fc7482..718cbce8d169 100644
@@ -133,6 +133,7 @@ static struct shash_alg alg = {
 		.cra_name		= "crc32",
 		.cra_driver_name	= "crc32-generic",
 		.cra_priority		= 100,
+		.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		= CHKSUM_BLOCK_SIZE,
 		.cra_ctxsize		= sizeof(u32),
 		.cra_module		= THIS_MODULE,
crypto/crc32c_generic.c +1 -0
index 4c0a0e271876..372320399622 100644
@@ -146,6 +146,7 @@ static struct shash_alg alg = {
 		.cra_name		=	"crc32c",
 		.cra_driver_name	=	"crc32c-generic",
 		.cra_priority		=	100,
+		.cra_flags		=	CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		=	CHKSUM_BLOCK_SIZE,
 		.cra_alignmask		=	3,
 		.cra_ctxsize		=	sizeof(struct chksum_ctx),
crypto/cryptd.c +3 -4
index 457ae3e66a41..addca7bae33f 100644
@@ -896,10 +896,9 @@ static int cryptd_create_hash(struct crypto_template *tmpl, struct rtattr **tb,
 	if (err)
 		goto out_free_inst;
 
-	type = CRYPTO_ALG_ASYNC;
-	if (alg->cra_flags & CRYPTO_ALG_INTERNAL)
-		type |= CRYPTO_ALG_INTERNAL;
-	inst->alg.halg.base.cra_flags = type;
+	inst->alg.halg.base.cra_flags = CRYPTO_ALG_ASYNC |
+		(alg->cra_flags & (CRYPTO_ALG_INTERNAL |
+				   CRYPTO_ALG_OPTIONAL_KEY));
 
 	inst->alg.halg.digestsize = salg->digestsize;
 	inst->alg.halg.statesize = salg->statesize;
crypto/mcryptd.c +3 -4
index ace346b976b3..fe5129d6ff4e 100644
@@ -516,10 +516,9 @@ static int mcryptd_create_hash(struct crypto_template *tmpl, struct rtattr **tb,
 	if (err)
 		goto out_free_inst;
 
-	type = CRYPTO_ALG_ASYNC;
-	if (alg->cra_flags & CRYPTO_ALG_INTERNAL)
-		type |= CRYPTO_ALG_INTERNAL;
-	inst->alg.halg.base.cra_flags = type;
+	inst->alg.halg.base.cra_flags = CRYPTO_ALG_ASYNC |
+		(alg->cra_flags & (CRYPTO_ALG_INTERNAL |
+				   CRYPTO_ALG_OPTIONAL_KEY));
 
 	inst->alg.halg.digestsize = halg->digestsize;
 	inst->alg.halg.statesize = halg->statesize;
drivers/crypto/bfin_crc.c +2 -1
index a118b9bed669..bfbf8bf77f03 100644
@@ -494,7 +494,8 @@ static struct ahash_alg algs = {
 		.cra_driver_name	= DRIVER_NAME,
 		.cra_priority		= 100,
 		.cra_flags		= CRYPTO_ALG_TYPE_AHASH |
-						CRYPTO_ALG_ASYNC,
+						CRYPTO_ALG_ASYNC |
+						CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		= CHKSUM_BLOCK_SIZE,
 		.cra_ctxsize		= sizeof(struct bfin_crypto_crc_ctx),
 		.cra_alignmask		= 3,
drivers/crypto/stm32/stm32_crc32.c +2 -0
index 090582baecfe..8f09b8430893 100644
@@ -208,6 +208,7 @@ static struct shash_alg algs[] = {
 			.cra_name               = "crc32",
 			.cra_driver_name        = DRIVER_NAME,
 			.cra_priority           = 200,
+			.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 			.cra_blocksize          = CHKSUM_BLOCK_SIZE,
 			.cra_alignmask          = 3,
 			.cra_ctxsize            = sizeof(struct stm32_crc_ctx),

@@ -229,6 +230,7 @@ static struct shash_alg algs[] = {
 			.cra_name               = "crc32c",
 			.cra_driver_name        = DRIVER_NAME,
 			.cra_priority           = 200,
+			.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 			.cra_blocksize          = CHKSUM_BLOCK_SIZE,
 			.cra_alignmask          = 3,
 			.cra_ctxsize            = sizeof(struct stm32_crc_ctx),
drivers/staging/lustre/lnet/libcfs/linux/linux-crypto-adler.c +1 -0
index 2e5d311d2438..db81ed527452 100644
@@ -120,6 +120,7 @@ static struct shash_alg alg = {
 		.cra_name		= "adler32",
 		.cra_driver_name	= "adler32-zlib",
 		.cra_priority		= 100,
+		.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
 		.cra_blocksize		= CHKSUM_BLOCK_SIZE,
 		.cra_ctxsize		= sizeof(u32),
 		.cra_module		= THIS_MODULE,
include/linux/crypto.h +6 -0
index 78508ca4b108..72e47b3ecdc8 100644
@@ -106,6 +106,12 @@
  */
 #define CRYPTO_ALG_INTERNAL		0x00002000
 
+/*
+ * Set if the algorithm has a ->setkey() method but can be used without
+ * calling it first, i.e. there is a default key.
+ */
+#define CRYPTO_ALG_OPTIONAL_KEY		0x00004000
+
 /*
  * Transform masks and values (for crt_flags).
  */
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#6 lore raw

RFC PATCH 6/9 crypto: hash - prevent using keyed hashes without setting key

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Currently, almost none of the keyed hash algorithms check whether a key has been set before proceeding. Some algorithms are okay with this and will effectively just use a key of all 0's or some other bogus default. However, others will severely break, as demonstrated using "hmac(sha3-512-generic)", the unkeyed use of which causes a kernel crash via a (potentially exploitable) stack buffer overflow.

A while ago, this problem was solved for AF_ALG by pairing each hash transform with a 'has_key' bool. However, there are still other places in the kernel where userspace can specify an arbitrary hash algorithm by name, and the kernel uses it as unkeyed hash without checking whether it is really unkeyed. Examples of this include:

- KEYCTL_DH_COMPUTE, via the KDF extension
- dm-verity
- dm-crypt, via the ESSIV support
- dm-integrity, via the "internal hash" mode with no key given
- drbd (Distributed Replicated Block Device)

This bug is especially bad for KEYCTL_DH_COMPUTE as that requires no privileges to call.

Fix the bug for all users by adding a flag CRYPTO_TFM_NEED_KEY to the ->crt_flags of each hash transform that indicates whether the transform still needs to be keyed or not. Then, make the hash init, import, and digest functions return -ENOKEY if the key is still needed.

The new flag also replaces the 'has_key' bool which algif_hash was previously using, thereby simplifying the algif_hash implementation.

Reported-by syzbot <[email protected]>
Cc [email protected]
Signed-off-by Eric Biggers <[email protected]>
---
 crypto/ahash.c         | 22 ++++++++++++++++-----
 crypto/algif_hash.c    | 52 +++++++++++---------------------------------------
 crypto/shash.c         | 25 ++++++++++++++++++++----
 include/crypto/hash.h  | 34 +++++++++++++++++++++++----------
 include/linux/crypto.h |  2 ++
 5 files changed, 75 insertions(+), 60 deletions(-)
crypto/ahash.c +17 -5
index d2c8895bb2fe..266fc1d64f61 100644
@@ -193,11 +193,18 @@ int crypto_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 			unsigned int keylen)
 {
 	unsigned long alignmask = crypto_ahash_alignmask(tfm);
+	int err;
 
 	if ((unsigned long)key & alignmask)
-		return ahash_setkey_unaligned(tfm, key, keylen);
+		err = ahash_setkey_unaligned(tfm, key, keylen);
+	else
+		err = tfm->setkey(tfm, key, keylen);
+
+	if (err)
+		return err;
 
-	return tfm->setkey(tfm, key, keylen);
+	crypto_ahash_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(crypto_ahash_setkey);
 

@@ -368,7 +375,12 @@ EXPORT_SYMBOL_GPL(crypto_ahash_finup);
 
 int crypto_ahash_digest(struct ahash_request *req)
 {
-	return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->digest);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+
+	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
+	return crypto_ahash_op(req, tfm->digest);
 }
 EXPORT_SYMBOL_GPL(crypto_ahash_digest);
 

@@ -450,7 +462,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 	struct ahash_alg *alg = crypto_ahash_alg(hash);
 
 	hash->setkey = ahash_nosetkey;
-	hash->has_setkey = false;
 	hash->export = ahash_no_export;
 	hash->import = ahash_no_import;
 

@@ -465,7 +476,8 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 
 	if (alg->setkey) {
 		hash->setkey = alg->setkey;
-		hash->has_setkey = true;
+		if (!(alg->halg.base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY))
+			crypto_ahash_set_flags(hash, CRYPTO_TFM_NEED_KEY);
 	}
 	if (alg->export)
 		hash->export = alg->export;
crypto/algif_hash.c +11 -41

104 lines not on this page · raw message

crypto/shash.c +21 -4
index e849d3ee2e27..5d732c6bb4b2 100644
@@ -58,11 +58,18 @@ int crypto_shash_setkey(struct crypto_shash *tfm, const u8 *key,
 {
 	struct shash_alg *shash = crypto_shash_alg(tfm);
 	unsigned long alignmask = crypto_shash_alignmask(tfm);
+	int err;
 
 	if ((unsigned long)key & alignmask)
-		return shash_setkey_unaligned(tfm, key, keylen);
+		err = shash_setkey_unaligned(tfm, key, keylen);
+	else
+		err = shash->setkey(tfm, key, keylen);
+
+	if (err)
+		return err;
 
-	return shash->setkey(tfm, key, keylen);
+	crypto_shash_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(crypto_shash_setkey);
 

@@ -181,6 +188,9 @@ int crypto_shash_digest(struct shash_desc *desc, const u8 *data,
 	struct shash_alg *shash = crypto_shash_alg(tfm);
 	unsigned long alignmask = crypto_shash_alignmask(tfm);
 
+	if (crypto_shash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
 	if (((unsigned long)data | (unsigned long)out) & alignmask)
 		return shash_digest_unaligned(desc, data, len, out);
 

@@ -360,7 +370,8 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
 	crt->digest = shash_async_digest;
 	crt->setkey = shash_async_setkey;
 
-	crt->has_setkey = alg->setkey != shash_no_setkey;
+	crypto_ahash_set_flags(crt, crypto_shash_get_flags(shash) &
+				    CRYPTO_TFM_NEED_KEY);
 
 	if (alg->export)
 		crt->export = shash_async_export;

@@ -375,8 +386,14 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
 static int crypto_shash_init_tfm(struct crypto_tfm *tfm)
 {
 	struct crypto_shash *hash = __crypto_shash_cast(tfm);
+	struct shash_alg *alg = crypto_shash_alg(hash);
+
+	hash->descsize = alg->descsize;
+
+	if (crypto_shash_alg_has_setkey(alg) &&
+	    !(alg->base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY))
+		crypto_shash_set_flags(hash, CRYPTO_TFM_NEED_KEY);
 
-	hash->descsize = crypto_shash_alg(hash)->descsize;
 	return 0;
 }
include/crypto/hash.h +24 -10
index 0ed31fd80242..3880793e280e 100644
@@ -210,7 +210,6 @@ struct crypto_ahash {
 		      unsigned int keylen);
 
 	unsigned int reqsize;
-	bool has_setkey;
 	struct crypto_tfm base;
 };
 

@@ -410,11 +409,6 @@ static inline void *ahash_request_ctx(struct ahash_request *req)
 int crypto_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 			unsigned int keylen);
 
-static inline bool crypto_ahash_has_setkey(struct crypto_ahash *tfm)
-{
-	return tfm->has_setkey;
-}
-
 /**
  * crypto_ahash_finup() - update and finalize message digest
  * @req: reference to the ahash_request handle that holds all information

@@ -487,7 +481,12 @@ static inline int crypto_ahash_export(struct ahash_request *req, void *out)
  */
 static inline int crypto_ahash_import(struct ahash_request *req, const void *in)
 {
-	return crypto_ahash_reqtfm(req)->import(req, in);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+
+	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
+	return tfm->import(req, in);
 }
 
 /**

@@ -503,7 +502,12 @@ static inline int crypto_ahash_import(struct ahash_request *req, const void *in)
  */
 static inline int crypto_ahash_init(struct ahash_request *req)
 {
-	return crypto_ahash_reqtfm(req)->init(req);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+
+	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
+	return tfm->init(req);
 }
 
 /**

@@ -855,7 +859,12 @@ static inline int crypto_shash_export(struct shash_desc *desc, void *out)
  */
 static inline int crypto_shash_import(struct shash_desc *desc, const void *in)
 {
-	return crypto_shash_alg(desc->tfm)->import(desc, in);
+	struct crypto_shash *tfm = desc->tfm;
+
+	if (crypto_shash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
+	return crypto_shash_alg(tfm)->import(desc, in);
 }
 
 /**

@@ -871,7 +880,12 @@ static inline int crypto_shash_import(struct shash_desc *desc, const void *in)
  */
 static inline int crypto_shash_init(struct shash_desc *desc)
 {
-	return crypto_shash_alg(desc->tfm)->init(desc);
+	struct crypto_shash *tfm = desc->tfm;
+
+	if (crypto_shash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
+	return crypto_shash_alg(tfm)->init(desc);
 }
 
 /**
include/linux/crypto.h +2 -0
index 72e47b3ecdc8..29c4257f9c5b 100644
@@ -115,6 +115,8 @@
 /*
  * Transform masks and values (for crt_flags).
  */
+#define CRYPTO_TFM_NEED_KEY		0x00000001
+
 #define CRYPTO_TFM_REQ_MASK		0x000fff00
 #define CRYPTO_TFM_RES_MASK		0xfff00000
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#7 lore raw

RFC PATCH 7/9 crypto: ghash - remove checks for key being set

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Now that the crypto API prevents a keyed hash from being used without setting the key, there's no need for GHASH to do this check itself.

Signed-off-by Eric Biggers <[email protected]>
---
 crypto/ghash-generic.c | 6 ------
 1 file changed, 6 deletions(-)
crypto/ghash-generic.c +0 -6
index 12ad3e3a84e3..1bffb3f712dd 100644
@@ -56,9 +56,6 @@ static int ghash_update(struct shash_desc *desc,
 	struct ghash_ctx *ctx = crypto_shash_ctx(desc->tfm);
 	u8 *dst = dctx->buffer;
 
-	if (!ctx->gf128)
-		return -ENOKEY;
-
 	if (dctx->bytes) {
 		int n = min(srclen, dctx->bytes);
 		u8 *pos = dst + (GHASH_BLOCK_SIZE - dctx->bytes);

@@ -111,9 +108,6 @@ static int ghash_final(struct shash_desc *desc, u8 *dst)
 	struct ghash_ctx *ctx = crypto_shash_ctx(desc->tfm);
 	u8 *buf = dctx->buffer;
 
-	if (!ctx->gf128)
-		return -ENOKEY;
-
 	ghash_flush(ctx, dctx);
 	memcpy(dst, buf, GHASH_BLOCK_SIZE);
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#8 lore raw

RFC PATCH 8/9 crypto: skcipher - prevent using skciphers without setting key

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Similar to what was done for the hash API, update the skcipher API to track whether each transform has been keyed, and reject encryption/decryption if a key is needed but one hasn't been set.

This isn't as important as the equivalent fix for the hash API because symmetric ciphers almost always require a key (the "null cipher" is the only exception), so are unlikely to be used without one. Still, tracking the key will prevent accidental unkeyed use. algif_skcipher also had to track the key anyway, so the new flag replaces that and simplifies the algif_skcipher implementation.

Signed-off-by Eric Biggers <[email protected]>
---
 crypto/algif_skcipher.c   | 59 +++++++++++------------------------------------
 crypto/skcipher.c         | 30 ++++++++++++++++++++----
 include/crypto/skcipher.h | 11 +++++----
 3 files changed, 45 insertions(+), 55 deletions(-)
crypto/algif_skcipher.c +13 -46
index c5c47b680152..c88e5e4cd6a6 100644
@@ -38,11 +38,6 @@
 #include <linux/net.h>
 #include <net/sock.h>
 
-struct skcipher_tfm {
-	struct crypto_skcipher *skcipher;
-	bool has_key;
-};
-
 static int skcipher_sendmsg(struct socket *sock, struct msghdr *msg,
 			    size_t size)
 {

@@ -50,8 +45,7 @@ static int skcipher_sendmsg(struct socket *sock, struct msghdr *msg,
 	struct alg_sock *ask = alg_sk(sk);
 	struct sock *psk = ask->parent;
 	struct alg_sock *pask = alg_sk(psk);
-	struct skcipher_tfm *skc = pask->private;
-	struct crypto_skcipher *tfm = skc->skcipher;
+	struct crypto_skcipher *tfm = pask->private;
 	unsigned ivsize = crypto_skcipher_ivsize(tfm);
 
 	return af_alg_sendmsg(sock, msg, size, ivsize);

@@ -65,8 +59,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct sock *psk = ask->parent;
 	struct alg_sock *pask = alg_sk(psk);
 	struct af_alg_ctx *ctx = ask->private;
-	struct skcipher_tfm *skc = pask->private;
-	struct crypto_skcipher *tfm = skc->skcipher;
+	struct crypto_skcipher *tfm = pask->private;
 	unsigned int bs = crypto_skcipher_blocksize(tfm);
 	struct af_alg_async_req *areq;
 	int err = 0;

@@ -221,7 +214,7 @@ static int skcipher_check_key(struct socket *sock)
 	int err = 0;
 	struct sock *psk;
 	struct alg_sock *pask;
-	struct skcipher_tfm *tfm;
+	struct crypto_skcipher *tfm;
 	struct sock *sk = sock->sk;
 	struct alg_sock *ask = alg_sk(sk);
 

@@ -235,7 +228,7 @@ static int skcipher_check_key(struct socket *sock)
 
 	err = -ENOKEY;
 	lock_sock_nested(psk, SINGLE_DEPTH_NESTING);
-	if (!tfm->has_key)
+	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
 		goto unlock;
 
 	if (!pask->refcnt++)

@@ -314,41 +307,17 @@ static struct proto_ops algif_skcipher_ops_nokey = {
 
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
-	struct skcipher_tfm *tfm;
-	struct crypto_skcipher *skcipher;
-
-	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
-	if (!tfm)
-		return ERR_PTR(-ENOMEM);
-
-	skcipher = crypto_alloc_skcipher(name, type, mask);
-	if (IS_ERR(skcipher)) {
-		kfree(tfm);
-		return ERR_CAST(skcipher);
-	}
-
-	tfm->skcipher = skcipher;
-
-	return tfm;
+	return crypto_alloc_skcipher(name, type, mask);
 }
 
 static void skcipher_release(void *private)
 {
-	struct skcipher_tfm *tfm = private;
-
-	crypto_free_skcipher(tfm->skcipher);
-	kfree(tfm);
+	crypto_free_skcipher(private);
 }
 
 static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	struct skcipher_tfm *tfm = private;
-	int err;
-
-	err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
-	tfm->has_key = !err;
-
-	return err;
+	return crypto_skcipher_setkey(private, key, keylen);
 }
 
 static void skcipher_sock_destruct(struct sock *sk)

@@ -357,8 +326,7 @@ static void skcipher_sock_destruct(struct sock *sk)
 	struct af_alg_ctx *ctx = ask->private;
 	struct sock *psk = ask->parent;
 	struct alg_sock *pask = alg_sk(psk);
-	struct skcipher_tfm *skc = pask->private;
-	struct crypto_skcipher *tfm = skc->skcipher;
+	struct crypto_skcipher *tfm = pask->private;
 
 	af_alg_pull_tsgl(sk, ctx->used, NULL, 0);
 	sock_kzfree_s(sk, ctx->iv, crypto_skcipher_ivsize(tfm));

@@ -370,22 +338,21 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 {
 	struct af_alg_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	struct skcipher_tfm *tfm = private;
-	struct crypto_skcipher *skcipher = tfm->skcipher;
+	struct crypto_skcipher *tfm = private;
 	unsigned int len = sizeof(*ctx);
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
+	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(tfm),
 			       GFP_KERNEL);
 	if (!ctx->iv) {
 		sock_kfree_s(sk, ctx, len);
 		return -ENOMEM;
 	}
 
-	memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
+	memset(ctx->iv, 0, crypto_skcipher_ivsize(tfm));
 
 	INIT_LIST_HEAD(&ctx->tsgl_list);
 	ctx->len = len;

@@ -405,9 +372,9 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
 
 static int skcipher_accept_parent(void *private, struct sock *sk)
 {
-	struct skcipher_tfm *tfm = private;
+	struct crypto_skcipher *tfm = private;
 
-	if (!tfm->has_key && crypto_skcipher_has_setkey(tfm->skcipher))
+	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
 		return -ENOKEY;
 
 	return skcipher_accept_parent_nokey(private, sk);
crypto/skcipher.c +26 -4
index 11af5fd6a443..0fe2a2923ad0 100644
@@ -598,8 +598,11 @@ static int skcipher_setkey_blkcipher(struct crypto_skcipher *tfm,
 	err = crypto_blkcipher_setkey(blkcipher, key, keylen);
 	crypto_skcipher_set_flags(tfm, crypto_blkcipher_get_flags(blkcipher) &
 				       CRYPTO_TFM_RES_MASK);
+	if (err)
+		return err;
 
-	return err;
+	crypto_skcipher_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
+	return 0;
 }
 
 static int skcipher_crypt_blkcipher(struct skcipher_request *req,

@@ -674,6 +677,9 @@ static int crypto_init_skcipher_ops_blkcipher(struct crypto_tfm *tfm)
 	skcipher->ivsize = crypto_blkcipher_ivsize(blkcipher);
 	skcipher->keysize = calg->cra_blkcipher.max_keysize;
 
+	if (skcipher->keysize)
+		crypto_skcipher_set_flags(skcipher, CRYPTO_TFM_NEED_KEY);
+
 	return 0;
 }
 

@@ -692,8 +698,11 @@ static int skcipher_setkey_ablkcipher(struct crypto_skcipher *tfm,
 	crypto_skcipher_set_flags(tfm,
 				  crypto_ablkcipher_get_flags(ablkcipher) &
 				  CRYPTO_TFM_RES_MASK);
+	if (err)
+		return err;
 
-	return err;
+	crypto_skcipher_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
+	return 0;
 }
 
 static int skcipher_crypt_ablkcipher(struct skcipher_request *req,

@@ -767,6 +776,9 @@ static int crypto_init_skcipher_ops_ablkcipher(struct crypto_tfm *tfm)
 			    sizeof(struct ablkcipher_request);
 	skcipher->keysize = calg->cra_ablkcipher.max_keysize;
 
+	if (skcipher->keysize)
+		crypto_skcipher_set_flags(skcipher, CRYPTO_TFM_NEED_KEY);
+
 	return 0;
 }
 

@@ -796,6 +808,7 @@ static int skcipher_setkey(struct crypto_skcipher *tfm, const u8 *key,
 {
 	struct skcipher_alg *cipher = crypto_skcipher_alg(tfm);
 	unsigned long alignmask = crypto_skcipher_alignmask(tfm);
+	int err;
 
 	if (keylen < cipher->min_keysize || keylen > cipher->max_keysize) {
 		crypto_skcipher_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);

@@ -803,9 +816,15 @@ static int skcipher_setkey(struct crypto_skcipher *tfm, const u8 *key,
 	}
 
 	if ((unsigned long)key & alignmask)
-		return skcipher_setkey_unaligned(tfm, key, keylen);
+		err = skcipher_setkey_unaligned(tfm, key, keylen);
+	else
+		err = cipher->setkey(tfm, key, keylen);
+
+	if (err)
+		return err;
 
-	return cipher->setkey(tfm, key, keylen);
+	crypto_skcipher_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
+	return 0;
 }
 
 static void crypto_skcipher_exit_tfm(struct crypto_tfm *tfm)

@@ -834,6 +853,9 @@ static int crypto_skcipher_init_tfm(struct crypto_tfm *tfm)
 	skcipher->ivsize = alg->ivsize;
 	skcipher->keysize = alg->max_keysize;
 
+	if (skcipher->keysize)
+		crypto_skcipher_set_flags(skcipher, CRYPTO_TFM_NEED_KEY);
+
 	if (alg->exit)
 		skcipher->base.exit = crypto_skcipher_exit_tfm;
include/crypto/skcipher.h +6 -5
index 562001cb412b..2f327f090c3e 100644
@@ -401,11 +401,6 @@ static inline int crypto_skcipher_setkey(struct crypto_skcipher *tfm,
 	return tfm->setkey(tfm, key, keylen);
 }
 
-static inline bool crypto_skcipher_has_setkey(struct crypto_skcipher *tfm)
-{
-	return tfm->keysize;
-}
-
 static inline unsigned int crypto_skcipher_default_keysize(
 	struct crypto_skcipher *tfm)
 {

@@ -442,6 +437,9 @@ static inline int crypto_skcipher_encrypt(struct skcipher_request *req)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 
+	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
 	return tfm->encrypt(req);
 }
 

@@ -460,6 +458,9 @@ static inline int crypto_skcipher_decrypt(struct skcipher_request *req)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 
+	if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
 	return tfm->decrypt(req);
 }
Signature · 1 line
2.15.1.620.gb9897f4670-goog
author
#9 lore raw

RFC PATCH 9/9 crypto: aead - prevent using AEADs without setting key

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Similar to what was done for the hash API, update the AEAD API to track whether each transform has been keyed, and reject encryption/decryption if a key is needed but one hasn't been set.

This isn't quite as important as the equivalent fix for the hash API because AEADs always require a key, so are unlikely to be used without one. Still, tracking the key will prevent accidental unkeyed use. algif_aead also had to track the key anyway, so the new flag replaces that and slightly simplifies the algif_aead implementation.

Signed-off-by Eric Biggers <[email protected]>
---
 crypto/aead.c         | 13 +++++++++++--
 crypto/algif_aead.c   | 11 +++--------
 include/crypto/aead.h | 10 +++++++++-
 3 files changed, 23 insertions(+), 11 deletions(-)
crypto/aead.c +11 -2
index fe00cbd7243d..60b3bbe973e7 100644
@@ -54,11 +54,18 @@ int crypto_aead_setkey(struct crypto_aead *tfm,
 		       const u8 *key, unsigned int keylen)
 {
 	unsigned long alignmask = crypto_aead_alignmask(tfm);
+	int err;
 
 	if ((unsigned long)key & alignmask)
-		return setkey_unaligned(tfm, key, keylen);
+		err = setkey_unaligned(tfm, key, keylen);
+	else
+		err = crypto_aead_alg(tfm)->setkey(tfm, key, keylen);
+
+	if (err)
+		return err;
 
-	return crypto_aead_alg(tfm)->setkey(tfm, key, keylen);
+	crypto_aead_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(crypto_aead_setkey);
 

@@ -93,6 +100,8 @@ static int crypto_aead_init_tfm(struct crypto_tfm *tfm)
 	struct crypto_aead *aead = __crypto_aead_cast(tfm);
 	struct aead_alg *alg = crypto_aead_alg(aead);
 
+	crypto_aead_set_flags(aead, CRYPTO_TFM_NEED_KEY);
+
 	aead->authsize = alg->maxauthsize;
 
 	if (alg->exit)
crypto/algif_aead.c +3 -8
index d963c8cf8a55..4b07edd5a9ff 100644
@@ -42,7 +42,6 @@
 
 struct aead_tfm {
 	struct crypto_aead *aead;
-	bool has_key;
 	struct crypto_skcipher *null_tfm;
 };
 

@@ -398,7 +397,7 @@ static int aead_check_key(struct socket *sock)
 
 	err = -ENOKEY;
 	lock_sock_nested(psk, SINGLE_DEPTH_NESTING);
-	if (!tfm->has_key)
+	if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_KEY)
 		goto unlock;
 
 	if (!pask->refcnt++)

@@ -523,12 +522,8 @@ static int aead_setauthsize(void *private, unsigned int authsize)
 static int aead_setkey(void *private, const u8 *key, unsigned int keylen)
 {
 	struct aead_tfm *tfm = private;
-	int err;
-
-	err = crypto_aead_setkey(tfm->aead, key, keylen);
-	tfm->has_key = !err;
 
-	return err;
+	return crypto_aead_setkey(tfm->aead, key, keylen);
 }
 
 static void aead_sock_destruct(struct sock *sk)

@@ -589,7 +584,7 @@ static int aead_accept_parent(void *private, struct sock *sk)
 {
 	struct aead_tfm *tfm = private;
 
-	if (!tfm->has_key)
+	if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_KEY)
 		return -ENOKEY;
 
 	return aead_accept_parent_nokey(private, sk);
include/crypto/aead.h +9 -1
index 03b97629442c..1e26f790b03f 100644
@@ -327,7 +327,12 @@ static inline struct crypto_aead *crypto_aead_reqtfm(struct aead_request *req)
  */
 static inline int crypto_aead_encrypt(struct aead_request *req)
 {
-	return crypto_aead_alg(crypto_aead_reqtfm(req))->encrypt(req);
+	struct crypto_aead *aead = crypto_aead_reqtfm(req);
+
+	if (crypto_aead_get_flags(aead) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
+	return crypto_aead_alg(aead)->encrypt(req);
 }
 
 /**

@@ -356,6 +361,9 @@ static inline int crypto_aead_decrypt(struct aead_request *req)
 {
 	struct crypto_aead *aead = crypto_aead_reqtfm(req);
 
+	if (crypto_aead_get_flags(aead) & CRYPTO_TFM_NEED_KEY)
+		return -ENOKEY;
+
 	if (req->cryptlen < crypto_aead_authsize(aead))
 		return -EINVAL;
Signature · 1 line
2.15.1.620.gb9897f4670-goog
maintainer
#10 lore raw
In reply to Eric Biggers
Quoted 24 lines · Eric Biggers

From: Eric Biggers <[email protected]>

This series updates the crypto API to consistently prevent using keyed algorithms without setting the key. Currently this is prevented for AF_ALG but not for other crypto API users, which is very problematic for other places in the kernel where userspace can specify a hash algorithm by name, e.g. KEYCTL_DH_COMPUTE as demonstrated by syzbot
(https://marc.info/?l=linux-crypto-vger&m=151395810921850).

This series fixes the bug for all users by adding a flag CRYPTO_ALG_NEED_KEY to crypto_tfm.crt_flags. This flag is set if needed when the tfm is created, is cleared when the key is set, and is checked when doing an operation that would require the key.

Patches 1-6 update the hash API, which is the primary fix. I've marked all those patches for stable, which is kind of a pain, but it seems the alternative would be very messy -- we'd have to patch at least 5 different crypto API users (probably missing some), then revert those patches upstream once we have the proper fix at the API level.

The last two patches also extend the fix to the skcipher and AEAD APIs, primarily as a sanity check since users should be less likely to try to use skciphers or AEADs without setting a key.

Quoted 10 lines

Eric Biggers (9):
crypto: hash - introduce crypto_hash_alg_has_setkey()
crypto: cryptd - pass through absence of ->setkey()
crypto: mcryptd - pass through absence of ->setkey()
crypto: poly1305 - remove ->setkey() method
crypto: hash - annotate algorithms taking optional key
crypto: hash - prevent using keyed hashes without setting key
crypto: ghash - remove checks for key being set
crypto: skcipher - prevent using skciphers without setting key
crypto: aead - prevent using AEADs without setting key

All applied. Thanks.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt