crypto: gcm - restrict assoclen for rfc4543
diff mbox series

Message ID 1563460984-24593-1-git-send-email-iuliana.prodan@nxp.com
State Superseded
Headers show
Series
  • crypto: gcm - restrict assoclen for rfc4543
Related show

Commit Message

Iuliana Prodan July 18, 2019, 2:43 p.m. UTC
Based on seqiv, IPsec ESP and rfc4543/rfc4106 the assoclen can be 16 or
20 bytes.

From esp4/esp6, assoclen is sizeof IP Header. This includes spi, seq_no
and extended seq_no, that is 8 or 12 bytes.
In seqiv, to asscolen is added the IV size (8 bytes).
Therefore, the assoclen, for rfc4543, should be restricted to 16 or 20
bytes, as for rfc4106.

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
 crypto/gcm.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Herbert Xu July 18, 2019, 2:46 p.m. UTC | #1
On Thu, Jul 18, 2019 at 05:43:04PM +0300, Iuliana Prodan wrote:
> Based on seqiv, IPsec ESP and rfc4543/rfc4106 the assoclen can be 16 or
> 20 bytes.
> 
> >From esp4/esp6, assoclen is sizeof IP Header. This includes spi, seq_no
> and extended seq_no, that is 8 or 12 bytes.
> In seqiv, to asscolen is added the IV size (8 bytes).
> Therefore, the assoclen, for rfc4543, should be restricted to 16 or 20
> bytes, as for rfc4106.
> 
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

Why does this matter? Is it for the fuzz test?

Cheers,
Iuliana Prodan July 18, 2019, 2:56 p.m. UTC | #2
On 7/18/2019 5:46 PM, Herbert Xu wrote:
> On Thu, Jul 18, 2019 at 05:43:04PM +0300, Iuliana Prodan wrote:
>> Based on seqiv, IPsec ESP and rfc4543/rfc4106 the assoclen can be 16 or
>> 20 bytes.
>>
>> >From esp4/esp6, assoclen is sizeof IP Header. This includes spi, seq_no
>> and extended seq_no, that is 8 or 12 bytes.
>> In seqiv, to asscolen is added the IV size (8 bytes).
>> Therefore, the assoclen, for rfc4543, should be restricted to 16 or 20
>> bytes, as for rfc4106.
>>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> 
> Why does this matter? Is it for the fuzz test?
> 
> Cheers,
> 

Yes, this is for fuzz testing.
The generic implementation for rfc4543 considers any assoclen valid, 
which is not correct.

Regards,
Iulia
Herbert Xu July 18, 2019, 2:59 p.m. UTC | #3
On Thu, Jul 18, 2019 at 02:56:35PM +0000, Iuliana Prodan wrote:
>
> Yes, this is for fuzz testing.
> The generic implementation for rfc4543 considers any assoclen valid, 
> which is not correct.

So I presume the driver does enforce the limit.  Please actually
state that in the commit description for future reference.

Thanks,
Herbert Xu July 18, 2019, 3:11 p.m. UTC | #4
On Thu, Jul 18, 2019 at 10:59:07PM +0800, Herbert Xu wrote:
> 
> So I presume the driver does enforce the limit.  Please actually
> state that in the commit description for future reference.

Also have you looked at whether other drivers would be affected
by this? It wouldn't be so nice if this change makes other drivers
fail the same test as a result.

Thanks,
Iuliana Prodan July 19, 2019, 7:13 a.m. UTC | #5
On 7/18/2019 6:11 PM, Herbert Xu wrote:
> On Thu, Jul 18, 2019 at 10:59:07PM +0800, Herbert Xu wrote:
>>
>> So I presume the driver does enforce the limit.  Please actually
>> state that in the commit description for future reference.
> 
> Also have you looked at whether other drivers would be affected
> by this? It wouldn't be so nice if this change makes other drivers
> fail the same test as a result.

I've checked the other drivers and I found rfc4543 support in bcm and 
ccree. I've sent fixes for these two, but I don't have hardware to test 
on, so they are just compile tested.

Link: https://patchwork.kernel.org/project/linux-crypto/list/?series=147747

Regards,
Iulia

Patch
diff mbox series

diff --git a/crypto/gcm.c b/crypto/gcm.c
index 33f45a9..4d720e6 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -1048,11 +1048,17 @@  static int crypto_rfc4543_copy_src_to_dst(struct aead_request *req, bool enc)
 
 static int crypto_rfc4543_encrypt(struct aead_request *req)
 {
+	if (req->assoclen != 16 && req->assoclen != 20)
+		return -EINVAL;
+
 	return crypto_rfc4543_crypt(req, true);
 }
 
 static int crypto_rfc4543_decrypt(struct aead_request *req)
 {
+	if (req->assoclen != 16 && req->assoclen != 20)
+		return -EINVAL;
+
 	return crypto_rfc4543_crypt(req, false);
 }