[RFC,0/3] move WEP implementation to skcipher interface

Message ID 20190607144944.13485-1-ard.biesheuvel@linaro.org
Headers show
Series
  • move WEP implementation to skcipher interface
Related show

Message

Ard Biesheuvel June 7, 2019, 2:49 p.m.
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

Comments

Eric Biggers June 7, 2019, 5:59 p.m. | #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
Ard Biesheuvel June 7, 2019, 6:08 p.m. | #2
On Fri, 7 Jun 2019 at 19:59, Eric Biggers <ebiggers@kernel.org> 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.
Marcel Holtmann June 7, 2019, 8:24 p.m. | #3
Hi Eric,

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


that is not unlikely, we use ecb(arc4) via AF_ALG in iwd. It is what the WiFi standard defines to be used.

Regards

Marcel
Denis Kenzior June 7, 2019, 8:45 p.m. | #4
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
Eric Biggers June 7, 2019, 9:15 p.m. | #5
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.

In any case, we can still remove the 'cipher' algorithm version as Ard is
suggesting, as well as possibly convert the in-kernel users to use an
arc4_crypt() library function and remove the hardware driver support.

- Eric
Denis Kenzior June 7, 2019, 9:28 p.m. | #6
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, ...)

Regards,
-Denis
Denis Kenzior June 7, 2019, 9:54 p.m. | #7
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
Eric Biggers June 7, 2019, 10:40 p.m. | #8
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
Denis Kenzior June 7, 2019, 10:47 p.m. | #9
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
Sandy Harris June 8, 2019, 1:03 p.m. | #10
First off, it is not clear we should implement WEP at all since it is
fatally flawed. This has been known for about a decade, there have
been at least two better algorithms added to the standards, & the only
reason anyone would need WEP today would be to connect to an old
router in an obviously insecure way.
https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html
https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html

Twenty years ago the FreeS/WAN project implemented IPsec for Linux &
deliberately did not include things like single DES which were known
to be insecure:
https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped
I think a similar policy was would be a fine idea for the kernel today
& WEP is hopelessly insecure.

> > 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 is more the case that the API does not apply to arc4, or more
generally to stream ciphers, than that "ecb(arc4) does not implement
this API correctly".

ECB (electronic code book) is a mode of operation for block ciphers
https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
Stream ciphers do not have those modes.

For that matter, not all block cipher modes use an IV. The very common
CBC mode -- the only mode used in IPsec, for example -- does, but
others including ECB do not. I do not know of any mode that ever
updates the IV. CBC uses the IV with the first block & on all other
blocks uses the ciphertext from the previous block the same way; one
might call that updating the IV I suppose, but I do not see why one
would want to.

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


As I see it, those are clearly not "he proper semantics" for a stream
cipher & the question of forcing it into them should not even arise.

One alternative would be to drop arc4. That would make sense if WEP is
the only usage & we elect to drop WEP. One could also argue the arc4
itself is insecure & should go, but I'm not sure that is accurate.
Certainly there have been some published attacks & other stream
ciphers are now generally preferrred, but I have not followed things
closely enough to know if RC$ should be considered fatally flawed.

A better choice might be to change the interface, defining a new
interface for stream ciphers and/or generalising the interface so it
works for either stream ciphers or block ciphers.
Ard Biesheuvel June 8, 2019, 2:37 p.m. | #11
On Sat, 8 Jun 2019 at 15:03, Sandy Harris <sandyinchina@gmail.com> wrote:
>

> First off, it is not clear we should implement WEP at all since it is

> fatally flawed. This has been known for about a decade, there have

> been at least two better algorithms added to the standards, & the only

> reason anyone would need WEP today would be to connect to an old

> router in an obviously insecure way.

> https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html

> https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html

>

> Twenty years ago the FreeS/WAN project implemented IPsec for Linux &

> deliberately did not include things like single DES which were known

> to be insecure:

> https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped

> I think a similar policy was would be a fine idea for the kernel today

> & WEP is hopelessly insecure.

>


It is actually pretty clear that we should implement WEP, simply
because we already do. We all know how broken it is, but that does not
mean we should be the ones policing its use. People may have good
reasons to stick with WEP in their particular use case, or maybe they
have bad reasons, but the bottom line is that it does not really
matter: if it works today, we can't just remove it.

What we can do is make the existing code less of an eyesore than it
already is, and in the context of what I want to achieve for the
crypto API, this involves moving it from the cipher API to something
else.

> > > 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 is more the case that the API does not apply to arc4, or more

> generally to stream ciphers, than that "ecb(arc4) does not implement

> this API correctly".

>

> ECB (electronic code book) is a mode of operation for block ciphers

> https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation

> Stream ciphers do not have those modes.

>


This is exactly the point Eric was making. Our skcipher abstraction
deals with stream ciphers fine, but the way the arc4 code is exposed
as ecb(arc4) and updates the key in the process makes absolutely no
sense.

> For that matter, not all block cipher modes use an IV. The very common

> CBC mode -- the only mode used in IPsec, for example -- does, but

> others including ECB do not. I do not know of any mode that ever

> updates the IV. CBC uses the IV with the first block & on all other

> blocks uses the ciphertext from the previous block the same way; one

> might call that updating the IV I suppose, but I do not see why one

> would want to.

>


If you want to split up a CBC transformation into several invocations
of the underlying API, then the last ciphertext block of the first
call serves as the IV for the next call. Arguing that we should not be
calling this an IV serves little purpose, since the code already
treats it exactly the same. In fact, our CTS template relies on this
feature as well, so a CBC implementation that does not return the last
ciphertext block in the IV buffer is broken wrt our API requirements.

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

>

> As I see it, those are clearly not "he proper semantics" for a stream

> cipher & the question of forcing it into them should not even arise.

>

> One alternative would be to drop arc4. That would make sense if WEP is

> the only usage & we elect to drop WEP. One could also argue the arc4

> itself is insecure & should go, but I'm not sure that is accurate.

> Certainly there have been some published attacks & other stream

> ciphers are now generally preferrred, but I have not followed things

> closely enough to know if RC$ should be considered fatally flawed.

>

> A better choice might be to change the interface, defining a new

> interface for stream ciphers and/or generalising the interface so it

> works for either stream ciphers or block ciphers.


Dropping WEP is out of the question, and apparently, there are
userspace dependencies on the ecb(arc4) cipher as well, so
unfortunately, we have already painted ourselves into a corner here.

skcipher works fine for block ciphers wrapped in CTR mode, and for
chacha/salsa as well, so I don't think there is a problem with the API
for other stream ciphers we care about. Calling the rc4 skcipher
'ecb(arc4)' was obviously a mistake, but it seems we're stuck with
that as well :-(
Ard Biesheuvel June 8, 2019, 3:51 p.m. | #12
On Sat, 8 Jun 2019 at 16:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> On Sat, 8 Jun 2019 at 15:03, Sandy Harris <sandyinchina@gmail.com> wrote:

> >

> > First off, it is not clear we should implement WEP at all since it is

> > fatally flawed. This has been known for about a decade, there have

> > been at least two better algorithms added to the standards, & the only

> > reason anyone would need WEP today would be to connect to an old

> > router in an obviously insecure way.

> > https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html

> > https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html

> >

> > Twenty years ago the FreeS/WAN project implemented IPsec for Linux &

> > deliberately did not include things like single DES which were known

> > to be insecure:

> > https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped

> > I think a similar policy was would be a fine idea for the kernel today

> > & WEP is hopelessly insecure.

> >

>

> It is actually pretty clear that we should implement WEP, simply

> because we already do. We all know how broken it is, but that does not

> mean we should be the ones policing its use. People may have good

> reasons to stick with WEP in their particular use case, or maybe they

> have bad reasons, but the bottom line is that it does not really

> matter: if it works today, we can't just remove it.

>

> What we can do is make the existing code less of an eyesore than it

> already is, and in the context of what I want to achieve for the

> crypto API, this involves moving it from the cipher API to something

> else.

>

> > > > 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 is more the case that the API does not apply to arc4, or more

> > generally to stream ciphers, than that "ecb(arc4) does not implement

> > this API correctly".

> >

> > ECB (electronic code book) is a mode of operation for block ciphers

> > https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation

> > Stream ciphers do not have those modes.

> >

>

> This is exactly the point Eric was making. Our skcipher abstraction

> deals with stream ciphers fine, but the way the arc4 code is exposed

> as ecb(arc4) and updates the key in the process makes absolutely no

> sense.

>

> > For that matter, not all block cipher modes use an IV. The very common

> > CBC mode -- the only mode used in IPsec, for example -- does, but

> > others including ECB do not. I do not know of any mode that ever

> > updates the IV. CBC uses the IV with the first block & on all other

> > blocks uses the ciphertext from the previous block the same way; one

> > might call that updating the IV I suppose, but I do not see why one

> > would want to.

> >

>

> If you want to split up a CBC transformation into several invocations

> of the underlying API, then the last ciphertext block of the first

> call serves as the IV for the next call. Arguing that we should not be

> calling this an IV serves little purpose, since the code already

> treats it exactly the same. In fact, our CTS template relies on this

> feature as well, so a CBC implementation that does not return the last

> ciphertext block in the IV buffer is broken wrt our API requirements.

>

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

> >

> > As I see it, those are clearly not "he proper semantics" for a stream

> > cipher & the question of forcing it into them should not even arise.

> >

> > One alternative would be to drop arc4. That would make sense if WEP is

> > the only usage & we elect to drop WEP. One could also argue the arc4

> > itself is insecure & should go, but I'm not sure that is accurate.

> > Certainly there have been some published attacks & other stream

> > ciphers are now generally preferrred, but I have not followed things

> > closely enough to know if RC$ should be considered fatally flawed.

> >

> > A better choice might be to change the interface, defining a new

> > interface for stream ciphers and/or generalising the interface so it

> > works for either stream ciphers or block ciphers.

>

> Dropping WEP is out of the question, and apparently, there are

> userspace dependencies on the ecb(arc4) cipher as well, so

> unfortunately, we have already painted ourselves into a corner here.

>

> skcipher works fine for block ciphers wrapped in CTR mode, and for

> chacha/salsa as well, so I don't think there is a problem with the API

> for other stream ciphers we care about. Calling the rc4 skcipher

> 'ecb(arc4)' was obviously a mistake, but it seems we're stuck with

> that as well :-(


As it turns out, we have other users of ecb(arc4) in the MPPE code,
the kerberos code and some realtek code in the staging tree. More
interestingly, the code this series changes was recently converted
from skcipher to cipher, while I am doing the opposite.

Given Eric's analysis that few of these users actually take advantage
of the crypto API (i.e., they all use the sync variety and hardcode
the algo name), we can simplify them to use library calls instead. The
only remaining skcipher user would be the Kerberos code, which
dynamically instantiates skciphers with a parameterized algo name.