Fix bugs in the PKCS #11 interface to AES GCM

RESOLVED FIXED in 3.15

Status

NSS
Libraries
P1
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 6 obsolete attachments)

730 bytes, patch
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
2.74 KB, patch
Robert Relyea
: review+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
20.68 KB, patch
Robert Relyea
: review+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
4.14 KB, patch
Robert Relyea
: review+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
1.31 KB, patch
Ryan Sleevi
: review+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
3.92 KB, patch
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
3.52 KB, patch
Robert Relyea
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
Attachment #727490 - Flags: superreview?(rrelyea)
Attachment #727490 - Flags: review?(agl)

Comment 2

4 years ago
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?
Attachment #727490 - Flags: review?(agl) → review+
(Assignee)

Comment 3

4 years ago
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.
Attachment #727901 - Flags: review?(rrelyea)
(Assignee)

Comment 4

4 years ago
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.
Attachment #727901 - Attachment is obsolete: true
Attachment #727901 - Flags: review?(rrelyea)
Attachment #727902 - Flags: review?(rrelyea)
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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
Attachment #727490 - Attachment is obsolete: true
Attachment #727490 - Flags: superreview?(rrelyea)
(Assignee)

Updated

4 years ago
Attachment #727915 - Flags: checked-in+
(Assignee)

Comment 7

4 years ago
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.
Attachment #727924 - Flags: review?(rrelyea)
(Assignee)

Comment 8

4 years ago
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
Attachment #727902 - Attachment is obsolete: true
Attachment #727902 - Flags: review?(rrelyea)
Attachment #729193 - Flags: review?
Attachment #729193 - Flags: checked-in+
(Assignee)

Comment 9

4 years ago
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.
Attachment #729245 - Flags: review?(rrelyea)
(Assignee)

Comment 10

4 years ago
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.
Attachment #729245 - Flags: checked-in+
(Assignee)

Comment 11

4 years ago
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
Attachment #729316 - Flags: review?(rrelyea)
Attachment #729316 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Attachment #729193 - Flags: review? → review?(rrelyea)
(Assignee)

Comment 12

4 years ago
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.
Attachment #730389 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #730389 - Flags: review? → review?(ryan.sleevi)

Updated

4 years ago
Attachment #730389 - Flags: review?(ryan.sleevi) → review+
(Assignee)

Comment 13

4 years ago
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
Attachment #730389 - Attachment is obsolete: true
Attachment #730851 - Flags: review?(ryan.sleevi)
Attachment #730851 - Flags: checked-in+
(Assignee)

Comment 14

4 years ago
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.
Attachment #730851 - Attachment is obsolete: true
Attachment #730851 - Flags: review?(ryan.sleevi)
Attachment #730853 - Flags: review?(ryan.sleevi)
Attachment #730853 - Flags: checked-in+

Comment 15

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

bob

Updated

4 years ago
Attachment #730853 - Flags: review?(ryan.sleevi) → review+

Comment 16

4 years ago
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
Attachment #727924 - Flags: review?(rrelyea) → review+

Comment 17

4 years ago
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+.
Attachment #729193 - Flags: review?(rrelyea) → review+

Comment 18

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

r+ rrelyea
Attachment #729245 - Flags: review?(rrelyea) → review+

Comment 19

4 years ago
Comment on attachment 729316 [details] [diff] [review]
Add the test files for AES GCM Test Cases 1, 7, 13

r+ rrelyea
Attachment #729316 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 20

4 years ago
(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
(Assignee)

Comment 21

4 years ago
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
Attachment #727924 - Attachment is obsolete: true
Attachment #733102 - Flags: checked-in+
(Assignee)

Comment 22

4 years ago
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.
Attachment #733123 - Flags: review?(rrelyea)

Comment 23

4 years ago
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
Attachment #733123 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 24

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.