Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10919163:
Add GCM, CTR, and CTS modes to AES. (Closed)
|
Created:
8 years, 3 months ago by wtc Modified:
8 years, 1 month ago CC:
chromium-reviews Visibility:
Public. |
DescriptionAdd GCM, CTR, and CTS modes to AES.
This is Bob Relyea's NSS freebl patch
https://bug373108.bugzilla.mozilla.org/attachment.cgi?id=658653
imported into Rietveld for code review.
The change to lib/freebl/manifest.mn is omitted.
R=rsleevi@chromium.org
BUG=none
TEST=none
Patch Set 1 #
Total comments: 84
Patch Set 2 : Move lib/util/pkcs11t.h from the softoken CL to this CL #Patch Set 3 : #
Total comments: 13
Patch Set 4 : Fix comments as rsleevi suggested, fix a 32-bit bug and miscellaneous issues #
Total comments: 3
Messages
Total messages: 26 (0 generated)
Patch set 1 is Bob Relyea's patch.
Wan-Teh: Here is the first batch of comments. I am still working my way through the GCM implementation - I have just focused on the CTR & CTS implementations at this time. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/blapii.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/blapii.h:43: #define MAX_BLOCK_SIZE 16 nit: It seems useful to add a comment here explaining this, much like found within SFTK_MAX_BLOCK_SIZE /** max block size of supported block ciphers */ http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/blapii.h:45: typedef SECStatus (*freeblCipherFunc)(void *cx, unsigned char *output, I think it would be helpful if you could document here what the expectations are. Most notably: - Is there a requirement that inputLen is a multiple of blocksize? - If not, what is the handling mode? - What is the expectation if inputLen = 0 (for an "empty block")? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/blapii.h:64: nit: unnecessary newline? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:25: ctr->counterBits = ctrParams->ulCounterBits; s/= ctrParams/= ctrParams/ (extra space) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:25: ctr->counterBits = ctrParams->ulCounterBits; It seems you should be checking here, instead of / in addition to line 72, that counterBits <= blocksize * BITS_PER_BYTE, and if it is not, then return SECFailure. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:53: PORT_Memset(ctr, 0, sizeof (CTRContext)); Why the additional memset here? In looking at the other ciphers, such as RC4, they tend to be implemented in terms of: if (freeit) PORT_ZFree(ctr, sizeof(*ctr)) Even AES_DestroyContext doesn't zeroize out its *cx. If this is for security purposes, shouldn't there be a "probably won't be optimized out by a compiler" form of cleansing? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:85: *counterPtr = ((*counterPtr) & ~mask) | count; This does not appear to handle counter rollover correctly, as specified by PKCS#11. From 6.9.3 of the Draft 7 of 2.30 If an attempt to encrypt/decrypt is made which will cause an overflow of the counter block’s counter bits, then the mechanism shall return CKR_DATA_LEN_RANGE. Note that the mechanism should allow the final post increment of the counter to overflow (if it implements it this way) but not allow any further processing after this point. E.g. if ulCounterBits = 2 and the counter bits start as 1 then only 3 blocks of data can be processed. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:18: }; In the other headers, the relevant ContextStr is forward declared (such as on line 20 in this file), but does not appear in the actual header. It seems like this belongs within ctr.c instead http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:22: SECStatus CTR_InitContext(CTRContext *ctr, void *context, nit: Does this need to be a public method? Are callers expected to call it? It seems an internal implementation detail of how CTR_CreateContext performs work. Compared with CTS_ and GCM_, which both lack InitContext functions, I had trouble understanding the distinction and purpose, unless it's supposed to be an optimization strategy (when combined with the public definition of CTRContextStr) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:23: freeblCipherFunc cipher, const unsigned char *param, nit/non-functional: Why the use of "const unsigned char" vs "const void"? The best I can come up with is that it may be somehow related to alignment and de-referencing, but given its usage, it seems like "const void*" would be better here and functionally equivalent. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:26: CTRContext * CTR_CreateContext(void *context, freeblCipherFunc cipher, It would be good to document what |context| is here (and on line 22), in that it represents the inner cipher context to use with freeblCipherFunc http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:29: void CTR_DestroyContext(CTRContext *ctr, PRBool freeit); It's worth noting that this function **does not** free the |context| supplied in Create/InitCOntext (meaning that the context needs to remain valid for as long as the CTRContext is valid) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:107: fullblocks = (inlen/blocksize)*blocksize; nit: The rest of the file uses spaces between operators, it seems, so this should be fullblocks = (inlen / blocksize) * blocksize; Also on line 193. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:123: * here's the CTS magic, we pad our final block with zeros, From merely inspecting the code (I've not yet fired into a debugger to trace), this surprises me that it does the right thing when a caller is using C_EncryptInit + C_EncryptUpdate( * 2 or more) + C_EncryptFinal. My concern is that it would perform the CTS for every EncryptUpdate, rather than deferring the CTS to the final call to C_EncryptFinal. With CKM_AES_GCM, context->doPad is not set to true (at least, not on line 773/774 of https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=373108&attachment=... ). context->update = AES_Encrypt ( http://mxr.mozilla.org/security/source/security/nss/lib/softoken/pkcs11c.c#772 ), context->blocksize = 0 ( http://mxr.mozilla.org/security/source/security/nss/lib/softoken/pkcs11c.c#433 ) When NSC_EncryptUpdate is called, if doPad = 0, it will call context->update with the (entire) data passed to C_EncryptUpdate. This will call AES_Encrypt, which will forward to cx->worker with the entire buffer from C_EncryptUpdate. cx->worker in this case is CTS_EncryptUpdate. If the buffer supplied to C_EncryptUpdate/NSC_EncryptUpdate is not a full AES blocksize, then it seems it might be prematurely doing the CTS before C_EncryptFinal has been called. Again, I haven't stepped through a debugger yet with this, but I didn't see any tests for the case, which is why I wanted to check how it was expected to behave here. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:131: PORT_Memset(lastBlock, 0, blocksize); In the "premature optimization" department, it's worth noting that you may be zeroizing memory you're just going to end up copying over. Perhaps: if (blocksize - len) PORT_Memset(lastBlock + inlen, 0, blocksize - inlen) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:140: #define XOR_BLOCK(x,y,count) for(i=0; i < count; i++) x[i] = x[i] ^ y[i] In CTR and GCM, you defined this as a function (ctr_xor/gcm_xor), but in CTS, you define it as a macro. Is there a reason for the inconsistency in implementations? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:193: fullblocks = (inlen/blocksize)*blocksize; This is a misleading variable name, as it's not the number of full blocks (inlen / blocksize) + (inlen % blocksize ? 1 : 0), but in fact the number of bytes necessary to accomodate all of the full blocks. This subtlety comes into play at lines 215/216, when performing the comparison, since the constant (2) has to be multiplied by blocksize before comparison. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:203: memcpy(outbuf, inbuf, inlen); This seems rather sub-optimal that an entire copy is made. While I understand the "easier to understand the code this way" argument, it does seem sub-optimal for something potentially performance sensitive. Perhaps this can be re-ordered such that the call to cts->cipher (line 219) only needs to handle blocks 0..n-2, using the original caller-supplied inbuf, while blocks n-2..n are handled using the temporary buffers (Cn, Cn-1, Cn-2) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:287: PORT_Memset(lastBlock, 0, blocksize); nit: Does NSS rely on PORT_Memset for cleansing? Can't a compiler tell that lastBlock is no longer used (since it's a stack variable) beyond this point and simply optimize it out? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.h:8: #include "blapii.h" nit: add a newline (match ctr.h) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.h:12: nit: unnecessary new line http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/gcm.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:47: * tables to speed things up even more */ PCLMULQDQ was the AES-NI instruction that provides the carryless multiply to make GF2 multiply http://software.intel.com/en-us/articles/intel-carry-less-multiplication-inst... This includes sample code for AES-GCM. It also discusses the table approach and an optimized implementation. However, it points out that the table lookup approach discloses side-channel information by virtue of cache lines. So I'm not sure whether or not that's a concern here in suggesting their usage. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:132: nit: spare newline? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:140: gcm_reverse(unsigned char *target, const unsigned char *src, The Intel documentation points out (in Algorithm 5 / page 17 / http://software.intel.com/sites/default/files/m/d/4/1/d/8/CLMUL_WP_Rev_02_Fin...) a way to perform the multiplication without requiring a reflection to happen, which seems desirable here. Namely, that reflected(A) * reflected(B) = reflected(A*B) >> 1 This is further enhanced in http://www.intel.com/content/dam/www/public/us/en/documents/white-papers/comm... , which points out that you can avoid an extra shift per-operation by pre-computing the shift over the hash key. reflected(A) * reflected(H << 1 mod g(x)) = reflected(A * H) mod g(x) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:212: nit: spare newline? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:242: static SECStatus nit: add newline http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:267: * the bultin bmulmod fro mpl s/bultin/builtin s/fro/from http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:278: nit: spare newline? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:288: /* use algorithm 1 of McGree & Viega "The Galois/Counter Mode of Operation" */ s/McGree/McGrew http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:388: #define GCM_HIGH_BIT (0x1UL << ((sizeof(unsigned long) * BITS_PER_BYTE)-1)) Throughout this file, you frequently have sizeof(unsigned long) * BITS_PER_BYTE It seems like it would be useful for readability to specify this as a static constant rather than repeating it. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:412: for (l=0; l < len; l++) t[l] ^= s[l] It's not clear why both this macro and the gcm_xor function (line 380), which appears to be unused. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/gcm.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.h:6: nit: delete blank line http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.h:9: nit: Missing #include of blapii.h http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/rijndael.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/rijndael.c:1194: if (freeit && cx->worker_cx && cx->destroy) { It's not clear why you switch on freeit here. My expectation would be the following conditional: if (cx->worker_cx) { if (cx->destroy) { (*cx->destroy)(cx->worker_cx, freeit); cx->destroy = NULL; } cx->worker_cx = NULL; } Could you explain why the above is not correct?
I have not reviewed the Algorithm 1 implementation in GCM, nor the changes to the MPI field multiply code. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/gcm.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:283: ghash->m = 0;; nit: extra ; http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:455: * function always takes blocksize lengths of data. HashUpdate will nit: extra space ("HashUpdate will") http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:460: unsigned int len, unsigned int blocksize) nit: Looks like alignment is incorrect here regardless of tab/spacing differences http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:470: PORT_Memcpy(ghash->buffer+ghash->bufLen, buf, needed); nit: Throughout this file, it seems to alter inconsistently between using spaces between operators and no spaces between operators. I think for readability, it would be more helpful to have spaces between operators - even at the expense of line-lengths. See for example line 487, 521-522, 528, 556, 582, etc http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:517: memcpy(ghash->counterBuf, &ghash->counterBuf[GCM_HASH_LEN_LEN], BUG? PORT_Memcpy http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:524: ghash->cLen = 0; It's not clear to me why this counter work happens here. While it's admittedly a no-op when called from gcmHash_Reset (line 592), I found it confusing to understand, since this counter manipulation is really only applicable when computing the final bit of data to hash (line 556). Have I misunderstood the construct? Perhaps you could point to what operation in Section 7.1 lines 516-524 are supposed to be implementing? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:540: * This does the final since, hashes the lengths, then returns "final since" ? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:577: unsigned int AADLen, unsigned int blocksize) nit: suggest instead of "AAD" this could be called "authenticatedData". While "AAD" is likely clear to someone who has read the GCM spec, the use of a three-letter acronym may make it harder to understand and keep track of (in spite of the definition at line 586) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:588: rv =gcmHash_Update(ghash, AAD, AADLen, blocksize); nit: rv = gcmHash_Update http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:592: rv = gcmHash_Sync(ghash, blocksize); In reading this, I actually found the naming here to be non-obvious. The purpose of gcmHash_Sync is to zero-pad the (previously hashed) data to be a multiple of the block size (In SP-800-38D, this is Step 5 of Section 7.1) - eg: A || 0v or C || 0u. I'm not sure a good name to suggest here, and while I suspect that "sync" was chosen as in "sync to a block size boundary", I'm still not sure it's best. Perhaps something related to "align" or "pad" to clarify exactly what is happening? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:609: * Now Inplement the GCM using gcmHash and CTR * nit: implement (typo and cap) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:620: PRBool freeHash = PR_FALSE; nit: s/ / / http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:646: PORT_Memcpy(&ctrParams.cb[4], gcmParams->pIv, gcmParams->ulIvLen); Isn't there an endianness concern here? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:652: gcmParams->ulIvLen, blocksize); nit: weird line breaking here (I would have expected iv & len to be on the same line) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:656: rv = gcmHash_Final(ghash, ctrParams.cb, &tmp, blocksize, blocksize); BUG: Worth noting here that, at this time, there hasn't been a check that blocksize < MAX_BLOCK_SIZE, nor that it's < sizeof(cb) [whose size is fixed at 16, which is not the same as MAX_SIZE_BLOCKS, even though MAX_SIZE_BLOCKS *currently* equals 16) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:668: /* fill int the gcm structure */ typo: fill in the http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:670: /* calculate the final tag key. NOTE:gcm->tagKey is zero to start with typo: NOTE: gcm->tagKey suggest breaking on to a newline, so the NOTE is clearer. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:679: rv = gcmHash_Reset(ghash,gcmParams->pAAD, gcmParams->ulAADLen, blocksize); nit: ghash, gcmParams->pAAD http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:748: outbuf[tagBytes-1] &= ~ ((1<< extra) -1); nit: ((1 << extra) - 1) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:769: if (maxout < inlen + tagBytes) { nit: While unlikely, I'm nervous about this from an overflow perspective, where inlen is near UINT_MAX (however unlikely), outlen == inlen, and tagBytes+inlen wraps back to 1/2/3. At that time, maxout > (inlen + tagBytes with overflow), and the check won't catch it. if (UINT_MAX - inlen < tagBytes) { PORT_SetError(SEC_ERROR_INPUT_LEN); return SECFailure; } if (maxout < inlen + tagBytes) { PORT_SetError(SEC_ERROR_OUTPUT_LEN); return SECFailure; } http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:836: if (PORT_Memcmp(tag, intag, tagBytes) != 0) { SECURITY BUG: NSS's SSL layer uses NSS_SecureMemcmp to provide a fixed time comparison, to avoid leaking timing information. Checking the authentication tag should happen using such a fixed time comparison.
Status update: rsleevi: thank you for the review comments. I have reviewed everything except cts.c. I will fix the minor problems I found and address the issues rsleevi reported. I should be able to upload a new patch set tonight or tomorrow. (I am quite busy today.)
rsleevi: please review patch set 3. Thanks. I added a ReviewComments file to summarize the changes we made and remaining issues for rrelyea. I made all the changes you suggested, except a small number of minor variable/function name and formatting issues. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/blapii.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/blapii.h:45: typedef SECStatus (*freeblCipherFunc)(void *cx, unsigned char *output, On 2012/09/11 19:34:30, Ryan Sleevi wrote: > I think it would be helpful if you could document here what the expectations > are. I looked into this. I found that freeblDestroyFunc can be a block cipher (in ECB or CBC/no-padding mode) or a stream cipher (CTR, CTS, and GCM), so it's not easy to document the expectations here. I chose to document what ctr.h, cts.h, and gcm.h expect of their |cipher| input argument. The downside of that approach is that I had to replicate the freeblCipherFunc function prototype in those headers, so that I can talk about the inputLen and blocksize arguments. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:53: PORT_Memset(ctr, 0, sizeof (CTRContext)); On 2012/09/11 19:34:30, Ryan Sleevi wrote: > > In looking at the other ciphers, such as RC4, they tend to be implemented in > terms of: > > if (freeit) > PORT_ZFree(ctr, sizeof(*ctr)) The form used here is also common. See, for example, SHA1_DestroyContext. Nelson commented out the memset call in AES_DestroyContext in his patch for bug 303334 to improve performance. > If this is for security purposes, shouldn't there be a "probably won't be > optimized out by a compiler" form of cleansing? Yes, this is to erase security-sensitive data from memory. We should add a NSS_SecureMemset function to lib/util/secport.h. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:22: SECStatus CTR_InitContext(CTRContext *ctr, void *context, On 2012/09/11 19:34:30, Ryan Sleevi wrote: > > Compared with CTS_ and GCM_, which both lack InitContext functions, I had > trouble understanding the distinction and purpose, unless it's supposed to be an > optimization strategy (when combined with the public definition of > CTRContextStr) Exactly. gcm.c also needs struct CTRContextStr and CTR_InitContext to avoid a memory allocation. (I also wondered about this.) http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:23: freeblCipherFunc cipher, const unsigned char *param, On 2012/09/11 19:34:30, Ryan Sleevi wrote: > nit/non-functional: Why the use of "const unsigned char" vs "const void"? The const unsigned char * type comes from the const unsigned char *iv argument of AES_CreateContext. The |iv| argument is now overloaded to mean |param| for the new modes of operations. I don't know whether we should change the type to const void * or the concrete type, such as const CK_AES_CTR_PARAMS *. I'll ask rrelyea. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:203: memcpy(outbuf, inbuf, inlen); On 2012/09/11 19:34:30, Ryan Sleevi wrote: > This seems rather sub-optimal that an entire copy is made. While I understand > the "easier to understand the code this way" argument, it does seem sub-optimal > for something potentially performance sensitive. > > Perhaps this can be re-ordered such that the call to cts->cipher (line 219) only > needs to handle blocks 0..n-2, using the original caller-supplied inbuf, while > blocks n-2..n are handled using the temporary buffers (Cn, Cn-1, Cn-2) I didn't study the cts.c code, so I don't know how to make the change you suggested. Would you like to give it a try and send me a patch? http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/gcm.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:524: ghash->cLen = 0; On 2012/09/12 00:22:47, Ryan Sleevi wrote: > > Perhaps you could point to what operation in Section 7.1 lines 516-524 are > supposed to be implementing? I bet rrelyea implemented the original GHASH function. This difference is mentioned in the footnote in Section 5.3 on page 9 of NIST SP 800-38D. I didn't read the original McGrew & Viega GCM paper, but I verified that rrelyea's code is equivalent to the GCM as specified in NIST SP 800-38D in spite of this difference. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:646: PORT_Memcpy(&ctrParams.cb[4], gcmParams->pIv, gcmParams->ulIvLen); On 2012/09/12 00:22:47, Ryan Sleevi wrote: > Isn't there an endianness concern here? No. Endianness only applies to the final 32 bits of ctrParams.cb. The first 96 bits of ctrParams.cb do not represent an integer. http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... File nss/mozilla/security/nss/lib/freebl/gcm.c (right): http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... nss/mozilla/security/nss/lib/freebl/gcm.c:468: if (ghash->bufLen != blocksize) { This test is equivalent to the original test (len == 0), but this test makes it clearer that ghash->buffer now has a full block of data that can be hashed. If you think the original test is clearer, I'll change this back.
wtc: I just reviewed the diffs from Patchset 2. Based on the ReviewComments, I think there are still some larger issues that need to be addressed, so I have not attempted a detailed re-review. http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... File nss/mozilla/security/nss/lib/freebl/ctr.h (right): http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... nss/mozilla/security/nss/lib/freebl/ctr.h:36: * unsigned int blocksize); I'm not sure it's necessary to copy the typedef of freeblCipherFunc. I think specifying that it's ECB (or, as Zooko likes to say, the "AES function") is sufficient. http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... nss/mozilla/security/nss/lib/freebl/cts.c:131: if (blocksize - inlen != 0) { Is it worth adding the explicit parenthesis? if ((blocksize - inlen) != 0) { http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... File nss/mozilla/security/nss/lib/freebl/cts.h (right): http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... nss/mozilla/security/nss/lib/freebl/cts.h:22: * inputLen is always a multiple of blocksize and never 0. I found this comment a little confusing at first, because it wasn't clear whether you were talking about CTS_EncryptUpdate or about the CBC function (answer: the CBC function). I'm also not sure if it's necessary to specify here, since that's specifying a behaviour/implementation detail of the CTS implementation, not on a pre-/post- condition of the CBC or CTS contexts. http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... File nss/mozilla/security/nss/lib/freebl/gcm.h (right): http://codereview.chromium.org/10919163/diff/3005/nss/mozilla/security/nss/li... nss/mozilla/security/nss/lib/freebl/gcm.h:16: */ I actually find this comment confusing, in that it's not clear from reading whether |context| and |cipher| should contain an initialized CTRContext (to be embedded) or if it should contain ECB context & pointer. I think it would be good to add a comment here that didn't have to refer to another header.
rsleevi: please review patch set 4. I made all the changes you suggested. Thanks. I think it is fine for rrelyea to check in this patch with the issues I listed in ReviewComments. It will make future code reviews easier. https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... File nss/mozilla/security/nss/lib/freebl/ctr.h (right): https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/ctr.h:36: * unsigned int blocksize); On 2012/09/17 22:57:26, Ryan Sleevi wrote: > I'm not sure it's necessary to copy the typedef of freeblCipherFunc. I think > specifying that it's ECB (or, as Zooko likes to say, the "AES function") is > sufficient. I have removed the freeblCipherFunc typedef that I copied here. It was my attempt to document specific instances of freeblCipherFunc (because it's hard to document freeblCipherFunc in general -- it can be any kind of cipher). https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... File nss/mozilla/security/nss/lib/freebl/cts.c (right): https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/cts.c:131: if (blocksize - inlen != 0) { On 2012/09/17 22:57:26, Ryan Sleevi wrote: > Is it worth adding the explicit parenthesis? > if ((blocksize - inlen) != 0) { I usually don't add parentheses in this kind of expression. In any case, I found that blocksize - inlen must be nonzero (because inlen has been reduced mod blocksize), so I removed the if. https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/cts.c:201: pad = blocksize - (fullblocks - inlen); pad = blocksize + (inlen - fullblocks); // avoid negative number BUG(?): I think this should be pad = inlen - fullblocks; if (pad) { because the comment for encrypt says: pad = *outlen % blocksize; if (pad) { https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/cts.c:213: memcpy(outbuf+inlen-blocksize, lastBlock, blocksize); BUG: you are doing the opposite conversion (from CS-2 to CS-1)! https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/cts.c:283: (void) (*cts->cipher)(cts->context, lastBlock, &tmpLen, blocksize, Cn, Should we check the return value? https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... File nss/mozilla/security/nss/lib/freebl/cts.h (right): https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/cts.h:22: * inputLen is always a multiple of blocksize and never 0. On 2012/09/17 22:57:26, Ryan Sleevi wrote: > I found this comment a little confusing at first, ... Comment removed. https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... File nss/mozilla/security/nss/lib/freebl/gcm.c (right): https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/gcm.c:468: if (ghash->bufLen != blocksize) { I changed this back to if (len == 0). https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... File nss/mozilla/security/nss/lib/freebl/gcm.h (right): https://chromiumcodereview.appspot.com/10919163/diff/3005/nss/mozilla/securit... nss/mozilla/security/nss/lib/freebl/gcm.h:16: */ On 2012/09/17 22:57:26, Ryan Sleevi wrote: > > I think it would be good to add a comment here that didn't have to refer to > another header. Done. https://chromiumcodereview.appspot.com/10919163/diff/16001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/ReviewComments (right): https://chromiumcodereview.appspot.com/10919163/diff/16001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/ReviewComments:83: Patch set 3: This is a typo. This should say "Patch set 4".
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.h (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:18: }; In other headers, the structure can be opaque. In the GCM case, we include a full CTR context in the GCM context, so the struct CTRContextStr needs to be in header (since it's shared with gcm and ctr. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:22: SECStatus CTR_InitContext(CTRContext *ctr, void *context, Yup, same reason the context isn't opaque. GCM needs to call the init function. This is similiar to the way the AES_Init function operates. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.h:23: freeblCipherFunc cipher, const unsigned char *param, I was reluctant to change the signature of AES_CreateContext, since there are some external users of the function, much as I have railed against such use.
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:53: PORT_Memset(ctr, 0, sizeof (CTRContext)); > We should add a NSS_SecureMemset function to lib/util/secport.h. That's a very good suggestion. bob http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:85: *counterPtr = ((*counterPtr) & ~mask) | count; I was under the impression that the counter was supposed to roll over so that you could handle 1,2,3, then 0. I can accept that I misread the spec, however. It's certainly easier to prevent the rollover condition, which is clearly more secure, so I'm OK with a change that implements Ryan's suggection.
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:85: *counterPtr = ((*counterPtr) & ~mask) | count; On 2012/09/19 21:43:40, rjrejyea wrote: > I was under the impression that the counter was supposed to roll over so that > you could handle 1,2,3, then 0. I can accept that I misread the spec, however. > It's certainly easier to prevent the rollover condition, which is clearly more > secure, so I'm OK with a change that implements Ryan's suggection. > Sorry, this may have been read opposite of how I intended. "Rolloverflow" is supposed to be supported (eg: [2, 3, 0, 1], [3, 0, 1, 2]), but it is expected that if you end up rolling back to the initial counter value, it should return CKR_DATA_LEN_RANGE. Meaning the four-block sequence [2, 3, 0, 1], should, if supplied a fifth block, return CKR_DATA_LEN_RANGE, rather than incrementing the counter to the sequence [2, 3, 0, 1, 2]. The reason why I made the comment is because of line 74: if (++(*counterPtr--)) { } seems to be checking against the counter looping back to zero (unless I've missed something), which is why I thought/think that the sequence [2, 3, 0, 1] is being blocked. Bitmath is hard, I may have misread here.
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:123: * here's the CTS magic, we pad our final block with zeros, So your current understanding any my initial understanding of the PKCS #11 spec for CTS was the same, and incorrect. The definition of CTS is on a call-for call basis. That is each call to EncryptUpdate must have at least 1 block, and will return a CTS encoded result of exactly the same size as the input. This is consistent with the Kerberos usage. I initially implemented something that kept back blocks and returned the CTS result at the end of the final, but then I looked at how PKCS #11 defined CTS and realized my understanding and implementation were incorrect. bob http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:140: #define XOR_BLOCK(x,y,count) for(i=0; i < count; i++) x[i] = x[i] ^ y[i] Because we really need a freebl version of xor. Almost every algorithm has it's own version. :). bob http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:287: PORT_Memset(lastBlock, 0, blocksize); Any data on the stack is still on that stack until overwritten. It's basically a FIPS thing. The less you have to handwave at the reviewer, the better. You would need to show a pretty strong performance win not to do this.. (and still have to show that lastBlock is somehow overwritten somewhere else... bob
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:85: *counterPtr = ((*counterPtr) & ~mask) | count; OK, so the bug here is that we need to store *counterPtr & mask in the data structure and blow up in count == that value. So you did catch a bug here. bob
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:85: *counterPtr = ((*counterPtr) & ~mask) | count; On 2012/09/19 21:53:15, Ryan Sleevi wrote: > On 2012/09/19 21:43:40, rjrejyea wrote: > > I was under the impression that the counter was supposed to roll over so that > > you could handle 1,2,3, then 0. I can accept that I misread the spec, however. > > It's certainly easier to prevent the rollover condition, which is clearly more > > secure, so I'm OK with a change that implements Ryan's suggection. > > > > Sorry, this may have been read opposite of how I intended. > > "Rolloverflow" is supposed to be supported (eg: [2, 3, 0, 1], [3, 0, 1, 2]), but > it is expected that if you end up rolling back to the initial counter value, it > should return CKR_DATA_LEN_RANGE. > > Meaning the four-block sequence [2, 3, 0, 1], should, if supplied a fifth block, > return CKR_DATA_LEN_RANGE, rather than incrementing the counter to the sequence > [2, 3, 0, 1, 2]. > > The reason why I made the comment is because of line 74: > > if (++(*counterPtr--)) { } seems to be checking against the counter looping back > to zero (unless I've missed something), which is why I thought/think that the > sequence [2, 3, 0, 1] is being blocked. > > Bitmath is hard, I may have misread here. Also, I didn't see the check here (or elsewhere) about ensuring that (total length of to-be-en/de-crypted) didn't exceed the number of blocks left in the counter. Since Freebl only supports a fire-and-forget en/decrypt (not streaming en/decrypt), this would mean that, somewhere, a check to make sure that the 2 ^ (counterBits) was greater-than-or-equal to the total number of blocks to be encrypted. Since rollover of the [2, 3, 0, 1, 2] form should be prevented.
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:85: *counterPtr = ((*counterPtr) & ~mask) | count; On 2012/09/19 21:59:36, rjrejyea wrote: > OK, so the bug here is that we need to store *counterPtr & mask in the data > structure and blow up in count == that value. So you did catch a bug here. > > bob I don't think that will be sufficient. *counterPtr is only comparing a byte at a time. Rollover on the byte level can happen, it's only when the total counter itself repeats that it's problematic. I think it's probably easier to make the check on the _Update function, by checking "How many blocks can I process for counterBits vs how many blocks will I process in this iteration" (even if only 1 iteration is ever performed)
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/gcm.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:47: * tables to speed things up even more */ On 2012/09/11 19:34:30, Ryan Sleevi wrote: It may be good to implement a PCLMULQDQ multiply in freebl, then the freebl code wout be even faster... as well as speeding up ECC 2fm curves. http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:412: for (l=0; l < len; l++) t[l] ^= s[l] I think the function came first and I added the macro when checking the performance (bug neglected to remove the function). bob http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:470: PORT_Memcpy(ghash->buffer+ghash->bufLen, buf, needed); The most common reason for contracting is to prevent line overflow, but it's true that sometimes it's just unnecessary. bob http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:517: memcpy(ghash->counterBuf, &ghash->counterBuf[GCM_HASH_LEN_LEN], On 2012/09/12 00:22:47, Ryan Sleevi wrote: > BUG? PORT_Memcpy yes http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/gcm.c:652: gcmParams->ulIvLen, blocksize); Line isn't long enough to handle the full length. the NSS standard is 80 characters. bob
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/rijndael.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/rijndael.c:1194: if (freeit && cx->worker_cx && cx->destroy) { It's ugly and has to do with psuedo private callers of freebl. If you call Destroy without 'freeit' it means you are starting with a static copy of and AES_Context. It is possible that you created a copy and zero'ed it and just started going with it. It's basically a bit of paranoia. In the case I meantioned, you should have called AES_InitContext(), which means worker_cx and destroy should be NULL, but I also know there is no case where freeit should be PR_TRUE and there is a worker_cx or a destroy field, so I'm basically protecting against the cases where someone creates and uninitialized AESContext and calls DestroyContext on it (I think I may have ran into a case or two in the tests). bob
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/ctr.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/ctr.c:75: return; Since counterBits is usually a multiple of 8, the counter rollover check needs to be performed earlier, here. I agree it is easier to just track the number of blocks processed than detecting counter bits rollover. Also, PKCS #11 wants us to stop after the counter bits become 0 ("overflow"). I think that's too strict, and problematic for GCM, whose counter bits don't always start with 0^31 || 1. This is why rsleevi and I describe the condition as "rollover". The counter bits should be allowed to wrap around as long as we don't reuse the same value.
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:123: * here's the CTS magic, we pad our final block with zeros, On 2012/09/19 21:56:02, rjrejyea wrote: > So your current understanding any my initial understanding of the PKCS #11 spec > for CTS was the same, and incorrect. The definition of CTS is on a call-for call > basis. That is each call to EncryptUpdate must have at least 1 block, and will > return a CTS encoded result of exactly the same size as the input. I'm not convinced that's the case. Calls to C_Encrypt are documented (in Table 44, in Section 6.10, of PKCS#11 2.30 Draft 7) as taking "Input Length: Any, >= block size" So a single call to C_Encrypt() should, as appropriate, finalize the block and do the stealing. However, for C_EncryptUpdate, which works in data parts, I did not think it was necessary that pPart/ulPartLen be the same length as pData/ulDataLen of C_Encrypt. That is, I haven't seen where (and I may have missed it) it's required the ulPartLen be a multiple of block size (instead of, say, 3 calls with lengths of 7 + 1 + 8) C_Encrypt explicitly mentions the CKR_DATA_LEN_RANGE error when the preconditions for the mechanism are not met, but C_EncryptUpdate does not place those requirements. However, C_EncryptFinal *does* explicitly call out CKR_DATA_LEN_RANGE, which is why I think the call sequence: C_EncryptInit C_EncryptUpdate(7) C_EncryptUpdate(1) C_EncryptUpdate(8) C_EncryptFinal() was/is a valid sequence (and which would break here) > > This is consistent with the Kerberos usage. I initially implemented something > that kept back blocks and returned the CTS result at the end of the final, but > then I looked at how PKCS #11 defined CTS and realized my understanding and > implementation were incorrect. > > bob
How do I see inter patch diffs? (I want see the difference between patchset 1 and patchset 4). bob
Bob, Please review the delta between patch sets 1 and 4 first: http://codereview.chromium.org/10919163/diff2/1:16001/nss/mozilla/security/ns... You will see that I have addressed most of rsleevi's comments in patch set 4. Then you just need to respond to the comments I didn't address in patch set 4.
On 2012/09/19 22:22:53, rjrejyea wrote: > How do I see inter patch diffs? (I want see the difference between patchset 1 > and patchset 4). > > bob The links in the column "Delta from patchset" can show you the inter-diff changes between any two patchsets. When viewing a file in inter-diff mode, you can just the navigation column right before the start of the file (keyboard navigation of j -> next, J -> next with comments, k -> prev, -> K -> prev with comments) to navigate files. If using keyboard navigation, it may mean that some files had no interdiff changes, but they will be shown (as no changes) rather than skipped entirely.
Turns out the conversation about C_EncryptUpdate has been had before, and by people much more knowledgeable on this than me: https://bugzilla.mozilla.org/show_bug.cgi?id=183146 Yay for ill-defined specs!
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:123: * here's the CTS magic, we pad our final block with zeros, You are misreading the table. If you see the AES code: C_Encrypt is 'multiple of block size'. This is the valid input for each call to C_EncryptUpdate. (It's illegal to do C_EncryptUpdate(7) C_EncryptUpdate(1) and then finalize). Note the comment: no final part. If you do 'cashing' of the partial blocks and then complete the stealing bits at the end, you have to return some data at final. The only way not to have to return data at final is to return CTS on each call to Update, which is what is implemented. bob Your sequence is not valid according to the PKCS #11 spec. bob http://codereview.chromium.org/10919163/diff/16001/nss/mozilla/security/nss/l... File nss/mozilla/security/nss/lib/freebl/ReviewComments (right): http://codereview.chromium.org/10919163/diff/16001/nss/mozilla/security/nss/l... nss/mozilla/security/nss/lib/freebl/ReviewComments:77: the AESContext structure is allocated. OK, so this is a bad idea, but it's not obvious why. AES_InitContext is pretty much used by some test programs and things like SSL Bypass mode. If you use it, you know a fair amount about Freebl because you have created your own copy of the AES_Context. Since we disuade apps from using the freebl functions directly, on pain of death, the callers are few, but SSL Bypass mode is instructive. It turns out that in some cases it someone may have created an AESContext which is uninitialized, then call freeContext with PR_FALSE in the freeit. I seem to remember tripping over such a case somewhere in SSL Bypass. My fix was not to call the destroy context calls when freeit was PR_FALSE. Anyway no new users of AES_InitContext should exist. I don't really think we need to support these mode in bypass mode, so I pretty specifically did not implement them in the AES_InitContext version. bob
http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... File nss/mozilla/security/nss/lib/freebl/cts.c (right): http://codereview.chromium.org/10919163/diff/1/nss/mozilla/security/nss/lib/f... nss/mozilla/security/nss/lib/freebl/cts.c:123: * here's the CTS magic, we pad our final block with zeros, On 2012/09/19 22:52:48, rjrejyea wrote: > You are misreading the table. If you see the AES code: > > C_Encrypt is 'multiple of block size'. This is the valid input for each call to > C_EncryptUpdate. (It's illegal to do C_EncryptUpdate(7) C_EncryptUpdate(1) and > then finalize). > > Note the comment: no final part. > > If you do 'cashing' of the partial blocks and then complete the stealing bits at > the end, you have to return some data at final. The only way not to have to > return data at final is to return CTS on each call to Update, which is what is > implemented. > > bob > > Your sequence is not valid according to the PKCS #11 spec. > > bob See linked bug - looks like Nelson read the spec the same way I did. As far as the caching, returning data at C_Final, which in the case of CTS would be the final two blocks (swapped and stolen, as appropriately), seems like a correct read under Section 11.2. "Cryptographic functions which return output in a variable-length buffer should always return as much output *as can be computed from what has been passed in to them thus far*. As an example, consider a session which is performing a multiple-part decryption operation with DES in cipher-block chaining mode with PKCS padding. Suppose that, initially, 8 bytes of ciphertext are passed to the C_DecryptUpdate function. The blocksize of DES is 8 bytes, but PKCS padding makes it unclear at this stage whether the ciphertext was produced from encrypting a 0-byte string, or from encrypting some string of length at least 8 bytes. Hence the call to C_DecryptUpdate should return 0 bytes of plaintext. If a single additional byte of ciphertext is supplied by a subsequent call to C_DecryptUpdate, then that call should return 8 bytes of plaintext (one full DES block)." While that focuses on Decrypt, and on CBC, it does seem to support the semantics I described. Note the last sentence "If a single additional byte of ciphertext is supplied" - which suggests that it is valid to call C_DecryptUpdate with one byte of input. When you look at Table 28 (Section 6.6.12), which describes general block cipher CBC with PKCS padding, it makes it clear that C_Decrypt inputs should be "multiple of blocksize". Combining the two sections, it seems that C_DecryptUpdate is not bound by the convention established in C_Decrypt. Thus, for streaming (_Init, _Update, Final), it *does* seem like the application does need to keep some degree of caching - as best it can. In the case of CTS, any time you have cached_input_len > 2*blocksize, it seems like you can return at least 1 blocksize of ciphertext.
> See linked bug - looks like Nelson read the spec the same way I did. Right, and I disagreed with him then. I did have the advantage of being in the initial PKCS #11 meetings, so I knew what the actual intent of the spec was. You will not the tables never have C_EncryptUpdate specified, but if you see the comment "no final part", you can be sure the main body refers to C_EncryptUpdate since there is no semantic meaning of 'no final part' in the C_Encrypt case since C_Encrypt is never followed by C_EncryptFinal. The PKCS #11 spec was written with hardware in mind. It purposefully pushed a lot off on the application. You'll be hard pressed not to find a PKCS #11 module that doesn't actually chock of you do a C_EncryptUpdate() of AES or DES without a multiple of blocksize for the length. As I said, I actually implemented what you are thinking initially. I then looked back at the spec and realized that is not what the spec was saying. I also know how CTS is used (basically in kerberos). It's exactly what my newer understanding of the PKCS #11 spec was saying. I am very certain my new interpretation is correct that my old one (which matches your current one) was wrong. bob
http://codereview.chromium.org/10919163/diff/16001/nss/mozilla/security/nss/l... File nss/mozilla/security/nss/lib/freebl/ReviewComments (right): http://codereview.chromium.org/10919163/diff/16001/nss/mozilla/security/nss/l... nss/mozilla/security/nss/lib/freebl/ReviewComments:77: the AESContext structure is allocated. Bob: thanks for the info. The reason for the asymmetry between AES_CreateContext and AES_InitContext and the reason for the 'freeit' check in AES_DestroyContext were not obvious at all. At first, I simply added comments to point out that AES_InitContext supports fewer modes than AES_CreateContext. Then I figured out how to eliminate that asymmetry and removed my comments. I debugged this SSL Bypass crash thoroughly last night. I summarized my findings in https://bugzilla.mozilla.org/show_bug.cgi?id=793033 The AESContext that causes the crash is actually initialized. It's just messed up by ssl_FreeSocket (in DEBUG build only) before being passed to AES_DestroyContext. |
