Closed Bug 854063 Opened 7 years ago Closed 7 years ago

Add new pk11wrap functions for AES GCM

Categories

(NSS :: Libraries, enhancement, P1)

3.14
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(5 files, 3 obsolete files)

In the softoken, CKM_AES_GCM is marked as not supporting
multi-part operations:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11c.c&rev=1.138&mark=847-849#844

So PK11_CipherOp fails if used with CKM_AES_GCM:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11c.c&rev=1.138&mark=343,345#325

There are two approaches to solve this problem.

1. Still use PK11_CreateContextBySymKey to create a PK11Context,
and add a single-part variant of PK11_CipherOp. The first patch
implements this approach.

2. Add PK11_EncryptWithSymKey and PK11_DecryptWithSymKey,
analogous to the existing PK11_Sign, PK11_SignWithSymKey,
PK11_PubEncryptPKCS1, PK11_PrivDecryptPKCS1 functions.
The second patch implements this approach.

Which approach do you prefer?
Attachment #728555 - Flags: review?(rrelyea)
Attachment #728555 - Flags: feedback?(agl)
This approach has another API design decision: whether the input
and output buffers should be specified using unsigned char* buffers
and unsigned int lengths, or using SECItems.

PK11_PubEncryptPKCS1, PK11_PrivDecryptPKCS1 use the former. This is
what I use in this patch, but I only have a slight preference for
this.

PK11_Sign, PK11_SignWithSymKey use the latter.
Attachment #728557 - Flags: review?(rrelyea)
Attachment #728557 - Flags: feedback?(agl)
I don't think that I have enough understanding of the space to have a useful judgement about the merits of these patches. Although I would note that I'm not sure that I would have found a function called PK11_SinglePartCipherOp given a desire to find a function for encryption or decryption.
Note: you can skip the following lengthy rationale if you are
not interested in how I chose the function names and prototypes.

Based on the comments of Adam and Ryan (reviewed in Chromium's
code review tool), I decided to add PK11_Decrypt and PK11_Encrypt,
with output and input buffers specified using five pointer and
length arguments rather than two SECItem arguments.

I chose the five-argument function prototype because the input
and output for encryption and decryption usually don't come in
SECItems and the five-argument function prototype is commonly
used with the symmetric key cipher functions in NSS.  In
addition, SECItem's |data| member would require the caller to
cast away the 'const' of the input buffer.

Since the recently added PK11_SignWithSymKey uses the two-SECItem
function prototype, I omitted "WithSymKey" in the function names
to avoid being associated with PK11_SignWithSymKey.

I found three pairs of functions where PK11_Foo is the
symmetric key version and PK11_PubFoo is the public key
version:
  PK11_Derive and PK11_PubDerive
  PK11_WrapSymKey and PK11_PubWrapSymKey
  PK11_UnwrapSymKey and PK11_PubUnwrapSymKey

So PK11_Decrypt and PK11_Encrypt are following this
naming convention and are intended to be associated with
PK11_PrivDecryptRaw/PKCS11 and PK11_PubEncryptRaw/PKCS11.
Attachment #728555 - Attachment is obsolete: true
Attachment #728557 - Attachment is obsolete: true
Attachment #728555 - Flags: review?(rrelyea)
Attachment #728555 - Flags: feedback?(agl)
Attachment #728557 - Flags: review?(rrelyea)
Attachment #728557 - Flags: feedback?(agl)
Attachment #730841 - Flags: superreview?(rrelyea)
Attachment #730841 - Flags: review?(ryan.sleevi)
Comment on attachment 730841 [details] [diff] [review]
Add PK11_Decrypt and PK11_Encrypt

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

r=rsleevi

I agree that the choice of SECItem vs unsigned char* is tricky. I think you've provided good justification why to use the latter rather than the former (which I originally preferred)
Attachment #730841 - Flags: review?(ryan.sleevi) → review+
Comment on attachment 730841 [details] [diff] [review]
Add PK11_Decrypt and PK11_Encrypt

Please leave the bug open until NSS 3.15 is released. I'd
like to add an AES GCM test program that uses PK11_Decrypt and
PK11_Encrypt.  (Such tests exist in the Chromium tree, but
NSS itself should also have them.)

https://hg.mozilla.org/projects/nss/rev/75b566f18970
Attachment #730841 - Flags: checked-in+
This patch adds a new test program nss/cmd/pk11gcmtest that
does AES GCM using the new PK11_Decrypt and PK11_Encrypt
functions.  It is based on nss/cmd/fipstest.  I am using the
NIST algorithm validation "RESPONSE" files as test input with
expected results.

Note that each test response file is about 3 MB.  So with six
test response files we are looking at 18 MB.

wtc@fips:~/nss-tip.5/nss/cmd/pk11gcmtest/tests$ ls -l
total 17008
-rw-r----- 1 wtc eng 2682450 Apr  1 16:38 gcmDecrypt128.rsp
-rw-r----- 1 wtc eng 2812795 Apr  1 16:38 gcmDecrypt192.rsp
-rw-r----- 1 wtc eng 2935620 Apr  1 16:38 gcmDecrypt256.rsp
-rw-r----- 1 wtc eng 2864783 Apr  1 16:38 gcmEncryptExtIV128.rsp
-rw-r----- 1 wtc eng 2990783 Apr  1 16:38 gcmEncryptExtIV192.rsp
-rw-r----- 1 wtc eng 3116783 Apr  1 16:38 gcmEncryptExtIV256.rsp
-rw-r----- 1 wtc eng     268 Apr  1 16:28 README

I suspect this is too big for the NSS source tree.

The contents of gcmDecrypt128.rsp look like:
# CAVS 14.0
# GCM Decrypt with keysize 128 test information
# Generated on Fri Aug 31 11:28:04 2012



[Keylen = 128]
[IVlen = 96]
[PTlen = 0]
[AADlen = 0]
[Taglen = 128]

Count = 0
Key = cf063a34d4a9a76c2c86787d3f96db71
IV = 113b9785971864c83b01c787
CT =
AAD =
Tag = 72ac8493e3a5228b5d130a69d2510e42
PT =

Count = 1
Key = a49a5e26a2f8cb63d05546c2a62f5343
IV = 907763b19b9b4ab6bd4f0281
CT =
AAD =
Tag = a2be08210d8c470a8df6e8fbd79ec5cf
FAIL

...

The contents of gcmEncryptExtIV128.rsp look like:
# CAVS 14.0
# GCM Encrypt with keysize 128 test information
# Generated on Fri Aug 31 11:23:06 2012



[Keylen = 128]
[IVlen = 96]
[PTlen = 0]
[AADlen = 0]
[Taglen = 128]

Count = 0
Key = 11754cd72aec309bf52f7687212e8957
IV = 3c819d9a9bed087615030b65
PT =
AAD =
CT =
Tag = 250327c674aaf477aef2675748cf6971

Count = 1
Key = ca47248ac0b6f8372a97ac43508308ed
IV = ffd2b598feabc9019262d2be
PT =
AAD =
CT =
Tag = 60d20404af527d248d893ae495707d1a

...
Attachment #732127 - Flags: superreview?
Attachment #732127 - Flags: review?
Comment on attachment 732127 [details] [diff] [review]
AES GCM test program that calls PK11_Decrypt and PK11_Encrypt

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

Please see comment 7 about this patch.
Attachment #732127 - Flags: superreview?(rrelyea)
Attachment #732127 - Flags: superreview?
Attachment #732127 - Flags: review?(ryan.sleevi)
Attachment #732127 - Flags: review?
Comment on attachment 732127 [details] [diff] [review]
AES GCM test program that calls PK11_Decrypt and PK11_Encrypt

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

I didn't review the makefiles (other than they look sane - but I'm far from one who understands the NSS makefile/manifests), but r+ for the other bits.
Attachment #732127 - Flags: review?(ryan.sleevi) → review+
This patch includes the test input files.

The original test input files have 525 x 15 test vectors each.
There are 525 sets of parameters (Keylen, IVlen, PTlen, AADlen, Taglen),
and each set has 15 test vectors.

I figured out the patterns in AADlen and Taglen.  I kept only
the representative values of AADlen and Taglen and pruned the
others.  In each set I kept just the first two test vectors.
The resulting test input files have 46 x 2 test vectors each.
I describe my pruning method in nss/cmd/pk11gcmtest/tests/README.
Attachment #732127 - Attachment is obsolete: true
Attachment #732127 - Flags: superreview?(rrelyea)
Attachment #732533 - Flags: review?(rrelyea)
The zip file that contains the GCM Test Vectors I downloaded
from NIST is too big to attach to Bugzilla.

I used this C program to prune the test input files.  It is
a modified version of pk11gcmtest.c.
Comment on attachment 732547 [details]
prunetest.c: program for pruning the GCM Test Vectors from NIST

Some notes on prunetest.c, if we need to run it again.

>	/* [Keylen = ...], [IVlen = ...], etc. */
>	if (buf[0] == '[') {
>	    if (strncmp(&buf[1], "Keylen = ", 9) == 0) {
>		unsigned int aad_group;
>		expected_keylen = atoi(&buf[10]);
>		test_group++;
>		if (test_group == 1) {
>		    fputs("\n", stdout);
>		}
>		aad_group = (test_group - 1) / 7;
>		include_this_test_group =
>		    (test_group % 7 == 1 || test_group % 7 == 5) &&
>		    (aad_group % 10 == 0 || aad_group % 10 == 4 || aad_group % 10 == 7);

This is where I select which Taglen and AADlen values to keep.

(test_group % 7 == 1 || test_group % 7 == 5) selects Taglen = 128, 96 from
each cycle of 7.  Each cycle of 7 is Taglen = 128, 120, 112, 104, 96, 64, 32.

(aad_group % 10 == 0 || aad_group % 10 == 4 || aad_group % 10 == 7) selects
AADlen = 0, 720, 160 from each two cycles of 10.  Each cycle of 5 is
AADlen = 0, 128, 160, 384, 720.

>	if (num_tests >= 2) {
>	    continue;
>	}

This is where I keep only the first two test vectors in each set.
Comment on attachment 732533 [details] [diff] [review]
AES GCM test program that calls PK11_Decrypt and PK11_Encrypt, v2

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

https://hg.mozilla.org/projects/nss/rev/f92ca36ccbb8

This changeset includes an unrelated change to lib/freebl/gcm.c
that Bob asked for in bug 853285 comment 17.  Sorry about the
mistake.
Attachment #732533 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
My new pk11gcmtest crashed on the buildbot 3-android-a32-OPT.
Kai, could you help me look at it?  Do I need to copy the
new test input files to the Android device?  Thanks.

cipher.sh: NIST AES128 GCM Decrypt --------------------------------
pk11gcmtest aes kat gcm /data/data/berserker.android.apps.sshdroid/home/nsstest/nss/tests/../cmd/pk11gcmtest/tests/gcmDecrypt128.rsp
Segmentation fault
cipher.sh: #41: NIST AES128 GCM Decrypt  - FAILED
cipher.sh: NIST AES192 GCM Decrypt --------------------------------
pk11gcmtest aes kat gcm /data/data/berserker.android.apps.sshdroid/home/nsstest/nss/tests/../cmd/pk11gcmtest/tests/gcmDecrypt192.rsp
Segmentation fault
cipher.sh: #42: NIST AES192 GCM Decrypt  - FAILED
cipher.sh: NIST AES256 GCM Decrypt --------------------------------
pk11gcmtest aes kat gcm /data/data/berserker.android.apps.sshdroid/home/nsstest/nss/tests/../cmd/pk11gcmtest/tests/gcmDecrypt256.rsp
Segmentation fault
cipher.sh: #43: NIST AES256 GCM Decrypt  - FAILED
cipher.sh: NIST AES128 GCM Encrypt --------------------------------
pk11gcmtest aes kat gcm /data/data/berserker.android.apps.sshdroid/home/nsstest/nss/tests/../cmd/pk11gcmtest/tests/gcmEncryptExtIV128.rsp
Segmentation fault
cipher.sh: #44: NIST AES128 GCM Encrypt  - FAILED
cipher.sh: NIST AES192 GCM Encrypt --------------------------------
pk11gcmtest aes kat gcm /data/data/berserker.android.apps.sshdroid/home/nsstest/nss/tests/../cmd/pk11gcmtest/tests/gcmEncryptExtIV192.rsp
Segmentation fault
cipher.sh: #45: NIST AES192 GCM Encrypt  - FAILED
cipher.sh: NIST AES256 GCM Encrypt --------------------------------
pk11gcmtest aes kat gcm /data/data/berserker.android.apps.sshdroid/home/nsstest/nss/tests/../cmd/pk11gcmtest/tests/gcmEncryptExtIV256.rsp
Segmentation fault
cipher.sh: #46: NIST AES256 GCM Encrypt  - FAILED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #732599 - Flags: superreview?(ryan.sleevi)
Attachment #732599 - Flags: review?(kaie)
Attachment #732604 - Flags: review?(ryan.sleevi)
Attachment #732604 - Flags: review?(ryan.sleevi) → review+
Comment on attachment 732604 [details] [diff] [review]
Check for fopen() failure

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

https://hg.mozilla.org/projects/nss/rev/83fcc09f85a3

(I also changed exit(-1) to exit(1) and added 'const' to a
char * input argument.)
Attachment #732604 - Flags: checked-in+
Attachment #732599 - Flags: review?(kaie) → review+
Comment on attachment 732599 [details] [diff] [review]
Package nss/cmd/bltest/tests for testing on an Android device

https://hg.mozilla.org/projects/nss/rev/97dd887d9dae
Attachment #732599 - Flags: checked-in+
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Comment on attachment 730841 [details] [diff] [review]
Add PK11_Decrypt and PK11_Encrypt

r+ rrelyea.
Attachment #730841 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 732533 [details] [diff] [review]
AES GCM test program that calls PK11_Decrypt and PK11_Encrypt, v2

r+, though it would be nice to make this generic. It tests PK11_Decrypt/PK11_Encrypt by using GCM, it might be good to make it test others ciphers as well.

OTHO, I never turn down a new test in all.sh;).

bob
Attachment #732533 - Flags: review?(rrelyea) → review+
(In reply to Robert Relyea from comment #20)
> Comment on attachment 732533 [details] [diff] [review]
> AES GCM test program that calls PK11_Decrypt and PK11_Encrypt, v2
> 
> r+, though it would be nice to make this generic. It tests
> PK11_Decrypt/PK11_Encrypt by using GCM, it might be good to make it test
> others ciphers as well.

I agree this is a good idea. Some of the code in bltest and fipstest
doesn't match exactly how the softoken uses those freebl functions.
It'd be better to exercise the softoken directly.

But even adding the GCM test took me 2-3 days. So I didn't work on
other algorithms. The next algorithm I work on may be RSA-PSS.
No problem, the r+ stands since the patch moves tests forward.
Attachment #732599 - Flags: superreview?(ryan.sleevi) → superreview+
You need to log in before you can comment on or make changes to this bug.