One of the issues that I would like to see addressed in the crypto API
is they way the cipher abstraction is used. In general, a cipher should
never be used directly, and so it would be much better to clean up the
existing uses of ciphers outside of the crypto subsystem itself, so that
we can make the cipher abstraction part of the internal API, only to
be used by templates or crypto drivers that require them as a callback.
As a first step, this series moves all users of the 'arc4' cipher to
the ecb(arc4) skcipher, which happens to be implemented by the same
driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
actually evaluates to 1.
Next step would be to switch the users of the 'des' and 'aes' ciphers
to other interfaces that are more appropriate, either ecb(...) or a
library interface, which may be more appropriate in some cases. In any
case, the end result should be that ciphers are no longer used outside
of crypto/ and drivers/crypto/
This series is presented as an RFC, since I am mostly interested in
discussing the above, but I prefer to do so in the context of actual
patches rather than an abstract discussion.
Ard Biesheuvel (3):
net/mac80211: switch to skcipher interface for arc4
lib80211/tkip: switch to skcipher interface for arc4
lib80211/wep: switch to skcipher interface for arc4
net/mac80211/ieee80211_i.h | 6 +-
net/mac80211/key.h | 1 +
net/mac80211/tkip.c | 8 +-
net/mac80211/tkip.h | 4 +-
net/mac80211/wep.c | 81 +++++++++++++++-----
net/mac80211/wep.h | 4 +-
net/mac80211/wpa.c | 4 +-
net/wireless/lib80211_crypt_tkip.c | 61 ++++++++++-----
net/wireless/lib80211_crypt_wep.c | 52 +++++++++----
9 files changed, 153 insertions(+), 68 deletions(-)
--
2.20.1
On Fri, Jun 07, 2019 at 04:49:41PM +0200, Ard Biesheuvel wrote:
> One of the issues that I would like to see addressed in the crypto API
> is they way the cipher abstraction is used. In general, a cipher should
> never be used directly, and so it would be much better to clean up the
> existing uses of ciphers outside of the crypto subsystem itself, so that
> we can make the cipher abstraction part of the internal API, only to
> be used by templates or crypto drivers that require them as a callback.
>
> As a first step, this series moves all users of the 'arc4' cipher to
> the ecb(arc4) skcipher, which happens to be implemented by the same
> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> actually evaluates to 1.
>
> Next step would be to switch the users of the 'des' and 'aes' ciphers
> to other interfaces that are more appropriate, either ecb(...) or a
> library interface, which may be more appropriate in some cases. In any
> case, the end result should be that ciphers are no longer used outside
> of crypto/ and drivers/crypto/
>
> This series is presented as an RFC, since I am mostly interested in
> discussing the above, but I prefer to do so in the context of actual
> patches rather than an abstract discussion.
>
> Ard Biesheuvel (3):
> net/mac80211: switch to skcipher interface for arc4
> lib80211/tkip: switch to skcipher interface for arc4
> lib80211/wep: switch to skcipher interface for arc4
>
The way the crypto API exposes ARC4 is definitely broken. It treats it as a
block cipher (with a block size of 1 byte...), when it's actually a stream
cipher. Also, it violates the API by modifying the key during each encryption.
Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
using, and the users call it on virtual addresses, perhaps we should instead
remove it from the crypto API and provide a library function arc4_crypt()? We'd
lose support for ARC4 in three hardware drivers, but are there real users who
really are using ARC4 and need those to get acceptable performance? Note that
they aren't being used in the cases where the 'cipher' API is currently being
used, so it would only be the current 'skcipher' users that might matter.
Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
seems unlikely...
As for removing the "cipher" API entirely, we'd have to consider how to convert
all the current users, not just ARC4, so that would be a somewhat different
discussion. How do you propose to handle dm-crypt and fscrypt which use the
cipher API to do ESSIV?
- Eric
On Fri, 7 Jun 2019 at 19:59, Eric Biggers <[email protected]> wrote:
>
> On Fri, Jun 07, 2019 at 04:49:41PM +0200, Ard Biesheuvel wrote:
> > One of the issues that I would like to see addressed in the crypto API
> > is they way the cipher abstraction is used. In general, a cipher should
> > never be used directly, and so it would be much better to clean up the
> > existing uses of ciphers outside of the crypto subsystem itself, so that
> > we can make the cipher abstraction part of the internal API, only to
> > be used by templates or crypto drivers that require them as a callback.
> >
> > As a first step, this series moves all users of the 'arc4' cipher to
> > the ecb(arc4) skcipher, which happens to be implemented by the same
> > driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> > actually evaluates to 1.
> >
> > Next step would be to switch the users of the 'des' and 'aes' ciphers
> > to other interfaces that are more appropriate, either ecb(...) or a
> > library interface, which may be more appropriate in some cases. In any
> > case, the end result should be that ciphers are no longer used outside
> > of crypto/ and drivers/crypto/
> >
> > This series is presented as an RFC, since I am mostly interested in
> > discussing the above, but I prefer to do so in the context of actual
> > patches rather than an abstract discussion.
> >
> > Ard Biesheuvel (3):
> > net/mac80211: switch to skcipher interface for arc4
> > lib80211/tkip: switch to skcipher interface for arc4
> > lib80211/wep: switch to skcipher interface for arc4
> >
>
> The way the crypto API exposes ARC4 is definitely broken. It treats it as a
> block cipher (with a block size of 1 byte...), when it's actually a stream
> cipher. Also, it violates the API by modifying the key during each encryption.
>
> Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> using, and the users call it on virtual addresses, perhaps we should instead
> remove it from the crypto API and provide a library function arc4_crypt()? We'd
> lose support for ARC4 in three hardware drivers, but are there real users who
> really are using ARC4 and need those to get acceptable performance? Note that
> they aren't being used in the cases where the 'cipher' API is currently being
> used, so it would only be the current 'skcipher' users that might matter.
>
In fact, this is what I started out doing, i.e., factor out the core
arc4 code into crypto/arc4_lib.c, and make the existing driver a thin
wrapper around it, so that we can invoke the library directly.
> Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> seems unlikely...
>
Yes, that seems highly unlikely.
> As for removing the "cipher" API entirely, we'd have to consider how to convert
> all the current users, not just ARC4, so that would be a somewhat different
> discussion. How do you propose to handle dm-crypt and fscrypt which use the
> cipher API to do ESSIV?
>
Without having looked in too much detail, ESSIV seems like something
that could be moved into the crypto subsystem, and be implemented as a
template.
Hi Ard,
>
> Ah ok, good to know. That does imply that the driver is not entirely
> broken, which is good news I suppose.
>
Not entirely, but we did have to resort to using multiple sockets,
otherwise parallel encrypt/decrypt operations on the socket would result
in invalid behavior. Probably due to the issue Eric already pointed out.
No such issue with any other ciphers that we use.
Regards,
-Denis
On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
> Hi Eric,
>
> On 06/07/2019 04:15 PM, Eric Biggers wrote:
> > On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> > > Hi Ard,
> > >
> > > >
> > > > Ah ok, good to know. That does imply that the driver is not entirely
> > > > broken, which is good news I suppose.
> > > >
> > >
> > > Not entirely, but we did have to resort to using multiple sockets, otherwise
> > > parallel encrypt/decrypt operations on the socket would result in invalid
> > > behavior. Probably due to the issue Eric already pointed out.
> > >
> > > No such issue with any other ciphers that we use.
> > >
> > > Regards,
> > > -Denis
> >
> > Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
> > we can't fix its name to be just "arc4". It's odd that someone would choose to
> > use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> >
> > Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
> > modern stream ciphers use a key + IV. To comply with the crypto API it would
> > have to copy the key to a stack buffer for each encryption/decryption. But it
> > doesn't; it just updates the key instead, making it non thread safe. If users
> > are actually relying on that, we'll have to settle for adding a mutex instead.
>
> Well the issue isn't even about being thread safe. We run a single thread
> in iwd. The details are a bit fuzzy now due to time elapsed, but if I
> recall correctly, even behavior like:
>
> fd = socket();
> bind(fd, ecb(arc4));
> setsockopt(fd, ...key...);
>
> sendmsg(fd, OP_ENCRYPT, ...);
> sendmsg(fd, OP_DECRYPT, ...);
> sendmsg(fd, OP_ENCRYPT, ...);
>
> would produce different (incorrect) encrypted results compared to
>
> sendmsg(fd, OP_ENCRYPT, ...)
> sendmsg(fd, OP_ENCRYPT, ...)
>
That's because currently each operation uses the next bytes from the keystream,
and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
There's no difference between ARC4 encryption and decryption; both just XOR the
keystream with the data. Are you saying you expected each encryption to be a
continuation of the previous encryption, but decryptions to be independent?
- Eric
Hi Eric,
On 06/07/2019 04:41 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
>> Hi Eric,
>>
>> On 06/07/2019 04:15 PM, Eric Biggers wrote:
>>> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>>>> Hi Ard,
>>>>
>>>>>
>>>>> Ah ok, good to know. That does imply that the driver is not entirely
>>>>> broken, which is good news I suppose.
>>>>>
>>>>
>>>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>>>> parallel encrypt/decrypt operations on the socket would result in invalid
>>>> behavior. Probably due to the issue Eric already pointed out.
>>>>
>>>> No such issue with any other ciphers that we use.
>>>>
>>>> Regards,
>>>> -Denis
>>>
>>> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
>>> we can't fix its name to be just "arc4". It's odd that someone would choose to
>>> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>>>
>>> Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
>>> modern stream ciphers use a key + IV. To comply with the crypto API it would
>>> have to copy the key to a stack buffer for each encryption/decryption. But it
>>> doesn't; it just updates the key instead, making it non thread safe. If users
>>> are actually relying on that, we'll have to settle for adding a mutex instead.
>>
>> Well the issue isn't even about being thread safe. We run a single thread
>> in iwd. The details are a bit fuzzy now due to time elapsed, but if I
>> recall correctly, even behavior like:
>>
>> fd = socket();
>> bind(fd, ecb(arc4));
>> setsockopt(fd, ...key...);
>>
>> sendmsg(fd, OP_ENCRYPT, ...);
>> sendmsg(fd, OP_DECRYPT, ...);
>> sendmsg(fd, OP_ENCRYPT, ...);
>>
>> would produce different (incorrect) encrypted results compared to
>>
>> sendmsg(fd, OP_ENCRYPT, ...)
>> sendmsg(fd, OP_ENCRYPT, ...)
>>
>
> That's because currently each operation uses the next bytes from the keystream,
> and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
> There's no difference between ARC4 encryption and decryption; both just XOR the
> keystream with the data. Are you saying you expected each encryption to be a
> continuation of the previous encryption, but decryptions to be independent?
>
From a userspace / api perspective, yes I would have expected the
encrypt and decrypt to work independently. No biggie now, but I
remember being surprised when this bit me as no other cipher had this
behavior. E.g. interleaving of operations seemed to only affect arc4
results.
Are the exact semantics spelled out somewhere?
Regards,
-Denis
On Fri, Jun 07, 2019 at 04:54:04PM -0500, Denis Kenzior wrote:
> Hi Eric,
>
> On 06/07/2019 04:41 PM, Eric Biggers wrote:
> > On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
> > > Hi Eric,
> > >
> > > On 06/07/2019 04:15 PM, Eric Biggers wrote:
> > > > On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> > > > > Hi Ard,
> > > > >
> > > > > >
> > > > > > Ah ok, good to know. That does imply that the driver is not entirely
> > > > > > broken, which is good news I suppose.
> > > > > >
> > > > >
> > > > > Not entirely, but we did have to resort to using multiple sockets, otherwise
> > > > > parallel encrypt/decrypt operations on the socket would result in invalid
> > > > > behavior. Probably due to the issue Eric already pointed out.
> > > > >
> > > > > No such issue with any other ciphers that we use.
> > > > >
> > > > > Regards,
> > > > > -Denis
> > > >
> > > > Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
> > > > we can't fix its name to be just "arc4". It's odd that someone would choose to
> > > > use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> > > >
> > > > Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
> > > > modern stream ciphers use a key + IV. To comply with the crypto API it would
> > > > have to copy the key to a stack buffer for each encryption/decryption. But it
> > > > doesn't; it just updates the key instead, making it non thread safe. If users
> > > > are actually relying on that, we'll have to settle for adding a mutex instead.
> > >
> > > Well the issue isn't even about being thread safe. We run a single thread
> > > in iwd. The details are a bit fuzzy now due to time elapsed, but if I
> > > recall correctly, even behavior like:
> > >
> > > fd = socket();
> > > bind(fd, ecb(arc4));
> > > setsockopt(fd, ...key...);
> > >
> > > sendmsg(fd, OP_ENCRYPT, ...);
> > > sendmsg(fd, OP_DECRYPT, ...);
> > > sendmsg(fd, OP_ENCRYPT, ...);
> > >
> > > would produce different (incorrect) encrypted results compared to
> > >
> > > sendmsg(fd, OP_ENCRYPT, ...)
> > > sendmsg(fd, OP_ENCRYPT, ...)
> > >
> >
> > That's because currently each operation uses the next bytes from the keystream,
> > and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
> > There's no difference between ARC4 encryption and decryption; both just XOR the
> > keystream with the data. Are you saying you expected each encryption to be a
> > continuation of the previous encryption, but decryptions to be independent?
> >
>
> From a userspace / api perspective, yes I would have expected the encrypt
> and decrypt to work independently. No biggie now, but I remember being
> surprised when this bit me as no other cipher had this behavior. E.g.
> interleaving of operations seemed to only affect arc4 results.
>
> Are the exact semantics spelled out somewhere?
>
For all other skcipher algorithms, every operation is independent and depends
only on the key which was set previously on the algorithm socket, plus the IV
provided for the operation. There is no way to perform a single encryption or
decryption incrementally in multiple parts, unless the algorithm supports it
naturally by updating the IV (e.g. CBC mode).
As I am attempting to explain, ecb(arc4) does not implement this API correctly
because it updates the *key* after each operation, not the IV. I doubt this is
documented anywhere, but this can only be changed if people aren't relying on it
already.
- Eric
Hi Eric,
On 06/07/2019 05:40 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 04:54:04PM -0500, Denis Kenzior wrote:
>> Hi Eric,
>>
>> On 06/07/2019 04:41 PM, Eric Biggers wrote:
>>> On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
>>>> Hi Eric,
>>>>
>>>> On 06/07/2019 04:15 PM, Eric Biggers wrote:
>>>>> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>>
>>>>>>> Ah ok, good to know. That does imply that the driver is not entirely
>>>>>>> broken, which is good news I suppose.
>>>>>>>
>>>>>>
>>>>>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>>>>>> parallel encrypt/decrypt operations on the socket would result in invalid
>>>>>> behavior. Probably due to the issue Eric already pointed out.
>>>>>>
>>>>>> No such issue with any other ciphers that we use.
>>>>>>
>>>>>> Regards,
>>>>>> -Denis
>>>>>
>>>>> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
>>>>> we can't fix its name to be just "arc4". It's odd that someone would choose to
>>>>> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>>>>>
>>>>> Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
>>>>> modern stream ciphers use a key + IV. To comply with the crypto API it would
>>>>> have to copy the key to a stack buffer for each encryption/decryption. But it
>>>>> doesn't; it just updates the key instead, making it non thread safe. If users
>>>>> are actually relying on that, we'll have to settle for adding a mutex instead.
>>>>
>>>> Well the issue isn't even about being thread safe. We run a single thread
>>>> in iwd. The details are a bit fuzzy now due to time elapsed, but if I
>>>> recall correctly, even behavior like:
>>>>
>>>> fd = socket();
>>>> bind(fd, ecb(arc4));
>>>> setsockopt(fd, ...key...);
>>>>
>>>> sendmsg(fd, OP_ENCRYPT, ...);
>>>> sendmsg(fd, OP_DECRYPT, ...);
>>>> sendmsg(fd, OP_ENCRYPT, ...);
>>>>
>>>> would produce different (incorrect) encrypted results compared to
>>>>
>>>> sendmsg(fd, OP_ENCRYPT, ...)
>>>> sendmsg(fd, OP_ENCRYPT, ...)
>>>>
>>>
>>> That's because currently each operation uses the next bytes from the keystream,
>>> and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
>>> There's no difference between ARC4 encryption and decryption; both just XOR the
>>> keystream with the data. Are you saying you expected each encryption to be a
>>> continuation of the previous encryption, but decryptions to be independent?
>>>
>>
>> From a userspace / api perspective, yes I would have expected the encrypt
>> and decrypt to work independently. No biggie now, but I remember being
>> surprised when this bit me as no other cipher had this behavior. E.g.
>> interleaving of operations seemed to only affect arc4 results.
>>
>> Are the exact semantics spelled out somewhere?
>>
>
> For all other skcipher algorithms, every operation is independent and depends
> only on the key which was set previously on the algorithm socket, plus the IV
> provided for the operation. There is no way to perform a single encryption or
> decryption incrementally in multiple parts, unless the algorithm supports it
> naturally by updating the IV (e.g. CBC mode).
Right, that is what I thought.
>
> As I am attempting to explain, ecb(arc4) does not implement this API correctly
> because it updates the *key* after each operation, not the IV. I doubt this is
> documented anywhere, but this can only be changed if people aren't relying on it
> already.
It sounds to me like it was broken and should be fixed. So our vote /
preference is to have ARC4 fixed to follow the proper semantics. We can
deal with the kernel behavioral change on our end easily enough; the
required workarounds are the worse evil.
Regards,
-Denis