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
Hi Alexander,
On Sun, Aug 20, 2006 at 04:23:46AM +0400, Solar Designer wrote:
> 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 :-)
> 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.
> 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.
> 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 ?
> (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
diff --git a/crypto/cipher.c b/crypto/cipher.c
index 6ab56eb..cdf650b 100644
--- a/crypto/cipher.c
+++ b/crypto/cipher.c
@@ -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:
Willy Tarreau <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
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
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
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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