Re: [PATCH] crypto: caam - Remove broken arc4 support

From: "Horia Geantă" <horia.geanta@nxp.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	Aymen Sghaier <aymen.sghaier@nxp.com>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	Iuliana Prodan <iuliana.prodan@nxp.com>
Subject: Re: [PATCH] crypto: caam - Remove broken arc4 support
Date: Wed, 8 Jul 2020 19:24:08 +0300	[thread overview]
Message-ID: <8e974767-7aa6-c644-8562-445a90206f47@nxp.com> (raw)
In-Reply-To: <CAMj1kXGK3v+YWd6E8zNP-tKWgq+aim7X67Ze4Bxrent4hndECw@mail.gmail.com>

On 7/6/2020 4:43 PM, Ard Biesheuvel wrote:
> On Sun, 5 Jul 2020 at 22:11, Horia Geantă <horia.geanta@nxp.com> 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@kernel.org/
>>
> 
> 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