Add GCM, CTR, and CTS modes to AES.

nss/mozilla/security/nss/lib/freebl/ReviewComments

Index: nss/mozilla/security/nss/lib/freebl/ReviewComments
===================================================================
--- nss/mozilla/security/nss/lib/freebl/ReviewComments	(revision 0)
+++ nss/mozilla/security/nss/lib/freebl/ReviewComments	(revision 0)
@@ -0,0 +1,98 @@
+Issues:
+
+1. The |param| input parameter to CTR_CreateContext, CTR_InitContext, etc.
+should be a const void * or a pointer to a concrete type, rather than
+const unsigned char *. Should we change the type to const void * or a
+a pointer to a concrete type, such as const CK_AES_CTR_PARAMS * ?
+
+2. lib/freebl should not use PKCS #11 types defined in pkcs11t.h, such as
+CK_AES_CTR_PARAMS. In addition, ctr.c was written to support any suitable
+block cipher, but CK_AES_CTR_PARAMS is AES-specific.
+
+3. ctr_GetNextCtr does not handle counter rollover.
+
+4. Only single-part operations are supported for CTS and GCM.
+CTS_EncryptUpdate can only be called once on each operation context.
+
+5. Why is the XOR operation defined as a function (ctr_xor) in CTR but as
+a macro (XOR_BLOCK and GCM_XOR) in CTS and GCM?
+
+6. The GCM code for 64-bit block ciphers is not tested. I recommend removing
+it.
+
+7. CK_AES_GCM_PARAMS and CK_AES_CCM_PARAMS were renamed CK_GCM_PARAMS and
+CK_CCM_PARAMS in PKCS #11 v2.30 draft.
+
+8. In ctr.c, ctr->bufPtr satisfies the following invariant:
+   0 < ctr->bufPtr <= blocksize
+I can make ctr->bufPtr behave like ghash->bufLen in gcm.c. Want me to do
+that?
+
+9. I found two bugs in CTS_DecryptUpdate where you convert the input buffer
+from CS-1 to CS-2. Since you didn't provide tests for CTS, I didn't try to
+fix these bugs.
+
+======================================================================
+
+Patch set 1:
+
+Original patch by rrelyea, except manifest.mn.
+
+======================================================================
+
+Patch set 2:
+
+Move lib/util/pkcs11t.h to this patch because lib/freebl needs the
+pkcs11t.h changes.
+
+======================================================================
+
+Patch set 3:
+
+Remove the RCS Id lines from new files. I seem to remember we decided to
+remove RCS Id lines from NSS source files.
+
+Add comments. Remove blank lines, spaces at the end of lines. Fix typos.
+
+Change freeblDestroyFunc to return void instead of SECStatus.
+
+Check that blocksize is <= the sizes of various block buffers.
+
+Change BITS_PER_BYTE (deprecated NSPR macro name) to PR_BITS_PER_BYTE.
+
+Fix a bug in ctr_GetNextCtr: *counterPtr should be pre-incremented.
+
+CTR_Update does not need the local cipherblock buffer. It can just use
+ctr->buffer when ctr->buffer is empty.
+
+Mark internal functions as static.
+
+Change some variables of type int to unsigned int.
+
+Use NSS_SecureMemcmp instead of PORT_Memcmp to check the GCM authentication
+tag.
+
+Make AES_InitContext support the three new modes (CTS, CTR, and GCM),
+so that AES_CreateContext and AES_InitContext differ only in whether
+the AESContext structure is allocated.

[rjrejyea, 2012/09/19 22:52:48]

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

[wtc, 2012/09/20 23:27:23]

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.

+
+AES_DestroyContext should still call cx->destroy when freeit is false.
+
+======================================================================
+
+Patch set 3:

[wtc, 2012/09/18 01:03:40]

This is a typo. This should say "Patch set 4".

+
+Add comments. Fix typos.
+
+Change some variables of type int to unsigned int.
+
+Improve the input validation in CTR_InitContext.
+
+Fix a bug in the generation of pre-counter block for 64-bit block ciphers
+in GCM_CreateContext. Note that the spec is gcm-revised-spec.pdf, dated
+May 31, 2005.
+
+Declare poly_128 and poly_64 as const.
+
+Fix a 32-bit bug in GCM code. cLen must be a 64-bit unsigned integer, so
+declaring cLen as unsigned long is not portable.