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