cit_encrypt_iv/cit_decrypt_iv for ECB mode

PATCH In review Solar Designer 3 participants 8 messages Aug 20, 2006 – Aug 22, 2006 Crypto API

lore

Share & tools

lore

Solar Designer

author

2006-08-20 00:27 UTC #0 lore raw

PATCH 1/1 cit_encrypt_iv/cit_decrypt_iv for ECB mode

Willy and all,

Attached is a patch (extracted from 2.4.33-ow1) that works around an unfortunate problem with patch-cryptoloop-jari-2.4.22.0 (and its other revisions). I am not sure whether the problem should be worked around in the main Linux kernel like that, but this is what I did in -ow patches for now.

Basically, crypto/cipher.c: crypto_init_cipher_ops() in Linux 2.4 (I did not check 2.6 for this) did not initialize cit_encrypt_iv/cit_decrypt_iv for ECB mode at all. While IV makes no sense for ECB mode, I would think that a safer approach would be to initialize those pointers to nocrypt_iv.

patch-cryptoloop-jari-2.4.22.0 calls cit_encrypt_iv/cit_decrypt_iv directly, ignoring their return value. Thus, when these pointers are not initialized (as they are not in vanilla Linux 2.4.33) and we request ECB mode encryption via cryptoloop (a bad idea, but anyway), the kernel most likely Oopses. When these pointers are initialized to nocrypt_iv (due to a "correct" patch), there's no Oops, but the kernel leaks uninitialized memory contents via the loop device (that's because patch-cryptoloop-jari-2.4.22.0 ignores the -ENOSYS returns). Neither behavior is any good.

The attached patch actually defines ecb_encrypt_iv() and ecb_decrypt_iv() functions that perform ECB encryption/decryption ignoring the IV, yet return -ENOSYS (just like nocrypt_iv would). The result is no more Oopses and no infoleaks either.

(Yes, I understand that ECB mode should be avoided and that this cryptoloop patch does not address watermarking. But the security of block device encryption offered by cryptoloop is irrelevant to the point that I am making.)

Opinions are welcome.

Thanks,

Alexander

crypto/cipher.c +23 -0

diff -urpPX nopatch linux-2.4.33/crypto/cipher.c linux/crypto/cipher.c
@@ -147,6 +147,15 @@ static int ecb_encrypt(struct crypto_tfm
 	             ecb_process, 1, NULL);
 }
 
+static int ecb_encrypt_iv(struct crypto_tfm *tfm,
+			  struct scatterlist *dst,
+			  struct scatterlist *src,
+			  unsigned int nbytes, u8 *iv)
+{
+	ecb_encrypt(tfm, dst, src, nbytes);
+	return -ENOSYS;
+}
+
 static int ecb_decrypt(struct crypto_tfm *tfm,
                        struct scatterlist *dst,
                        struct scatterlist *src,

@@ -157,6 +166,15 @@ static int ecb_decrypt(struct crypto_tfm
 	             ecb_process, 1, NULL);
 }
 
+static int ecb_decrypt_iv(struct crypto_tfm *tfm,
+			  struct scatterlist *dst,
+			  struct scatterlist *src,
+			  unsigned int nbytes, u8 *iv)
+{
+	ecb_decrypt(tfm, dst, src, nbytes);
+	return -ENOSYS;
+}
+
 static int cbc_encrypt(struct crypto_tfm *tfm,
                        struct scatterlist *dst,
                        struct scatterlist *src,

@@ -235,6 +253,11 @@ int crypto_init_cipher_ops(struct crypto
 	case CRYPTO_TFM_MODE_ECB:
 		ops->cit_encrypt = ecb_encrypt;
 		ops->cit_decrypt = ecb_decrypt;
+/* These should have been nocrypt_iv, but patch-cryptoloop-jari-2.4.22.0
+ * (and its other revisions) directly calls the *_iv() functions even in
+ * ECB mode and ignores their return value. */
+		ops->cit_encrypt_iv = ecb_encrypt_iv;
+		ops->cit_decrypt_iv = ecb_decrypt_iv;
 		break;
 		
 	case CRYPTO_TFM_MODE_CBC:

Willy Tarreau

2006-08-20 08:04 UTC #1 lore raw

Hi Alexander,

Quoted 7 lines · Solar Designer

Willy and all,
Attached is a patch (extracted from 2.4.33-ow1) that works around an unfortunate problem with patch-cryptoloop-jari-2.4.22.0 (and its other revisions). I am not sure whether the problem should be worked around in the main Linux kernel like that, but this is what I did in -ow patches for now.

You definitely provide interesting cases :-)

Quoted 5 lines

Basically, crypto/cipher.c: crypto_init_cipher_ops() in Linux 2.4 (I did not check 2.6 for this) did not initialize cit_encrypt_iv/cit_decrypt_iv for ECB mode at all. While IV makes no sense for ECB mode, I would think that a safer approach would be to initialize those pointers to nocrypt_iv.

That's what I thought after reading the code too. BTW, 2.6 does not initialize the pointers either.

Quoted 9 lines

patch-cryptoloop-jari-2.4.22.0 calls cit_encrypt_iv/cit_decrypt_iv directly, ignoring their return value. Thus, when these pointers are not initialized (as they are not in vanilla Linux 2.4.33) and we request ECB mode encryption via cryptoloop (a bad idea, but anyway), the kernel most likely Oopses. When these pointers are initialized to nocrypt_iv (due to a "correct" patch), there's no Oops, but the kernel leaks uninitialized memory contents via the loop device (that's because patch-cryptoloop-jari-2.4.22.0 ignores the -ENOSYS returns). Neither behavior is any good.

Indeed, that's bad too.

Quoted 4 lines

The attached patch actually defines ecb_encrypt_iv() and ecb_decrypt_iv() functions that perform ECB encryption/decryption ignoring the IV, yet return -ENOSYS (just like nocrypt_iv would). The result is no more Oopses and no infoleaks either.

Can the cryptoloop patch use CRYPTO_TFM_MODE_CFB or CRYPTO_TFM_MODE_CTR and so be redirected to nocrypt() which will leave uninitialized memory too ?

Quoted 6 lines

(Yes, I understand that ECB mode should be avoided and that this cryptoloop patch does not address watermarking. But the security of block device encryption offered by cryptoloop is irrelevant to the point that I am making.)
Opinions are welcome.

I wonder whether we shouldn't consider that those functions must at least clear the memory area that was submitted to them, such as proposed below. It would also fix the problem for potential other users. I don't think we need to check whether dst is valid given the small amount of tests performed in crypt().

Opinions are welcome too.
Willy

crypto/cipher.c +8 -0

index 6ab56eb..cdf650b 100644
@@ -202,6 +202,9 @@ static int nocrypt(struct crypto_tfm *tf
                    struct scatterlist *src,
 		   unsigned int nbytes)
 {
+	/* do not leak uninitialized data in the return buffer */
+	if (nbytes)
+		memset(dst, 0, nbytes);
 	return -ENOSYS;
 }

@@ -210,6 +213,9 @@ static int nocrypt_iv(struct crypto_tfm 
                       struct scatterlist *src,
                       unsigned int nbytes, u8 *iv)
 {
+	/* do not leak uninitialized data in the return buffer */
+	if (nbytes)
+		memset(dst, 0, nbytes);
 	return -ENOSYS;
 }

@@ -235,6 +241,8 @@ int crypto_init_cipher_ops(struct crypto
 	case CRYPTO_TFM_MODE_ECB:
 		ops->cit_encrypt = ecb_encrypt;
 		ops->cit_decrypt = ecb_decrypt;
+		ops->cit_encrypt_iv = nocrypt_iv;
+		ops->cit_decrypt_iv = nocrypt_iv;
 		break;
 		
 	case CRYPTO_TFM_MODE_CBC:

Herbert Xu

maintainer

2006-08-20 11:20 UTC #2 lore raw

Quoted 3 lines · Willy Tarreau

That's what I thought after reading the code too. BTW, 2.6 does not initialize the pointers either.

This has been changed in the cryptodev-2.6 tree:

http://www.kernel.org/git/?p=linux/kernel/git/herbert/cryptodev-2.6.git;a=commitdiff;h=310d6a0c14eda153869adaf74e69dbd1a1256e7f

[CRYPTO] cipher: Removed special IV checks for ECB

This patch makes IV operations on ECB fail through nocrypt_iv rather than calling BUG(). This is needed to generalise CBC/ECB using the template mechanism.

In fact with the new block cipher type calling the IV-specific functions on ECB will work in the same way as the IV-less functions. This makes sense because the IV length is simply zero.

Quoted 5 lines

I wonder whether we shouldn't consider that those functions must at least clear the memory area that was submitted to them, such as proposed below. It would also fix the problem for potential other users. I don't think we need to check whether dst is valid given the small amount of tests performed in crypt().

If the user is ignoring the error value here then you're in serious trouble anyway since they've just lost all their data.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Solar Designer

author

2006-08-20 14:53 UTC #3 lore raw

In reply to Willy Tarreau

Quoted 9 lines · Willy Tarreau

Quoted 4 lines · Solar Designer
The attached patch actually defines ecb_encrypt_iv() and ecb_decrypt_iv() functions that perform ECB encryption/decryption ignoring the IV, yet return -ENOSYS (just like nocrypt_iv would). The result is no more Oopses and no infoleaks either.
Can the cryptoloop patch use CRYPTO_TFM_MODE_CFB or CRYPTO_TFM_MODE_CTR and so be redirected to nocrypt() which will leave uninitialized memory too ?

At least patch-cryptoloop-jari-2.4.22.0 in particular will only do CBC (default, preferred) or ECB (if requested); it won't attempt to use CFB or CTR.

Regarding nocrypt*():

Quoted 4 lines

I wonder whether we shouldn't consider that those functions must at least clear the memory area that was submitted to them, such as proposed below. It would also fix the problem for potential other users.

This makes sense to me, although it is not perfect as Herbert has correctly pointed out:

Quoted 2 lines

If the user is ignoring the error value here then you're in serious trouble anyway since they've just lost all their data.

Can we maybe define working but IV-ignoring functions for ECB (like I did), but use memory-clearing nocrypt*() for CFB and CTR (as long as these are not supported)? Of course, all of these will return -ENOSYS.

Alexander

Willy Tarreau

2006-08-20 16:14 UTC #4 lore raw

Quoted 25 lines · Solar Designer

Quoted 9 lines · Willy Tarreau
Quoted 4 lines · Solar Designer
The attached patch actually defines ecb_encrypt_iv() and ecb_decrypt_iv() functions that perform ECB encryption/decryption ignoring the IV, yet return -ENOSYS (just like nocrypt_iv would). The result is no more Oopses and no infoleaks either.
Can the cryptoloop patch use CRYPTO_TFM_MODE_CFB or CRYPTO_TFM_MODE_CTR and so be redirected to nocrypt() which will leave uninitialized memory too ?
At least patch-cryptoloop-jari-2.4.22.0 in particular will only do CBC (default, preferred) or ECB (if requested); it won't attempt to use CFB or CTR.
Regarding nocrypt*():
Quoted 4 lines
I wonder whether we shouldn't consider that those functions must at least clear the memory area that was submitted to them, such as proposed below. It would also fix the problem for potential other users.
This makes sense to me, although it is not perfect as Herbert has correctly pointed out:

Quoted 6 lines

Quoted 2 lines
If the user is ignoring the error value here then you're in serious trouble anyway since they've just lost all their data.
Can we maybe define working but IV-ignoring functions for ECB (like I did), but use memory-clearing nocrypt*() for CFB and CTR (as long as these are not supported)? Of course, all of these will return -ENOSYS.

I thought we would not have to protect users from shooting themselves in the foot (right now they get an oops). But I agree that the cost of protecting them is close to zero so we probably should do it. If Herbert is OK, do you care to provide a new patch ?

Quoted 1 line

Alexander

Thanks,
willy

Solar Designer

author

2006-08-20 17:02 UTC #5 lore raw

Quoted 9 lines · Willy Tarreau

Quoted 3 lines · Solar Designer
Can we maybe define working but IV-ignoring functions for ECB (like I did), but use memory-clearing nocrypt*() for CFB and CTR (as long as these are not supported)? Of course, all of these will return -ENOSYS.
I thought we would not have to protect users from shooting themselves in the foot (right now they get an oops). But I agree that the cost of protecting them is close to zero so we probably should do it. If Herbert is OK, do you care to provide a new patch ?

Yes, if the above proposal is OK with Herbert, I will provide a new patch for 2.4.

Thanks,

Alexander

Herbert Xu

maintainer

2006-08-20 22:58 UTC #6 lore raw

In reply to Solar Designer

Quoted 4 lines · Solar Designer

Can we maybe define working but IV-ignoring functions for ECB (like I did), but use memory-clearing nocrypt*() for CFB and CTR (as long as these are not supported)? Of course, all of these will return -ENOSYS.

In cryptodev-2.6, with block ciphers you can no longer select CFB/CTR until someone writes support for them so this is no longer an issue.

For 2.4, I don't really mind either way what nocrypt does.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Solar Designer

author

2006-08-22 06:32 UTC #7 lore raw

Quoted 10 lines · Herbert Xu

Quoted 4 lines · Solar Designer
Can we maybe define working but IV-ignoring functions for ECB (like I did), but use memory-clearing nocrypt*() for CFB and CTR (as long as these are not supported)? Of course, all of these will return -ENOSYS.
In cryptodev-2.6, with block ciphers you can no longer select CFB/CTR until someone writes support for them so this is no longer an issue.
For 2.4, I don't really mind either way what nocrypt does.

OK, I've merged Willy's suggestion for the memset()s into the patch that I had submitted previously. The resulting patch is attached.

Alexander

crypto/cipher.c +33 -0

diff -urp linux-2.4.33/crypto/cipher.c linux/crypto/cipher.c
@@ -147,6 +147,15 @@ static int ecb_encrypt(struct crypto_tfm
 	             ecb_process, 1, NULL);
 }
 
+static int ecb_encrypt_iv(struct crypto_tfm *tfm,
+			  struct scatterlist *dst,
+			  struct scatterlist *src,
+			  unsigned int nbytes, u8 *iv)
+{
+	ecb_encrypt(tfm, dst, src, nbytes);
+	return -ENOSYS;
+}
+
 static int ecb_decrypt(struct crypto_tfm *tfm,
                        struct scatterlist *dst,
                        struct scatterlist *src,

@@ -157,6 +166,15 @@ static int ecb_decrypt(struct crypto_tfm
 	             ecb_process, 1, NULL);
 }
 
+static int ecb_decrypt_iv(struct crypto_tfm *tfm,
+			  struct scatterlist *dst,
+			  struct scatterlist *src,
+			  unsigned int nbytes, u8 *iv)
+{
+	ecb_decrypt(tfm, dst, src, nbytes);
+	return -ENOSYS;
+}
+
 static int cbc_encrypt(struct crypto_tfm *tfm,
                        struct scatterlist *dst,
                        struct scatterlist *src,

@@ -197,11 +215,20 @@ static int cbc_decrypt_iv(struct crypto_
 	             cbc_process, 0, iv);
 }
 
+/*
+ * nocrypt*() zeroize the destination buffer to make sure we don't leak
+ * uninitialized memory contents if the caller ignores the return value.
+ * This is bad since the data in the source buffer is unused and may be
+ * lost, but an infoleak would be even worse.  The performance cost of
+ * memset() is irrelevant since a well-behaved caller would not bump into
+ * the error repeatedly.
+ */
 static int nocrypt(struct crypto_tfm *tfm,
                    struct scatterlist *dst,
                    struct scatterlist *src,
 		   unsigned int nbytes)
 {
+	memset(dst, 0, nbytes);
 	return -ENOSYS;
 }

@@ -210,6 +237,7 @@ static int nocrypt_iv(struct crypto_tfm 
                       struct scatterlist *src,
                       unsigned int nbytes, u8 *iv)
 {
+	memset(dst, 0, nbytes);
 	return -ENOSYS;
 }

@@ -235,6 +263,11 @@ int crypto_init_cipher_ops(struct crypto
 	case CRYPTO_TFM_MODE_ECB:
 		ops->cit_encrypt = ecb_encrypt;
 		ops->cit_decrypt = ecb_decrypt;
+/* These should have been nocrypt_iv, but patch-cryptoloop-jari-2.4.22.0
+ * (and its other revisions) directly calls the *_iv() functions even in
+ * ECB mode and ignores their return value. */
+		ops->cit_encrypt_iv = ecb_encrypt_iv;
+		ops->cit_decrypt_iv = ecb_decrypt_iv;
 		break;
 		
 	case CRYPTO_TFM_MODE_CBC: