linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] crypto: gcm - restrict assoclen for rfc4543
@ 2019-07-18 14:43 Iuliana Prodan
  2019-07-18 14:46 ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Iuliana Prodan @ 2019-07-18 14:43 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David S. Miller, linux-crypto, linux-kernel, linux-imx

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(+)

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);
 }
 
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: gcm - restrict assoclen for rfc4543
  2019-07-18 14:43 [PATCH] crypto: gcm - restrict assoclen for rfc4543 Iuliana Prodan
@ 2019-07-18 14:46 ` Herbert Xu
  2019-07-18 14:56   ` Iuliana Prodan
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2019-07-18 14:46 UTC (permalink / raw)
  To: Iuliana Prodan; +Cc: David S. Miller, linux-crypto, linux-kernel, linux-imx

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: gcm - restrict assoclen for rfc4543
  2019-07-18 14:46 ` Herbert Xu
@ 2019-07-18 14:56   ` Iuliana Prodan
  2019-07-18 14:59     ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Iuliana Prodan @ 2019-07-18 14:56 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: gcm - restrict assoclen for rfc4543
  2019-07-18 14:56   ` Iuliana Prodan
@ 2019-07-18 14:59     ` Herbert Xu
  2019-07-18 15:11       ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2019-07-18 14:59 UTC (permalink / raw)
  To: Iuliana Prodan; +Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: gcm - restrict assoclen for rfc4543
  2019-07-18 14:59     ` Herbert Xu
@ 2019-07-18 15:11       ` Herbert Xu
  2019-07-19  7:13         ` Iuliana Prodan
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2019-07-18 15:11 UTC (permalink / raw)
  To: Iuliana Prodan; +Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] crypto: gcm - restrict assoclen for rfc4543
  2019-07-18 15:11       ` Herbert Xu
@ 2019-07-19  7:13         ` Iuliana Prodan
  0 siblings, 0 replies; 6+ messages in thread
From: Iuliana Prodan @ 2019-07-19  7:13 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-07-19  7:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-18 14:43 [PATCH] crypto: gcm - restrict assoclen for rfc4543 Iuliana Prodan
2019-07-18 14:46 ` Herbert Xu
2019-07-18 14:56   ` Iuliana Prodan
2019-07-18 14:59     ` Herbert Xu
2019-07-18 15:11       ` Herbert Xu
2019-07-19  7:13         ` Iuliana Prodan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).