Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 25 Feb 2019 19:14:16 +0000 (UTC)
From:      Sean Eric Fagan <sef@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r344547 - head/sys/opencrypto
Message-ID:  <201902251914.x1PJEGme046232@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: sef
Date: Mon Feb 25 19:14:16 2019
New Revision: 344547
URL: https://svnweb.freebsd.org/changeset/base/344547

Log:
  Fix another bug introduced during the review process of r344140:
  the tag wasn't being computed properly due to chaning a >= comparison
  to an == comparison.
  
  Specifically:  CBC-MAC encodes the length of the authorization data
  into the the stream to be encrypted/hashed.  For short data, this is
  two bytes (big-endian 16 bit value); for larger data, it's 6 bytes
  (a prefix of 0xff, 0xfe, followed by a 32-bit big-endian length).  And
  there's a larger size, which is 10 bytes.  These extra bytes weren't
  being accounted for with the post-review code.  The other bit that then came
  into play was that OCF only calls the Update code with blksiz=16, which
  meant that I had to ignore the length variable.  (It also means that it
  can't be called with a single buffer containing the AAD and payload;
  however, OCF doesn't do this for the software-only algorithsm.)
  
  I tested with this script:
  
  ALG=aes-ccm
  DEV=soft
  
  for aad in 0 1 2 3 4 14 16 24 30 32 34 36 1020
  do
          for dln in 16 32 1024 2048 10240
          do
                  echo "Testing AAD length ${aad} data length ${dln}"
                  /root/cryptocheck -A ${aad} -a ${ALG} -d ${DEV} ${dln}
          done
  done
  
  Reviewed by:	cem
  Sponsored by:	iXsystems Inc.

Modified:
  head/sys/opencrypto/cbc_mac.c

Modified: head/sys/opencrypto/cbc_mac.c
==============================================================================
--- head/sys/opencrypto/cbc_mac.c	Mon Feb 25 19:07:52 2019	(r344546)
+++ head/sys/opencrypto/cbc_mac.c	Mon Feb 25 19:14:16 2019	(r344547)
@@ -124,23 +124,31 @@ AES_CBC_MAC_Reinit(struct aes_cbc_mac_ctx *ctx, const 
 	rijndaelEncrypt(ctx->keysched, ctx->rounds, b0, ctx->block);
 	/* If there is auth data, we need to set up the staging block */
 	if (ctx->authDataLength) {
+		size_t addLength;
 		if (ctx->authDataLength < ((1<<16) - (1<<8))) {
 			uint16_t sizeVal = htobe16(ctx->authDataLength);
 			bcopy(&sizeVal, ctx->staging_block, sizeof(sizeVal));
-			ctx->blockIndex = sizeof(sizeVal);
+			addLength = sizeof(sizeVal);
 		} else if (ctx->authDataLength < (1ULL<<32)) {
 			uint32_t sizeVal = htobe32(ctx->authDataLength);
 			ctx->staging_block[0] = 0xff;
 			ctx->staging_block[1] = 0xfe;
 			bcopy(&sizeVal, ctx->staging_block+2, sizeof(sizeVal));
-			ctx->blockIndex = 2 + sizeof(sizeVal);
+			addLength = 2 + sizeof(sizeVal);
 		} else {
 			uint64_t sizeVal = htobe64(ctx->authDataLength);
 			ctx->staging_block[0] = 0xff;
 			ctx->staging_block[1] = 0xff;
 			bcopy(&sizeVal, ctx->staging_block+2, sizeof(sizeVal));
-			ctx->blockIndex = 2 + sizeof(sizeVal);
+			addLength = 2 + sizeof(sizeVal);
 		}
+		ctx->blockIndex = addLength;
+		/*
+		 * The length descriptor goes into the AAD buffer, so we
+		 * need to account for it.
+		 */
+		ctx->authDataLength += addLength;
+		ctx->authDataCount = addLength;
 	}
 }
 
@@ -181,10 +189,9 @@ AES_CBC_MAC_Update(struct aes_cbc_mac_ctx *ctx, const 
 			ctx->authDataCount += copy_amt;
 			ctx->blockIndex += copy_amt;
 			ctx->blockIndex %= sizeof(ctx->staging_block);
-			if (ctx->authDataCount == ctx->authDataLength)
-				length = 0;
+
 			if (ctx->blockIndex == 0 ||
-			    ctx->authDataCount >= ctx->authDataLength) {
+			    ctx->authDataCount == ctx->authDataLength) {
 				/*
 				 * We're done with this block, so we
 				 * xor staging_block with block, and then
@@ -193,8 +200,17 @@ AES_CBC_MAC_Update(struct aes_cbc_mac_ctx *ctx, const 
 				xor_and_encrypt(ctx, ctx->staging_block, ctx->block);
 				bzero(ctx->staging_block, sizeof(ctx->staging_block));
 				ctx->blockIndex = 0;
+				if (ctx->authDataCount >= ctx->authDataLength)
+					break;
 			}
 		}
+		/*
+		 * We'd like to be able to check length == 0 and return
+		 * here, but the way OCF calls us, length is always
+		 * blksize (16, in this case).  So we have to count on
+		 * the fact that OCF calls us separately for the AAD and
+		 * for the real data.
+		 */
 		return (0);
 	}
 	/*

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201902251914.x1PJEGme046232>