security: keys: switch big_key encryption to AES in CTR mode
diff mbox

Message ID 20170915223723.20789-1-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Sept. 15, 2017, 10:37 p.m. UTC
The ECB chaining mode only supports inputs that are a multiple of the
blocksize. Furthermore, it is not recommended for direct use, given
that it may reveal recurring patterns in the plaintext, due to the
lack of feedback between input blocks. So let's solve both issues at
once, and switch to AES in CTR mode.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 security/keys/big_key.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Eric Biggers Sept. 16, 2017, 2:47 a.m. UTC | #1
[Added Jason Donenfeld to Cc]

Hi Ard,

On Fri, Sep 15, 2017 at 03:37:23PM -0700, Ard Biesheuvel wrote:
> The ECB chaining mode only supports inputs that are a multiple of the
> blocksize. Furthermore, it is not recommended for direct use, given
> that it may reveal recurring patterns in the plaintext, due to the
> lack of feedback between input blocks. So let's solve both issues at
> once, and switch to AES in CTR mode.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

The choice of ECB here is definitely a mistake, but it really should use an
authenticated encryption mode such as GCM rather than plain CTR.  There was a
patch a few months ago that implemented this and fixed a couple other problems,
but it hasn't been merged; maybe someone wants to take over that patch instead?
Link: http://lkml.kernel.org/r/20170607101209.7603-1-Jason@zx2c4.com

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Sept. 16, 2017, 7:05 a.m. UTC | #2
On 15 September 2017 at 19:47, Eric Biggers <ebiggers3@gmail.com> wrote:
> [Added Jason Donenfeld to Cc]
>
> Hi Ard,
>
> On Fri, Sep 15, 2017 at 03:37:23PM -0700, Ard Biesheuvel wrote:
>> The ECB chaining mode only supports inputs that are a multiple of the
>> blocksize. Furthermore, it is not recommended for direct use, given
>> that it may reveal recurring patterns in the plaintext, due to the
>> lack of feedback between input blocks. So let's solve both issues at
>> once, and switch to AES in CTR mode.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> The choice of ECB here is definitely a mistake, but it really should use an
> authenticated encryption mode such as GCM rather than plain CTR.  There was a
> patch a few months ago that implemented this and fixed a couple other problems,
> but it hasn't been merged; maybe someone wants to take over that patch instead?
> Link: http://lkml.kernel.org/r/20170607101209.7603-1-Jason@zx2c4.com
>

Ah right, fair enough. I just happened to be standing next to David
when Ilhan brought up the issue, so I had no idea that a patch had
been proposed already.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason A. Donenfeld Sept. 16, 2017, 12:40 p.m. UTC | #3
Hey Eric,

Thanks for adding me to the CC.

On Sep 16, 2017 05:14, "Eric Biggers" <ebiggers3@gmail.com> wrote:
> There was a
> patch a few months ago that implemented this and fixed a couple other problems,
> but it hasn't been merged; maybe someone wants to take over that patch instead?
> Link: http://lkml.kernel.org/r/20170607101209.7603-1-Jason@zx2c4.com

I didn't realize this patch wasn't merged and was eventually
neglected. (I wonder how many other of my patches have met this
fate...) I'll take another look at it this week and resubmit (and
possibly tweek if needed).

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 835c1ab30d01..66ee432dad43 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -50,6 +50,7 @@  enum big_key_op {
  * Key size for big_key data encryption
  */
 #define ENC_KEY_SIZE	16
+#define ENC_IV_SIZE	16
 
 /*
  * big_key defined keys take an arbitrary string as the description and an
@@ -70,7 +71,7 @@  struct key_type key_type_big_key = {
  * Crypto names for big_key data encryption
  */
 static const char big_key_rng_name[] = "stdrng";
-static const char big_key_alg_name[] = "ecb(aes)";
+static const char big_key_alg_name[] = "ctr(aes)";
 
 /*
  * Crypto algorithms for big_key data encryption
@@ -83,7 +84,8 @@  static struct crypto_skcipher *big_key_skcipher;
  */
 static inline int big_key_gen_enckey(u8 *key)
 {
-	return crypto_rng_get_bytes(big_key_rng, key, ENC_KEY_SIZE);
+	return crypto_rng_get_bytes(big_key_rng, key,
+				    ENC_KEY_SIZE + ENC_IV_SIZE);
 }
 
 /*
@@ -105,7 +107,8 @@  static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key)
 				      NULL, NULL);
 
 	sg_init_one(&sgio, data, datalen);
-	skcipher_request_set_crypt(req, &sgio, &sgio, datalen, NULL);
+	skcipher_request_set_crypt(req, &sgio, &sgio, datalen,
+				   key + ENC_KEY_SIZE);
 
 	if (op == BIG_KEY_ENC)
 		ret = crypto_skcipher_encrypt(req);
@@ -157,7 +160,7 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 		memset(data + datalen, 0x00, enclen - datalen);
 
 		/* generate random key */
-		enckey = kmalloc(ENC_KEY_SIZE, GFP_KERNEL);
+		enckey = kmalloc(ENC_KEY_SIZE + ENC_IV_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;