2019-06-08 13:04:14

by Sandy Harris

[permalink] [raw]
Subject: Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

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.


2019-06-08 14:41:24

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

On Sat, 8 Jun 2019 at 15:03, Sandy Harris <[email protected]> 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 :-(

2019-06-08 15:52:06

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

On Sat, 8 Jun 2019 at 16:37, Ard Biesheuvel <[email protected]> wrote:
>
> On Sat, 8 Jun 2019 at 15:03, Sandy Harris <[email protected]> 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.