| Message ID | 20190607144944.13485-1-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | move WEP implementation to skcipher interface | expand |
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 <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.
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
On Fri, 7 Jun 2019 at 22:24, Marcel Holtmann <marcel@holtmann.org> wrote: > > 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. > Ah ok, good to know. That does imply that the driver is not entirely broken, which is good news I suppose.
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 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
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
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
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.
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 :-(
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.