Last Comment Bug 853285 - Fix bugs in the PKCS #11 interface to AES GCM
: Fix bugs in the PKCS #11 interface to AES GCM
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.14
: All All
: P1 major (vote)
: 3.15
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-20 19:21 PDT by Wan-Teh Chang
Modified: 2013-05-22 09:36 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch that fixes bugs (3.41 KB, patch)
2013-03-20 19:21 PDT, Wan-Teh Chang
agl: review+
Details | Diff | Review
Fix AES GCM bug with zero-length AAD and plaintext (8.81 KB, patch)
2013-03-21 14:37 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Fix AES GCM bug with zero-length AAD and plaintext, v2 (9.55 KB, patch)
2013-03-21 14:39 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Update PK11_GetKeyType to handle CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM (730 bytes, patch)
2013-03-21 15:15 PDT, Wan-Teh Chang
wtc: checked‑in+
Details | Diff | Review
Update more pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM (2.91 KB, patch)
2013-03-21 15:30 PDT, Wan-Teh Chang
rrelyea: review+
Details | Diff | Review
Fix AES GCM bug with zero-length AAD and plaintext, v3 (2.74 KB, patch)
2013-03-25 14:07 PDT, Wan-Teh Chang
rrelyea: review+
wtc: checked‑in+
Details | Diff | Review
Fix AES GCM tests (20.68 KB, patch)
2013-03-25 15:28 PDT, Wan-Teh Chang
rrelyea: review+
wtc: checked‑in+
Details | Diff | Review
Add the test files for AES GCM Test Cases 1, 7, 13 (4.14 KB, patch)
2013-03-25 17:26 PDT, Wan-Teh Chang
rrelyea: review+
wtc: checked‑in+
Details | Diff | Review
Don't call PORT_Memcpy with a NULL source buffer pointer (776 bytes, patch)
2013-03-27 15:45 PDT, Wan-Teh Chang
ryan.sleevi: review+
Details | Diff | Review
Don't call PORT_Memcpy with a NULL source buffer pointer. Use a better error code when the input buffer is too short. (2.76 KB, patch)
2013-03-28 13:34 PDT, Wan-Teh Chang
wtc: checked‑in+
Details | Diff | Review
[Correct patch] Don't call PORT_Memcpy with a NULL source buffer pointer. Use a better error code when the input buffer is too short. (1.31 KB, patch)
2013-03-28 13:35 PDT, Wan-Teh Chang
ryan.sleevi: review+
wtc: checked‑in+
Details | Diff | Review
Update more pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM, v2 (3.92 KB, patch)
2013-04-03 16:38 PDT, Wan-Teh Chang
wtc: checked‑in+
Details | Diff | Review
Update the remaining pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM (3.52 KB, patch)
2013-04-03 17:07 PDT, Wan-Teh Chang
rrelyea: review+
Details | Diff | Review

Description Wan-Teh Chang 2013-03-20 19:21:12 PDT
Created attachment 727490 [details] [diff] [review]
Patch that fixes bugs

AES GCM was added in NSS 3.14 (bug 373108). Unfortunately we only
tested the low-level freebl interface but not the PKCS #11 interface.
So there are some bugs. In addition, we disallow multi-part operations
for CKM_AES_GCM, but there are no PK11_xxx functions that invoke
C_Encrypt and C_Decrypt. The closest thing is PK11_CipherOp, which
invokes C_EncryptUpdate and C_DecryptUpdate.

The attached patch fixes all the problems except the lack of a
PK11_xxx function that invokes C_Encrypt and C_Decrypt. If I
comment out the code that disallows multi-part operations for
CKM_AES_GCM, then this patch allows me to use PK11_CipherOp to
do AES GCM.
Comment 1 Wan-Teh Chang 2013-03-20 19:29:20 PDT
Comment on attachment 727490 [details] [diff] [review]
Patch that fixes bugs

Review of attachment 727490 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/gcm.c
@@ +229,5 @@
>      if (err < 0) {
>  	PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>  	return SECFailure;
>      }
> +    gcm_reverse(T, tmp_buf, blocksize);

Note that we may increment X on line 225. So X does not always
point to the beginning of tmp_buf. Here we must pass the
beginning of tmp_buf to gcm_reverse().

I discovered this bug when I tried to do AES GCM encryption with
zero-length AAD and zero-length plaintext. In this case, len=1
because ghash->X is 0 (one digit, which is 0) on entering this
function, so X is incremented on line 225.

Bob: do you know why this bug wasn't detected by our GCM test
vectors?

@@ +582,1 @@
>      }

This change is not necessary to fix the zero-length AAD, zero-length
plaintext bug. But I think it is clearer to still call gcmHash_Sync
when AADLen is 0. In fact, we can also call gcmHash_Update when AADLen
is 0. Do you want me to make that change?

::: lib/freebl/rijndael.c
@@ +1220,5 @@
>              const unsigned char *input, unsigned int inputLen)
>  {
>      int blocksize;
>      /* Check args */
> +    if (cx == NULL || output == NULL || (input == NULL && inputLen != 0)) {

The two changes in this file allow us to represent a zero-length
input with input=NULL, inputLen=0.
Comment 2 Adam Langley 2013-03-21 07:31:35 PDT
Comment on attachment 727490 [details] [diff] [review]
Patch that fixes bugs

Review of attachment 727490 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pk11wrap/pk11mech.c
@@ +223,5 @@
>      case CKM_AES_CBC:
>      case CKM_AES_MAC:
>      case CKM_AES_MAC_GENERAL:
>      case CKM_AES_CBC_PAD:
> +    case CKM_AES_CTR:

Locally I also added CKM_AES_CCM here. Should that be included in this patch?
Comment 3 Wan-Teh Chang 2013-03-21 14:37:22 PDT
Created attachment 727901 [details] [diff] [review]
Fix AES GCM bug with zero-length AAD and plaintext

This patch fixes the AES GCM bug with zero-length AAD and plaintext.
The patch includes the following.

1. New AES GCM tests (numbered 15 and 16), with zero-length AAD and
plaintext. I got these test vectors from the NIST CMVP website.

2. Bob's hex.c program, which is needed to convert the test source
test<n>.txt to the test files.

3. The actual bug fix is in lib/freebl/gcm.c and
lib/freebl/rijndael.c.

4. Portions of Bob's patch "Tests for CTR and GCM V2" from
bug 373108 comment 66 that haven't been checked in for some
reason. Because of this mistake, we are not running the new
AES CTS, CTR, GCM, and DSA-2 tests yet.
Comment 4 Wan-Teh Chang 2013-03-21 14:39:51 PDT
Created attachment 727902 [details] [diff] [review]
Fix AES GCM bug with zero-length AAD and plaintext, v2

I forgot to include the cleanup change in gcmHash_Reset.
Comment 5 Wan-Teh Chang 2013-03-21 15:09:33 PDT
Comment on attachment 727490 [details] [diff] [review]
Patch that fixes bugs

Review of attachment 727490 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pk11wrap/pk11mech.c
@@ +223,5 @@
>      case CKM_AES_CBC:
>      case CKM_AES_MAC:
>      case CKM_AES_MAC_GENERAL:
>      case CKM_AES_CBC_PAD:
> +    case CKM_AES_CTR:

Good idea. Will do.
Comment 6 Wan-Teh Chang 2013-03-21 15:15:34 PDT
Created attachment 727915 [details] [diff] [review]
Update PK11_GetKeyType to handle CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM

Pushed to the NSS hg repository (NSS 3.15):
https://hg.mozilla.org/projects/nss/rev/aeac2806263b
Comment 7 Wan-Teh Chang 2013-03-21 15:30:14 PDT
Created attachment 727924 [details] [diff] [review]
Update more pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM

agl's comment reminded me to do an exhaustive search for
CKM_AES_CBC in lib/pk11wrap and see if those functions
also need to handle CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS,
and CKM_AES_GCM.

This patch handles the obvious changes.
Comment 8 Wan-Teh Chang 2013-03-25 14:07:02 PDT
Created attachment 729193 [details] [diff] [review]
Fix AES GCM bug with zero-length AAD and plaintext, v3

I figured out what was wrong with the AES GCM test vectors.
I will deal with that separately. This patch contains just
the bug fix.

Patch pushed to the NSS hg repository (NSS 3.15):
https://hg.mozilla.org/projects/nss/rev/c54516454820
Comment 9 Wan-Teh Chang 2013-03-25 15:28:58 PDT
Created attachment 729245 [details] [diff] [review]
Fix AES GCM tests

I looked at nss/cmd/bltest/tests/aes_gcm/ more closely and
saw what was wrong with the test cases. This patch fixes the
problems.

1. Add the portions of Bob's patch "Tests for CTR and GCM V2"
from bug 373108 comment 66 that haven't been checked in.
Because of this mistake, we are not running the new AES CTS,
CTR, GCM, and DSA-2 tests yet.

2. Fix the errors in Test Case 7.

3. Generate the test files for test cases 1, 7, and 13.
(These test cases all have zero-length AAD and plaintext
and would have caught this bug.) This requires renaming
many of the existing test files.

4. Add Bob's hex.c program, which is needed to convert the
test source test<n>.txt to the test files.
Comment 10 Wan-Teh Chang 2013-03-25 15:41:44 PDT
Comment on attachment 729245 [details] [diff] [review]
Fix AES GCM tests

Patch pushed to the NSS hg repository (NSS 3.15):
https://hg.mozilla.org/projects/nss/rev/1d30b6fde2a6

I will make any change that Bob asks for.
Comment 11 Wan-Teh Chang 2013-03-25 17:26:50 PDT
Created attachment 729316 [details] [diff] [review]
Add the test files for AES GCM Test Cases 1, 7, 13

The files are numbered 0, 6, 12 (counting from 0).

For some reason I didn't see these new files when I
did "hg status" last time. Sorry about my mistake.

Patch pushed to the NSS hg repository (NSS 3.15):
https://hg.mozilla.org/projects/nss/rev/c3f285fed43e
Comment 12 Wan-Teh Chang 2013-03-27 15:45:11 PDT
Created attachment 730389 [details] [diff] [review]
Don't call PORT_Memcpy with a NULL source buffer pointer

Ryan pointed out that the Standard C library functions
still require a valid pointer arguments when the "size_t n"
argument is 0.  Now that AES_Encrypt and AES_Decrypt allow
input=NULL inputLen=0, this becomes an issue.

I audited the three AES-related files in lib/freebl:
rijndael.c, ctr.c, and gcm.c.  I searched for "emc" to cover
both memc* and PORT_Memc*.  (The |input| pointer is const,
so it can't be passed to memset, only to memcpy and memcmp.)

This is the only PORT_Memcpy call that need to be protected.
I can also add an early return
    if (len == 0) {
        return SECSuccess;
    }
at the beginning of the gcmHash_Update function.  Would you
like me to do that instead?  Thanks.
Comment 13 Wan-Teh Chang 2013-03-28 13:34:23 PDT
Created attachment 730851 [details] [diff] [review]
Don't call PORT_Memcpy with a NULL source buffer pointer. Use a better error code when the input buffer is too short.

Ryan: I found an error code problem while reviewing the buffer
size checks in our AES code. So I fixed that as well.

https://hg.mozilla.org/projects/nss/rev/956d2677f6f0
Comment 14 Wan-Teh Chang 2013-03-28 13:35:40 PDT
Created attachment 730853 [details] [diff] [review]
[Correct patch] Don't call PORT_Memcpy with a NULL source buffer pointer. Use a better error code when the input buffer is too short.
Comment 15 Robert Relyea 2013-03-28 13:59:16 PDT
I just want to confirm that WTC's naming logic for the new functions is sound.

bob
Comment 16 Robert Relyea 2013-04-01 16:14:57 PDT
Comment on attachment 727924 [details] [diff] [review]
Update more pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM

r+ 

Also fix -CK_MECHANISM_TYPE PK11_GetKeyType() in pk11pub.h

bob
Comment 17 Robert Relyea 2013-04-01 16:25:01 PDT
Comment on attachment 729193 [details] [diff] [review]
Fix AES GCM bug with zero-length AAD and plaintext, v3

r+, but the movinging the gcmHash_Sync() out of the if is not strictly necessary. gcmHash_Sync() moves the low portion of the count buffer to the high portion, then copies the current count into the low portion. If there are no AAD data, then the count is zero, so there is no need to move any counts around.

It's not wrong to call it here, it's just inefficient, thus the r+.
Comment 18 Robert Relyea 2013-04-01 16:26:34 PDT
Comment on attachment 729245 [details] [diff] [review]
Fix AES GCM tests

r+ rrelyea
Comment 19 Robert Relyea 2013-04-01 16:27:22 PDT
Comment on attachment 729316 [details] [diff] [review]
Add the test files for AES GCM Test Cases 1, 7, 13

r+ rrelyea
Comment 20 Wan-Teh Chang 2013-04-02 15:14:49 PDT
(In reply to Robert Relyea from comment #17)
> Comment on attachment 729193 [details] [diff] [review]
> Fix AES GCM bug with zero-length AAD and plaintext, v3
> 
> r+, but the movinging the gcmHash_Sync() out of the if is not strictly
> necessary. gcmHash_Sync() moves the low portion of the count buffer to the
> high portion, then copies the current count into the low portion. If there
> are no AAD data, then the count is zero, so there is no need to move any
> counts around.
> 
> It's not wrong to call it here, it's just inefficient, thus the r+.

I knew it wasn't necessary to move the gcmHash_Sync() out of the if.
I wasn't sure if you deliberately put the gcmHash_Sync() inside the if,
so I made the change to make the code clearer.

I moved the gcmHash_Sync() back into the if in
https://hg.mozilla.org/projects/nss/rev/f92ca36ccbb8
Comment 21 Wan-Teh Chang 2013-04-03 16:38:35 PDT
Created attachment 733102 [details] [diff] [review]
Update more pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM, v2

https://hg.mozilla.org/projects/nss/rev/eca0c48e59c1
Comment 22 Wan-Teh Chang 2013-04-03 17:07:20 PDT
Created attachment 733123 [details] [diff] [review]
Update the remaining pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM

Bob, please review this patch carefully.

These are the remaining pk11wrap functions that may need to
be updated for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and
CKM_AES_GCM.  I am less certain about these functions.

1. I changed PK11_GetBlockSize to return 0 for these four
new AES mechanisms because their input doesn't need to be
a multiple of AES block size.  Is this correct?

2. All the other functions are related to IV or Algid.
I only added CKM_AES_CTS to these functions.  I am not
sure if that's right.  In particular, the IV for CBC mode
is different from the IV/nonce for GCM, which doesn't
need to be an AES block size long.
Comment 23 Robert Relyea 2013-04-04 17:03:57 PDT
Comment on attachment 733123 [details] [diff] [review]
Update the remaining pk11wrap functions for CKM_AES_CCM, CKM_AES_CTR, CKM_AES_CTS, and CKM_AES_GCM

r+ rrelyea
Comment 24 Wan-Teh Chang 2013-05-22 09:36:41 PDT
I moved the last patch (attachment 733123 [details] [diff] [review]) to bug 874940
so I have more time to take a closer look at it myself.

Marked the bug fixed.

Note You need to log in before you can comment on or make changes to this bug.