Re: potential bug in GMAC implementation. not work in ESN mode

[Jussi Kivilinna <jussi.kivilinna@iki.fi>]

[-- Attachment #1.1: Type: text/plain, Size: 2203 bytes --]

On 25.03.2013 18:12, Chaoxing Lin wrote:
> 2nd ping....
> 
> Nobody is maintaining crypto/gcm.c?
> 
> 
> 
> -----Original Message-----
> From: Chaoxing Lin 
> Sent: Friday, March 08, 2013 11:38 AM
> To: 'linux-crypto@vger.kernel.org'
> Subject: potential bug in GMAC implementation. not work in ESN mode
> 
> I was testing ipsec with GMAC and found that the rfc4543 GMAC implementation in kernel software crypto work in "esp=aes256gmac-noesn!" mode.
> It does not work in in "esp=aes256gmac-esn!" mode. The tunnel was established but no data traffic is possible.
> 
> Looking at source code, I found this piece of code is suspicious.
> Line 1146~1147 tries to put req->assoc to assoc[1]. But I think this way only works when req->assoc has only one segment. In ESN mode, req->assoc contains 3 segments (SPI, SN-hi, SN-low). Line 1146~1147 will only attach SPI segment(with total length) in assoc.
> 
> Please let me know whether I understand it right.

Your analysis seems correct. Does attached the patch fix the problem? (I've only compile tested it.)

-Jussi

> Thanks,
> 
> Chaoxing
> 
> 
> Source from kernel 3.8.2
> path: root/crypto/gcm.c
> 
> 1136: /* construct the aad */
> 1137:	dstp = sg_page(dst);
> 	vdst = PageHighMem(dstp) ? NULL : page_address(dstp) + dst->offset;
> 
> 	sg_init_table(payload, 2);
> 	sg_set_buf(payload, req->iv, 8);
> 	scatterwalk_crypto_chain(payload, dst, vdst == req->iv + 8, 2);
> 	assoclen += 8 + req->cryptlen - (enc ? 0 : authsize);
> 
> 	sg_init_table(assoc, 2);
> 1146:	sg_set_page(assoc, sg_page(req->assoc), req->assoc->length,
> 1147:		    req->assoc->offset);
> 	scatterwalk_crypto_chain(assoc, payload, 0, 2);
> 
> 	aead_request_set_tfm(subreq, ctx->child);
> 	aead_request_set_callback(subreq, req->base.flags, req->base.complete,
> 				  req->base.data);
> 	aead_request_set_crypt(subreq, cipher, cipher, enc ? 0 : authsize, iv);
> 1154:	aead_request_set_assoc(subreq, assoc, assoclen);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 10-gcm-fix-assumption-that-assoc-has-one-segment.patch --]
[-- Type: text/x-patch; name="10-gcm-fix-assumption-that-assoc-has-one-segment.patch", Size: 1901 bytes --]

crypto: gcm - fix assumption that assoc has one segment

From: Jussi Kivilinna <jussi.kivilinna@iki.fi>

Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
---
 crypto/gcm.c    |   17 ++++++++++++++---
 crypto/tcrypt.c |    4 ++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/crypto/gcm.c b/crypto/gcm.c
index 137ad1e..13ccbda 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -44,6 +44,7 @@ struct crypto_rfc4543_ctx {
 
 struct crypto_rfc4543_req_ctx {
 	u8 auth_tag[16];
+	u8 assocbuf[32];
 	struct scatterlist cipher[1];
 	struct scatterlist payload[2];
 	struct scatterlist assoc[2];
@@ -1133,9 +1134,19 @@ static struct aead_request *crypto_rfc4543_crypt(struct aead_request *req,
 	scatterwalk_crypto_chain(payload, dst, vdst == req->iv + 8, 2);
 	assoclen += 8 + req->cryptlen - (enc ? 0 : authsize);
 
-	sg_init_table(assoc, 2);
-	sg_set_page(assoc, sg_page(req->assoc), req->assoc->length,
-		    req->assoc->offset);
+	if (req->assoc->length == req->assoclen) {
+		sg_init_table(assoc, 2);
+		sg_set_page(assoc, sg_page(req->assoc), req->assoc->length,
+			    req->assoc->offset);
+	} else {
+		BUG_ON(req->assoclen > sizeof(rctx->assocbuf));
+
+		scatterwalk_map_and_copy(rctx->assocbuf, req->assoc, 0,
+					 req->assoclen, 0);
+
+		sg_init_table(assoc, 2);
+		sg_set_buf(assoc, rctx->assocbuf, req->assoclen);
+	}
 	scatterwalk_crypto_chain(assoc, payload, 0, 2);
 
 	aead_request_set_tfm(subreq, ctx->child);
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 87ef7d6..6b911ef 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -1225,6 +1225,10 @@ static int do_test(int m)
 		ret += tcrypt_test("rfc4106(gcm(aes))");
 		break;
 
+	case 152:
+		ret += tcrypt_test("rfc4543(gcm(aes))");
+		break;
+
 	case 200:
 		test_cipher_speed("ecb(aes)", ENCRYPT, sec, NULL, 0,
 				speed_template_16_24_32);

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 730 bytes --]