Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

[Herbert Xu]

Willy Tarreau <w@1wt.eu> wrote:
> 
> 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.
 
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

[Solar Designer]

On Sun, Aug 20, 2006 at 10:04:03AM +0200, Willy Tarreau wrote:
> On Sun, Aug 20, 2006 at 04:23:46AM +0400, Solar Designer wrote:
> > 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*():

> 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:

> 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

Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

[Willy Tarreau]

On Sun, Aug 20, 2006 at 06:49:08PM +0400, Solar Designer wrote:
> On Sun, Aug 20, 2006 at 10:04:03AM +0200, Willy Tarreau wrote:
> > On Sun, Aug 20, 2006 at 04:23:46AM +0400, Solar Designer wrote:
> > > 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*():
> 
> > 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:
> 
> > 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 ?

> Alexander

Thanks,
willy

Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

[Solar Designer]

On Sun, Aug 20, 2006 at 06:13:46PM +0200, Willy Tarreau wrote:
> On Sun, Aug 20, 2006 at 06:49:08PM +0400, Solar Designer wrote:
> > 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

Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

[Herbert Xu]

On Sun, Aug 20, 2006 at 06:49:08PM +0400, Solar Designer wrote:
> 
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] cit_encrypt_iv/cit_decrypt_iv for ECB mode

[Solar Designer]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On Mon, Aug 21, 2006 at 08:58:30AM +1000, Herbert Xu wrote:
> On Sun, Aug 20, 2006 at 06:49:08PM +0400, Solar Designer wrote:
> > 
> > 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

[-- Attachment #2: linux-2.4.33-nocrypt.diff --]
[-- Type: text/plain, Size: 2508 bytes --]

diff -urp linux-2.4.33/crypto/cipher.c linux/crypto/cipher.c
--- linux-2.4.33/crypto/cipher.c	Sun Aug  8 03:26:04 2004
+++ linux/crypto/cipher.c	Tue Aug 22 09:53:35 2006
@@ -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: