853285 - Fix bugs in the PKCS #11 interface to AES GCM

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: 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]

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]

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]

Attachment: 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]

Attachment: 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]

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]

Attachment: 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]

Attachment: 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]

Attachment: 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]

Attachment: 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]

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]

Attachment: 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]

Attachment: 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]

Attachment: 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]

Attachment: [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]

I just want to confirm that WTC's naming logic for the new functions is sound.

bob

Comment 16

[Robert Relyea]

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]

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]

Comment on attachment 729245 [details] [diff] [review]
Fix AES GCM tests

r+ rrelyea

Comment 19

[Robert Relyea]

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]

(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]

Attachment: 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]

Attachment: 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]

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]

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.