From 8289b99b6683641386e6c6f1b7db1c1c36a621bf Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Sat, 3 Nov 2018 14:56:03 -0700 Subject: crypto: user - clean up report structure copying There have been a pretty ridiculous number of issues with initializing the report structures that are copied to userspace by NETLINK_CRYPTO. Commit 813e2f3cf818 ("crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion") replaced some strncpy()s with strlcpy()s, thereby introducing information leaks. Later two other people tried to replace other strncpy()s with strlcpy() too, which would have introduced even more information leaks: - https://lore.kernel.org/patchwork/patch/954991/ - https://patchwork.kernel.org/patch/10434351/ Commit 0115ab23b941 ("crypto: user - Implement a generic crypto statistics") also uses the buggy strlcpy() approach and therefore leaks uninitialized memory to userspace. A fix was proposed, but it was originally incomplete. Seeing as how apparently no one can get this right with the current approach, change all the reporting functions to: - Start by memsetting the report structure to 0. This guarantees it's always initialized, regardless of what happens later. - Initialize all strings using strscpy(). This is safe after the memset, ensures null termination of long strings, avoids unnecessary work, and avoids the -Wstringop-truncation warnings from gcc. - Use sizeof(var) instead of sizeof(type). This is more robust against copy+paste errors. For simplicity, also reuse the -EMSGSIZE return value from nla_put(). Signed-off-by: Eric Biggers Signed-off-by: Herbert Xu --- crypto/ablkcipher.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) (limited to 'crypto/ablkcipher.c') diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c index 8882e90e..b5e9ce19 100644 --- a/crypto/ablkcipher.c +++ b/crypto/ablkcipher.c @@ -365,23 +365,19 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg) { struct crypto_report_blkcipher rblkcipher; - strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type)); - strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "", + memset(&rblkcipher, 0, sizeof(rblkcipher)); + + strscpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type)); + strscpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "", sizeof(rblkcipher.geniv)); - rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0'; rblkcipher.blocksize = alg->cra_blocksize; rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize; rblkcipher.max_keysize = alg->cra_ablkcipher.max_keysize; rblkcipher.ivsize = alg->cra_ablkcipher.ivsize; - if (nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER, - sizeof(struct crypto_report_blkcipher), &rblkcipher)) - goto nla_put_failure; - return 0; - -nla_put_failure: - return -EMSGSIZE; + return nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER, + sizeof(rblkcipher), &rblkcipher); } #else static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg) @@ -440,23 +436,19 @@ static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg) { struct crypto_report_blkcipher rblkcipher; - strncpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type)); - strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "", + memset(&rblkcipher, 0, sizeof(rblkcipher)); + + strscpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type)); + strscpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "", sizeof(rblkcipher.geniv)); - rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0'; rblkcipher.blocksize = alg->cra_blocksize; rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize; rblkcipher.max_keysize = alg->cra_ablkcipher.max_keysize; rblkcipher.ivsize = alg->cra_ablkcipher.ivsize; - if (nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER, - sizeof(struct crypto_report_blkcipher), &rblkcipher)) - goto nla_put_failure; - return 0; - -nla_put_failure: - return -EMSGSIZE; + return nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER, + sizeof(rblkcipher), &rblkcipher); } #else static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg) -- cgit v1.2.3 From c9d9cac24da89c1b6930f948e2f6803c9761fcc8 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Sun, 16 Dec 2018 15:55:06 -0800 Subject: crypto: skcipher - remove remnants of internal IV generators Remove dead code related to internal IV generators, which are no longer used since they've been replaced with the "seqiv" and "echainiv" templates. The removed code includes: - The "givcipher" (GIVCIPHER) algorithm type. No algorithms are registered with this type anymore, so it's unneeded. - The "const char *geniv" member of aead_alg, ablkcipher_alg, and blkcipher_alg. A few algorithms still set this, but it isn't used anymore except to show via /proc/crypto and CRYPTO_MSG_GETALG. Just hardcode "" or "" in those cases. - The 'skcipher_givcrypt_request' structure, which is never used. Signed-off-by: Eric Biggers Signed-off-by: Herbert Xu --- crypto/ablkcipher.c | 76 ++--------------------------------------------------- crypto/blkcipher.c | 6 ++--- crypto/cryptd.c | 4 +-- crypto/ctr.c | 2 -- crypto/skcipher.c | 6 ++--- 5 files changed, 7 insertions(+), 87 deletions(-) (limited to 'crypto/ablkcipher.c') diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c index b5e9ce19..b3395870 100644 --- a/crypto/ablkcipher.c +++ b/crypto/ablkcipher.c @@ -368,8 +368,7 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg) memset(&rblkcipher, 0, sizeof(rblkcipher)); strscpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type)); - strscpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "", - sizeof(rblkcipher.geniv)); + strscpy(rblkcipher.geniv, "", sizeof(rblkcipher.geniv)); rblkcipher.blocksize = alg->cra_blocksize; rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize; @@ -399,7 +398,7 @@ static void crypto_ablkcipher_show(struct seq_file *m, struct crypto_alg *alg) seq_printf(m, "min keysize : %u\n", ablkcipher->min_keysize); seq_printf(m, "max keysize : %u\n", ablkcipher->max_keysize); seq_printf(m, "ivsize : %u\n", ablkcipher->ivsize); - seq_printf(m, "geniv : %s\n", ablkcipher->geniv ?: ""); + seq_printf(m, "geniv : \n"); } const struct crypto_type crypto_ablkcipher_type = { @@ -411,74 +410,3 @@ const struct crypto_type crypto_ablkcipher_type = { .report = crypto_ablkcipher_report, }; EXPORT_SYMBOL_GPL(crypto_ablkcipher_type); - -static int crypto_init_givcipher_ops(struct crypto_tfm *tfm, u32 type, - u32 mask) -{ - struct ablkcipher_alg *alg = &tfm->__crt_alg->cra_ablkcipher; - struct ablkcipher_tfm *crt = &tfm->crt_ablkcipher; - - if (alg->ivsize > PAGE_SIZE / 8) - return -EINVAL; - - crt->setkey = tfm->__crt_alg->cra_flags & CRYPTO_ALG_GENIV ? - alg->setkey : setkey; - crt->encrypt = alg->encrypt; - crt->decrypt = alg->decrypt; - crt->base = __crypto_ablkcipher_cast(tfm); - crt->ivsize = alg->ivsize; - - return 0; -} - -#ifdef CONFIG_NET -static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg) -{ - struct crypto_report_blkcipher rblkcipher; - - memset(&rblkcipher, 0, sizeof(rblkcipher)); - - strscpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type)); - strscpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "", - sizeof(rblkcipher.geniv)); - - rblkcipher.blocksize = alg->cra_blocksize; - rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize; - rblkcipher.max_keysize = alg->cra_ablkcipher.max_keysize; - rblkcipher.ivsize = alg->cra_ablkcipher.ivsize; - - return nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER, - sizeof(rblkcipher), &rblkcipher); -} -#else -static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg) -{ - return -ENOSYS; -} -#endif - -static void crypto_givcipher_show(struct seq_file *m, struct crypto_alg *alg) - __maybe_unused; -static void crypto_givcipher_show(struct seq_file *m, struct crypto_alg *alg) -{ - struct ablkcipher_alg *ablkcipher = &alg->cra_ablkcipher; - - seq_printf(m, "type : givcipher\n"); - seq_printf(m, "async : %s\n", alg->cra_flags & CRYPTO_ALG_ASYNC ? - "yes" : "no"); - seq_printf(m, "blocksize : %u\n", alg->cra_blocksize); - seq_printf(m, "min keysize : %u\n", ablkcipher->min_keysize); - seq_printf(m, "max keysize : %u\n", ablkcipher->max_keysize); - seq_printf(m, "ivsize : %u\n", ablkcipher->ivsize); - seq_printf(m, "geniv : %s\n", ablkcipher->geniv ?: ""); -} - -const struct crypto_type crypto_givcipher_type = { - .ctxsize = crypto_ablkcipher_ctxsize, - .init = crypto_init_givcipher_ops, -#ifdef CONFIG_PROC_FS - .show = crypto_givcipher_show, -#endif - .report = crypto_givcipher_report, -}; -EXPORT_SYMBOL_GPL(crypto_givcipher_type); diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c index 19323751..c5398bd5 100644 --- a/crypto/blkcipher.c +++ b/crypto/blkcipher.c @@ -510,8 +510,7 @@ static int crypto_blkcipher_report(struct sk_buff *skb, struct crypto_alg *alg) memset(&rblkcipher, 0, sizeof(rblkcipher)); strscpy(rblkcipher.type, "blkcipher", sizeof(rblkcipher.type)); - strscpy(rblkcipher.geniv, alg->cra_blkcipher.geniv ?: "", - sizeof(rblkcipher.geniv)); + strscpy(rblkcipher.geniv, "", sizeof(rblkcipher.geniv)); rblkcipher.blocksize = alg->cra_blocksize; rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize; @@ -537,8 +536,7 @@ static void crypto_blkcipher_show(struct seq_file *m, struct crypto_alg *alg) seq_printf(m, "min keysize : %u\n", alg->cra_blkcipher.min_keysize); seq_printf(m, "max keysize : %u\n", alg->cra_blkcipher.max_keysize); seq_printf(m, "ivsize : %u\n", alg->cra_blkcipher.ivsize); - seq_printf(m, "geniv : %s\n", alg->cra_blkcipher.geniv ?: - ""); + seq_printf(m, "geniv : \n"); } const struct crypto_type crypto_blkcipher_type = { diff --git a/crypto/cryptd.c b/crypto/cryptd.c index 7118fb5e..5640e5db 100644 --- a/crypto/cryptd.c +++ b/crypto/cryptd.c @@ -422,8 +422,6 @@ static int cryptd_create_blkcipher(struct crypto_template *tmpl, inst->alg.cra_ablkcipher.min_keysize = alg->cra_blkcipher.min_keysize; inst->alg.cra_ablkcipher.max_keysize = alg->cra_blkcipher.max_keysize; - inst->alg.cra_ablkcipher.geniv = alg->cra_blkcipher.geniv; - inst->alg.cra_ctxsize = sizeof(struct cryptd_blkcipher_ctx); inst->alg.cra_init = cryptd_blkcipher_init_tfm; @@ -1174,7 +1172,7 @@ struct cryptd_ablkcipher *cryptd_alloc_ablkcipher(const char *alg_name, return ERR_PTR(-EINVAL); type = crypto_skcipher_type(type); mask &= ~CRYPTO_ALG_TYPE_MASK; - mask |= (CRYPTO_ALG_GENIV | CRYPTO_ALG_TYPE_BLKCIPHER_MASK); + mask |= CRYPTO_ALG_TYPE_BLKCIPHER_MASK; tfm = crypto_alloc_base(cryptd_alg_name, type, mask); if (IS_ERR(tfm)) return ERR_CAST(tfm); diff --git a/crypto/ctr.c b/crypto/ctr.c index 435b75bd..30f3946e 100644 --- a/crypto/ctr.c +++ b/crypto/ctr.c @@ -233,8 +233,6 @@ static struct crypto_instance *crypto_ctr_alloc(struct rtattr **tb) inst->alg.cra_blkcipher.encrypt = crypto_ctr_crypt; inst->alg.cra_blkcipher.decrypt = crypto_ctr_crypt; - inst->alg.cra_blkcipher.geniv = "chainiv"; - out: crypto_mod_put(alg); return inst; diff --git a/crypto/skcipher.c b/crypto/skcipher.c index 41b4f7f2..2a969296 100644 --- a/crypto/skcipher.c +++ b/crypto/skcipher.c @@ -579,8 +579,7 @@ static unsigned int crypto_skcipher_extsize(struct crypto_alg *alg) if (alg->cra_type == &crypto_blkcipher_type) return sizeof(struct crypto_blkcipher *); - if (alg->cra_type == &crypto_ablkcipher_type || - alg->cra_type == &crypto_givcipher_type) + if (alg->cra_type == &crypto_ablkcipher_type) return sizeof(struct crypto_ablkcipher *); return crypto_alg_extsize(alg); @@ -844,8 +843,7 @@ static int crypto_skcipher_init_tfm(struct crypto_tfm *tfm) if (tfm->__crt_alg->cra_type == &crypto_blkcipher_type) return crypto_init_skcipher_ops_blkcipher(tfm); - if (tfm->__crt_alg->cra_type == &crypto_ablkcipher_type || - tfm->__crt_alg->cra_type == &crypto_givcipher_type) + if (tfm->__crt_alg->cra_type == &crypto_ablkcipher_type) return crypto_init_skcipher_ops_ablkcipher(tfm); skcipher->setkey = skcipher_setkey; -- cgit v1.2.3