On 7/6/2020 4:43 PM, Ard Biesheuvel wrote: > On Sun, 5 Jul 2020 at 22:11, Horia Geantă <horia.geanta@xxxxxxx> wrote: >> >> On 7/2/2020 7:36 AM, Herbert Xu wrote: >>> The arc4 algorithm requires storing state in the request context >>> in order to allow more than one encrypt/decrypt operation. As this >>> driver does not seem to do that, it means that using it for more >>> than one operation is broken. >>> >> The fact that smth. is broken doesn't necessarily means it has to be removed. >> >> Looking at the HW capabilities, I am sure the implementation could be >> modified to save/restore the internal state to/from the request context. >> >> Anyhow I would like to know if only the correctness is being debated, >> or this patch should be dealt with in the larger context of >> removing crypto API based ecb(arc4) altogether: >> [RFC PATCH 0/7] crypto: get rid of ecb(arc4) >> https://lore.kernel.org/linux-crypto/20200702101947.682-1-ardb@xxxxxxxxxx/ >> > > The problem with 'fixing' ecb(arc4) is that it will make it less > secure than it already is. For instance, imagine two peers using the > generic ecb(arc4) implementation, and using a pair of skcipher TFMs to > en/decrypt the communication between them, similar to how the WEP code > works today. if we 'fix' the implementation, every request will be > served from the same initial state (including the key), and therefore > reuse the same keystream, resulting in catastrophic failure. (Of > course, the code should set a different key for each request anyway, > but failure to do so does not result in the same security fail with > the current implementation) > > So the problem is really that the lack of a key vs IV distinction in > ARC4 means that it does not fit the skcipher model cleanly, and > issuing more than a single request without an intermediate setkey() > operation should be forbidden in any case. > > The reason I suggested removing it is that we really have no use for > it in the kernel, and the only known af_alg users are dubious as well, > and so we'd be much better off simply getting rid of ecb(arc4) > entirely, not because any of the implementations are flawed, but > simply because I don't think we should waste more time on it in > general. > Thanks Ard. My understanding was along these lines, just wanted to make sure. I think the commit message should be updated to reflect this logic: indeed, caam's implementation of ecb(arc4) is broken, but instead of fixing it, crypto API-based ecb(arc4) is removed completely from the kernel (hence from caam driver) due to skcipher limitations in terms of handling the keystream. Horia
