This is a follow-up to the discussion [0] started by regarding adding
new uses of the cipher API to dm-crypt. In particular, the discussion
was about EBOIV, which is used by BitLocker to generate IVs from byte
offsets, to be used for AES encryption in CBC mode.
The way EBOIV support is currently integrated does not restrict it at
all, which means we may paint ourselves into a corner where we are
forced to support unexpected and novel ways users have decided to
wire up EBOIV. This may become a maintenance burden going forward,
and given that EBOIV uses the same key for generating the IV via
AES encryption as the one used for the data, it may produce configurations
that are not entirely safe.
So let's restrict EBOIV to cbc(aes) (patch #1), to prevent it from
being used in arbitrary cipher cocktails, and avoid ending up with
a disproportionate maintenance burden on the crypto API side.
Patch #2 switches the IV generation to the AES library, which avoids
potential key leaks due to the use of aes-generic as the cipher used
for IV generation.
[0] https://www.redhat.com/archives/dm-devel/2019-July/msg00041.html
Ard Biesheuvel (2):
md/dm-crypt - restrict EBOIV to cbc(aes)
md/dm-crypt - switch to AES library for EBOIV
drivers/md/dm-crypt.c | 34 ++++++++------------
1 file changed, 13 insertions(+), 21 deletions(-)
--
2.17.1
Support for the EBOIV IV mode was introduced this cycle, and is
explicitly intended for interoperability with BitLocker, which
only uses it combined with AES in CBC mode.
Using EBOIV in combination with any other skcipher or aead mode
is not recommended, and so there is no need to support this.
However, the way the EBOIV support is currently integrated permits
it to be combined with other skcipher or aead modes, and once the
cat is out of the bag, we will need to support it indefinitely.
So let's restrict EBOIV to cbc(aes), and reject attempts to
instantiate it with other modes.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/md/dm-crypt.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index d5216bcc4649..a5e8d5bc1581 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -861,6 +861,13 @@ static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target *ti,
struct iv_eboiv_private *eboiv = &cc->iv_gen_private.eboiv;
struct crypto_cipher *tfm;
+ if (test_bit(CRYPT_MODE_INTEGRITY_AEAD, &cc->cipher_flags) ||
+ strcmp("cbc(aes)",
+ crypto_tfm_alg_name(crypto_skcipher_tfm(any_tfm(cc))))) {
+ ti->error = "Unsupported encryption mode for EBOIV";
+ return -EINVAL;
+ }
+
tfm = crypto_alloc_cipher(cc->cipher, 0, 0);
if (IS_ERR(tfm)) {
ti->error = "Error allocating crypto tfm for EBOIV";
--
2.17.1
The EBOIV IV mode reuses the same AES encryption key that is used for
encrypting the data, and uses it to perform a single block encryption
of the byte offset to produce the IV.
Since table-based AES is known to be susceptible to known-plaintext
attacks on the key, and given that the same key is used to encrypt
the byte offset (which is known to an attacker), we should be
careful not to permit arbitrary instantiations where the allocated
AES cipher is provided by aes-generic or other table-based drivers
that are known to be time variant and thus susceptible to this kind
of attack.
Instead, let's switch to the new AES library, which has a D-cache
footprint that is only 1/32th of the generic AES driver, and which
contains some mitigations to reduce the timing variance even further.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/md/dm-crypt.c | 33 ++++++--------------
1 file changed, 9 insertions(+), 24 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index a5e8d5bc1581..4650ab4b9415 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -27,6 +27,7 @@
#include <linux/ctype.h>
#include <asm/page.h>
#include <asm/unaligned.h>
+#include <crypto/aes.h>
#include <crypto/hash.h>
#include <crypto/md5.h>
#include <crypto/algapi.h>
@@ -121,7 +122,7 @@ struct iv_tcw_private {
};
struct iv_eboiv_private {
- struct crypto_cipher *tfm;
+ struct crypto_aes_ctx aes_ctx;
};
/*
@@ -851,16 +852,12 @@ static void crypt_iv_eboiv_dtr(struct crypt_config *cc)
{
struct iv_eboiv_private *eboiv = &cc->iv_gen_private.eboiv;
- crypto_free_cipher(eboiv->tfm);
- eboiv->tfm = NULL;
+ memset(eboiv, 0, sizeof(*eboiv));
}
static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target *ti,
const char *opts)
{
- struct iv_eboiv_private *eboiv = &cc->iv_gen_private.eboiv;
- struct crypto_cipher *tfm;
-
if (test_bit(CRYPT_MODE_INTEGRITY_AEAD, &cc->cipher_flags) ||
strcmp("cbc(aes)",
crypto_tfm_alg_name(crypto_skcipher_tfm(any_tfm(cc))))) {
@@ -868,20 +865,6 @@ static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target *ti,
return -EINVAL;
}
- tfm = crypto_alloc_cipher(cc->cipher, 0, 0);
- if (IS_ERR(tfm)) {
- ti->error = "Error allocating crypto tfm for EBOIV";
- return PTR_ERR(tfm);
- }
-
- if (crypto_cipher_blocksize(tfm) != cc->iv_size) {
- ti->error = "Block size of EBOIV cipher does "
- "not match IV size of block cipher";
- crypto_free_cipher(tfm);
- return -EINVAL;
- }
-
- eboiv->tfm = tfm;
return 0;
}
@@ -890,7 +873,7 @@ static int crypt_iv_eboiv_init(struct crypt_config *cc)
struct iv_eboiv_private *eboiv = &cc->iv_gen_private.eboiv;
int err;
- err = crypto_cipher_setkey(eboiv->tfm, cc->key, cc->key_size);
+ err = aes_expandkey(&eboiv->aes_ctx, cc->key, cc->key_size);
if (err)
return err;
@@ -899,8 +882,10 @@ static int crypt_iv_eboiv_init(struct crypt_config *cc)
static int crypt_iv_eboiv_wipe(struct crypt_config *cc)
{
- /* Called after cc->key is set to random key in crypt_wipe() */
- return crypt_iv_eboiv_init(cc);
+ struct iv_eboiv_private *eboiv = &cc->iv_gen_private.eboiv;
+
+ memset(eboiv, 0, sizeof(*eboiv));
+ return 0;
}
static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
@@ -910,7 +895,7 @@ static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
memset(iv, 0, cc->iv_size);
*(__le64 *)iv = cpu_to_le64(dmreq->iv_sector * cc->sector_size);
- crypto_cipher_encrypt_one(eboiv->tfm, iv, iv);
+ aes_encrypt(&eboiv->aes_ctx, iv, iv);
return 0;
}
--
2.17.1
On 06/08/2019 10:02, Ard Biesheuvel wrote:
> The EBOIV IV mode reuses the same AES encryption key that is used for
> encrypting the data, and uses it to perform a single block encryption
> of the byte offset to produce the IV.
>
> Since table-based AES is known to be susceptible to known-plaintext
> attacks on the key, and given that the same key is used to encrypt
> the byte offset (which is known to an attacker), we should be
> careful not to permit arbitrary instantiations where the allocated
> AES cipher is provided by aes-generic or other table-based drivers
> that are known to be time variant and thus susceptible to this kind
> of attack.
>
> Instead, let's switch to the new AES library, which has a D-cache
> footprint that is only 1/32th of the generic AES driver, and which
> contains some mitigations to reduce the timing variance even further.
NACK.
We discussed here that we will not limit combinations inside dm-crypt.
For generic crypto API, this policy should be different, but I really
do not want these IVs to be visible outside of dm-crypt.
Allowing arbitrary combinations of a cipher, mode, and IV is how dm-crypt
works since the beginning, and I really do not see the reason to change it.
This IV mode is intended to be used for accessing old BitLocker images,
so I do not care about performance much.
Thanks,
Milan
On Tue, 6 Aug 2019 at 13:43, Milan Broz <[email protected]> wrote:
>
> On 06/08/2019 10:02, Ard Biesheuvel wrote:
> > The EBOIV IV mode reuses the same AES encryption key that is used for
> > encrypting the data, and uses it to perform a single block encryption
> > of the byte offset to produce the IV.
> >
> > Since table-based AES is known to be susceptible to known-plaintext
> > attacks on the key, and given that the same key is used to encrypt
> > the byte offset (which is known to an attacker), we should be
> > careful not to permit arbitrary instantiations where the allocated
> > AES cipher is provided by aes-generic or other table-based drivers
> > that are known to be time variant and thus susceptible to this kind
> > of attack.
> >
> > Instead, let's switch to the new AES library, which has a D-cache
> > footprint that is only 1/32th of the generic AES driver, and which
> > contains some mitigations to reduce the timing variance even further.
>
> NACK.
>
> We discussed here that we will not limit combinations inside dm-crypt.
> For generic crypto API, this policy should be different, but I really
> do not want these IVs to be visible outside of dm-crypt.
>
> Allowing arbitrary combinations of a cipher, mode, and IV is how dm-crypt
> works since the beginning, and I really do not see the reason to change it.
>
> This IV mode is intended to be used for accessing old BitLocker images,
> so I do not care about performance much.
>
Apologies for being blunt, but you are basically driving home the
point I made before about why the cipher API should become internal to
the crypto subsystem.
Even though EBOIV is explicitly only intended for accessing old
BitLocker images, you prioritize non-functional properties like API
symmetry and tradition over sound cryptographic engineering practice,
even after I pointed out to you that
a) the way EBOIV uses the same symmetric key for two different
purposes is a bad idea in general, and
b) table based AES in particular is a hazard for this mode, since the
way the IV is generated is susceptible to exactly the attack that
table based AES is most criticized for.
So if you insist on supporting EBOIV in combination with arbitrary
skciphers or AEADs (or AES on systems where crypto_alloc_cipher()
produces a table based AES driver), how do you intend to mitigate
these issues?
On 06/08/2019 14:17, Ard Biesheuvel wrote:
> On Tue, 6 Aug 2019 at 13:43, Milan Broz <[email protected]> wrote:
>>
>> On 06/08/2019 10:02, Ard Biesheuvel wrote:
>>> The EBOIV IV mode reuses the same AES encryption key that is used for
>>> encrypting the data, and uses it to perform a single block encryption
>>> of the byte offset to produce the IV.
>>>
>>> Since table-based AES is known to be susceptible to known-plaintext
>>> attacks on the key, and given that the same key is used to encrypt
>>> the byte offset (which is known to an attacker), we should be
>>> careful not to permit arbitrary instantiations where the allocated
>>> AES cipher is provided by aes-generic or other table-based drivers
>>> that are known to be time variant and thus susceptible to this kind
>>> of attack.
>>>
>>> Instead, let's switch to the new AES library, which has a D-cache
>>> footprint that is only 1/32th of the generic AES driver, and which
>>> contains some mitigations to reduce the timing variance even further.
>>
>> NACK.
>>
>> We discussed here that we will not limit combinations inside dm-crypt.
>> For generic crypto API, this policy should be different, but I really
>> do not want these IVs to be visible outside of dm-crypt.
>>
>> Allowing arbitrary combinations of a cipher, mode, and IV is how dm-crypt
>> works since the beginning, and I really do not see the reason to change it.
>>
>> This IV mode is intended to be used for accessing old BitLocker images,
>> so I do not care about performance much.
>>
>
> Apologies for being blunt, but you are basically driving home the
> point I made before about why the cipher API should become internal to
> the crypto subsystem.
>
> Even though EBOIV is explicitly only intended for accessing old
> BitLocker images, you prioritize non-functional properties like API
> symmetry and tradition over sound cryptographic engineering practice,
> even after I pointed out to you that
> a) the way EBOIV uses the same symmetric key for two different
> purposes is a bad idea in general, and
> b) table based AES in particular is a hazard for this mode, since the
> way the IV is generated is susceptible to exactly the attack that
> table based AES is most criticized for.
>
> So if you insist on supporting EBOIV in combination with arbitrary
> skciphers or AEADs (or AES on systems where crypto_alloc_cipher()
> produces a table based AES driver), how do you intend to mitigate
> these issues?
I am not going to mitigate these. We will never format new devices
using these exotic configurations. And if user enforces it, there can be
a reason - or it is just stupid, like using cipher_null.
(Which is entirely insecure but very useful for testing.)
The IV concept in dm-crypt is straightforward and allows many insecure
and obscure combinations (aes-ecb-null, for example - and this is used
for millions of chipset encrypted drivers, people used it to access through
dmcrypt without the USB bridge.) The same applies for obscure cryptloop
image combinations. (I would better spent time to remove cryptoloop,
it is much worse that what we are discussing here :)
So I see no reason to spend hours and hours attacks for devices
that use crypto that is obsolete anyway (all new drives use XTS).
I would like to provide way to access data on existing, maybe obsolete and insecure,
encrypted images from foreign OSes.
But all that said is meant in the isolated context of dm-crypt driver,
if you want to provide generic API, it perhaps makes sense to enforce such a policy.
I understand you want to propose more secure ways of implementing crypto,
but then - if you decide to remove existing API I used, we can switch to something better.
(Is there something better except AES-only lib you used?)
I just disagree with adding various checks for cipher/mode/iv combinations inside dm-crypt.
It was meant to be configurable from userspace.
Milan
On Tue, 6 Aug 2019 at 16:19, Milan Broz <[email protected]> wrote:
>
> On 06/08/2019 14:17, Ard Biesheuvel wrote:
> > On Tue, 6 Aug 2019 at 13:43, Milan Broz <[email protected]> wrote:
> >>
> >> On 06/08/2019 10:02, Ard Biesheuvel wrote:
> >>> The EBOIV IV mode reuses the same AES encryption key that is used for
> >>> encrypting the data, and uses it to perform a single block encryption
> >>> of the byte offset to produce the IV.
> >>>
> >>> Since table-based AES is known to be susceptible to known-plaintext
> >>> attacks on the key, and given that the same key is used to encrypt
> >>> the byte offset (which is known to an attacker), we should be
> >>> careful not to permit arbitrary instantiations where the allocated
> >>> AES cipher is provided by aes-generic or other table-based drivers
> >>> that are known to be time variant and thus susceptible to this kind
> >>> of attack.
> >>>
> >>> Instead, let's switch to the new AES library, which has a D-cache
> >>> footprint that is only 1/32th of the generic AES driver, and which
> >>> contains some mitigations to reduce the timing variance even further.
> >>
> >> NACK.
> >>
> >> We discussed here that we will not limit combinations inside dm-crypt.
> >> For generic crypto API, this policy should be different, but I really
> >> do not want these IVs to be visible outside of dm-crypt.
> >>
> >> Allowing arbitrary combinations of a cipher, mode, and IV is how dm-crypt
> >> works since the beginning, and I really do not see the reason to change it.
> >>
> >> This IV mode is intended to be used for accessing old BitLocker images,
> >> so I do not care about performance much.
> >>
> >
> > Apologies for being blunt, but you are basically driving home the
> > point I made before about why the cipher API should become internal to
> > the crypto subsystem.
> >
> > Even though EBOIV is explicitly only intended for accessing old
> > BitLocker images, you prioritize non-functional properties like API
> > symmetry and tradition over sound cryptographic engineering practice,
> > even after I pointed out to you that
> > a) the way EBOIV uses the same symmetric key for two different
> > purposes is a bad idea in general, and
> > b) table based AES in particular is a hazard for this mode, since the
> > way the IV is generated is susceptible to exactly the attack that
> > table based AES is most criticized for.
> >
> > So if you insist on supporting EBOIV in combination with arbitrary
> > skciphers or AEADs (or AES on systems where crypto_alloc_cipher()
> > produces a table based AES driver), how do you intend to mitigate
> > these issues?
> I am not going to mitigate these. We will never format new devices
> using these exotic configurations. And if user enforces it, there can be
> a reason - or it is just stupid, like using cipher_null.
> (Which is entirely insecure but very useful for testing.)
>
> The IV concept in dm-crypt is straightforward and allows many insecure
> and obscure combinations (aes-ecb-null, for example - and this is used
> for millions of chipset encrypted drivers, people used it to access through
> dmcrypt without the USB bridge.) The same applies for obscure cryptloop
> image combinations. (I would better spent time to remove cryptoloop,
> it is much worse that what we are discussing here :)
>
> So I see no reason to spend hours and hours attacks for devices
> that use crypto that is obsolete anyway (all new drives use XTS).
>
> I would like to provide way to access data on existing, maybe obsolete and insecure,
> encrypted images from foreign OSes.
>
> But all that said is meant in the isolated context of dm-crypt driver,
> if you want to provide generic API, it perhaps makes sense to enforce such a policy.
>
> I understand you want to propose more secure ways of implementing crypto,
> but then - if you decide to remove existing API I used, we can switch to something better.
> (Is there something better except AES-only lib you used?)
>
> I just disagree with adding various checks for cipher/mode/iv combinations inside dm-crypt.
> It was meant to be configurable from userspace.
>
OK, so you are using that the dm-crypt user space tools will only
allow eboiv to be instantiated in combination with cbc(aes)? If so, do
you have a problem with patch #1 as well, which just double checks
that at the kernel level. With that patch applied, we can at least
avoid having to support every combination imaginable forever, while
the discussion takes place.
In any case, if performance does not really matter, restricting
ourselves to CBC mode has the beneficial side effect that the same tfm
used for decrypting the data could be used to encrypt the IV, and you
don't need to instantiate another cipher at all. On most systems,
cbc(aes) will be provided by a time invariant driver, and even if it
is costly to invoke this in some cases, it will at least not create
any security holes.