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 wh
wtc
2012/09/20 23:27:23
Bob: thanks for the info. The reason for the asymm
|
+ |
+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. |