Closed
Bug 471665
Opened 16 years ago
Closed 15 years ago
NSS reports incorrect sizes for (AES) symmetric keys
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: david.konrad.stutzman, Assigned: nelson)
Details
(Whiteboard: FIPS)
Attachments
(1 file, 1 obsolete file)
1.03 KB,
patch
|
rrelyea
:
review+
wtc
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729) Build Identifier: NSS 3.12 RTM It appears NSS 3.12 (on Vista 32-bit) has issues reporting key sizes both to Java and using symkeyutil directly: *** NSS 3.12 output *** C:\nss\fips>symkeyutil -K -n aesKey3 -t aes -s 128 -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": aesKey3 128 1024 aes <restricted> Here's the output showing all keys (aesKey and aesKey2 were created using sunPKCS11-nss from Java application): C:\nss\fips>symkeyutil -L -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": Name Len Strength Type Data NSS FIPS 140-2 Certificate DB: aesKey3 -2147483648 0 aes <restricted> aesKey2 268435456 -2147483648 aes <restricted> aesKey 268435456 -2147483648 aes <restricted> *** NSS 3.11.4 Output *** C:\nss\fips>symkeyutil -L -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": Name Len Strength Type Data NSS FIPS 140-2 Certificate DB: aesKey3 128 1024 aes <restricted> aesKey2 16 128 aes <restricted> aesKey 16 128 aes <restricted> Doing the following in Java to generate and store a key (Provider "p" is the Sun PKCS11 one I initialized with the config at the end of report): KeyGenerator keyGen = KeyGenerator.getInstance("AES", p); keyGen.init(128); SecretKey rawKey = keyGen.generateKey(); System.out.println("Generated symmetric key:" + rawKey.toString()); ks.setEntry("javaAES", new KeyStore.SecretKeyEntry(rawKey), new KeyStore.PasswordProtection(password)); ks.store(null, password); *** Java application using NSS 3.11.4 libraries output (note the size of key after pulling it out of keystore, reported as only 16 bits) *** Some output from Java test program showing it's session key at first, then after importing it's token key: Generated symmetric key:SunPKCS11-NSSfips AES secret key, 128 bits (id 1, session object, sensitive, extractable) Stored aesKey on token pulled sym key out of keystore? SunPKCS11-NSSfips AES secret key, 16 bits (id 3126949473, token object, sensitive, extractable) *** NSS 3.11.4 output *** C:\nss\fips>symkeyutil -L -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": Name Len Strength Type Data NSS FIPS 140-2 Certificate DB: javaAES 16 128 aes <restricted> *** Java application using NSS 3.12 output *** Deleting javaAES using symkeyutil, switching back to nss 3.12 libs/utils and re-running test program... Generated symmetric key:SunPKCS11-NSSfips AES secret key, 128 bits (id 1, session object, sensitive, extractable) Stored aesKey on token pulled sym key out of keystore? SunPKCS11-NSSfips AES secret key, 268435456 bits (id 3663820385, token object, sensitive, extractable) Exception in thread "main" java.security.InvalidKeyException: Illegal key size at javax.crypto.Cipher.a(DashoA13*..) at javax.crypto.Cipher.init(DashoA13*..) at javax.crypto.Cipher.init(DashoA13*..) at NssPkcs11.main(NssPkcs11.java:64) *** NSS 3.12 output *** and symkeyutil output: C:\nss\fips>symkeyutil -L -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": Name Len Strength Type Data NSS FIPS 140-2 Certificate DB: javaAES 268435456 -2147483648 aes <restricted> ***Java PKCS11 config info *** I have all NSS libs/utils in /usr/mozilla I have a db in /nss/fips that's in fips mode My pkcs11 cfg file for the sun PKCS11 provider contains the following: name = NSSfips nssLibraryDirectory = /usr/mozilla nssSecmodDirectory = /nss/fips nssModule = fips nssDbMode = readWrite Both the command line util and the library (passing info to Java) are getting the wrong info so I assume the problem is in the library somewhere. Reproducible: Always
Assignee | ||
Comment 1•16 years ago
|
||
Observations: 268435456 = 0x10000000 -2147483648 = 0x80000000 Maybe I haven't had enough coffee this morning, and this should be obvious to me, but I don't recall that 1024 bits is a valid AES key size. So I wonder if the fact that symkeyutil reportedly is willing to create a 1024 bit AES key is really the root cause of the problem.
Reporter | ||
Comment 2•16 years ago
|
||
It's doing it for the 128-bit (16 byte) keys as well: *** NSS 3.12 Output *** C:\nss\fips>symkeyutil -L -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": Name Len Strength Type Data NSS FIPS 140-2 Certificate DB: aesKey3 -2147483648 0 aes <restricted> aesKey2 268435456 -2147483648 aes <restricted> aesKey 268435456 -2147483648 aes <restricted> *** NSS 3.11.4 Output *** C:\nss\fips>symkeyutil -L -d . Enter Password or Pin for "NSS FIPS 140-2 Certificate DB": Name Len Strength Type Data NSS FIPS 140-2 Certificate DB: aesKey3 128 1024 aes <restricted> aesKey2 16 128 aes <restricted> aesKey 16 128 aes <restricted> And while using 3.11.4 libs, pulling the key from key DB java reports the size as 16 bits instead of 128 bits: pulled sym key out of keystore? SunPKCS11-NSSfips AES secret key, 16 bits (id 3126949473, token object, sensitive, extractable)
Assignee | ||
Comment 3•16 years ago
|
||
The problem occurs in this stack: nssdbm3.dll!lg_ULongAttribute() Line 151 nssdbm3.dll!lg_FindSecretKeyAttribute() Line 818 + 0x11 bytes nssdbm3.dll!lg_GetSingleAttribute() Line 1377 + 0x11 bytes nssdbm3.dll!lg_GetAttributeValue() Line 1400 + 0x13 bytes softokn3.dll!sftkdb_GetAttributeValue() Line 1294 + 0x18 bytes softokn3.dll!NSC_GetAttributeValue() Line 3834 + 0x15 bytes nss3.dll!PK11_ReadULongAttribute() Line 170 + 0x1b bytes nss3.dll!PK11_GetKeyLength() Line 715 + 0x18 bytes symkeyutil.exe!PrintKey() Line 320 + 0x9 bytes symkeyutil.exe!ListKeys() Line 356 + 0x9 bytes symkeyutil.exe!main() Line 1033 + 0x14 bytes The code is trying to get the CKA_VALUE_LEN attribute. The code in lg_FindSecretKeyAttribute looks like this: case CKA_VALUE_LEN: keyLen=key->u.rsa.privateExponent.len; return lg_ULongAttribute(attribute,type, keyLen); The value in keyLen is the correct/expected value (0x80), but function ld_UlongAttribute effectively converts it from little to big endian, so the value output is 0x80000000 instead of 0x00000080. This is a regression in 3.12, and a must-fix for the FIPS evaluation
Assignee: nobody → rrelyea
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Whiteboard: FIPS
Version: unspecified → 3.12
Assignee | ||
Comment 4•16 years ago
|
||
I will add that, IMO, it is a bug that the keygen succeeded with that obviously invalid AES key length. Function validateSecretKey in file pkcs11.c has several purposes, including: a) making sure the key length is valid and b) making sure the key length is recorded in the CKA_VALUE_LEN attribute. There is a switch that handles the different key types, but it has no case for AES keys. IMO, it should have a case for AES keys that detects invalid lengths and returns CKR_KEY_SIZE_RANGE.
Comment 5•15 years ago
|
||
Hmm, the value should get converted back out the other end. The database interface is supposed to store data in Big Endian IIRC. sftkdb_GetAttributeValue is supposed to convert it to the native value...
Assignee | ||
Comment 6•15 years ago
|
||
Bob, function lg_ULongAttribute takes an argument CK_ULONG value, and contains this code: 150 data = (unsigned char *)attr->pValue; 151 for (i=0; i < 4; i++) { 152 data[i] = (value >> ((3-i)*8)) & 0xff; 153 } Here are some observations about that code. It is unconditionally compiled, being used on both big endian and little endian machines alike. It is actually an implementation of htonl. It has the effect of converting the value of "value" to big-endian format, on both big endian and little endian machines. (On big endian machines, it's an expensive no-op. On little endian machines, it's an expensive htonl function.) Given that the content of the cert8.db file is supposed to always be in network byte order (big-endian), this function would be appropriate to use in preparing a CK_ULONG value to be stored into the DB, assuming that the value of the "value" argument is in host byte order (a native integer). Are you saying (in comment 5) that key->u.rsa.privateExponent.len is not supposed to be in host byte order, but rather in network byte order (big endian)?
Comment 7•15 years ago
|
||
The value in key->u.rsa.privateExponent.len is supposed to be host byte order. The value returned in by lg_GetAttributeValue is supposed to be network byte order. Are you saying the actual value in key->u.rsa.privateExponent.len is in the wrong byte order? bob
Assignee | ||
Comment 8•15 years ago
|
||
(In reply to comment #7) > The value in key->u.rsa.privateExponent.len is supposed to be host byte order. It is in host byte order. > The value returned in by lg_GetAttributeValue is supposed to be network byte > order. It is in network byte order. I am surprised to learn that this is expected. There clearly needs to be more documentation (comments) explaining all this in the code. > Are you saying the actual value in key->u.rsa.privateExponent.len is in the > wrong byte order? No. I'm merely saying that the value that ultimately gets out to the application is in network byte order.
Comment 9•15 years ago
|
||
> It is in network byte order. I am surprised to learn that this is expected. > There clearly needs to be more documentation (comments) explaining all this > in the code. Ironically this is one area that is well documented:): https://wiki.mozilla.org/NSS_Shared_DB#sdb_GetAttributeValue > No. I'm merely saying that the value that ultimately gets out to the > application is in network byte order. That means their is still a bug. It's probably sftkdb_GetAttributeValue. bob
Assignee | ||
Comment 10•15 years ago
|
||
Granted that the sdb_ functions are documented, but the functions being discussed in comment 6 and comment 8 are lg_ functions, and they are not documented, AFAIK.
Comment 11•15 years ago
|
||
The sbd_ functions define an interface (they are function pointers). The purpose of documenting them is the idea that they can be supplied from other sources. lg_ functions implement that interface for DBM. (see https://wiki.mozilla.org/NSS_Shared_DB#Layering and https://wiki.mozilla.org/NSS_Shared_DB#legacy_DB_support :)
Assignee | ||
Comment 12•15 years ago
|
||
The problem was in function sftkdb_isULONGAttribute. It has a switch which is supposed to contain cases for all the attributes that are CK_ULONGs. But its list was incomplete. It lacked CKA_VALUE_LEN. This patch fixes that. Bob, please review. But now I'm worried. What OTHER attributes are missing from this switch?
Attachment #356120 -
Flags: review?(rrelyea)
Updated•15 years ago
|
Attachment #356120 -
Flags: review?(rrelyea) → review+
Comment 13•15 years ago
|
||
Comment on attachment 356120 [details] [diff] [review] Patch v1 for NSS Trunk r+ I'll answer nelson's question in the next post, which may trigger a new patch (from either nelson or myself). bob
Comment 14•15 years ago
|
||
There are other attributes missing from this switch statement. To understand how they were missing, I'll go over how I constructed the switch statement to begin with.... The table was generated as follows: 1) I walked through the object section (section 10) of the PKCS #11 v2.20 spec, searching form attributes that mapped to CKU_LONGs in the PKCS #11 spec (including 'typed' U_LONGS like CK_CLASS and CK_KEY_TYPE). I did know include attributes that I knew we did not use. 2) I walked through the attributes in pkcs11n.h, identifying those where where CKU_LONGs. Unfortutately several of the attributes are specified in section 12. These are attributes that are considered 'type' specific (that is attributes specific to CKK_AES versus CKK_DES for instance. I don't remember if I had actually perused those attributes, but I have done so now... I found the following CKA_VALUE_LEN CKA_VALUE_BITS CKA_PRIME_BITS CKA_SUBPRIME_BITS CKA_MODULUS_BITS All of these attributes are really 'non-stored' attributes. You only find this out, however, by reading the text of the algorithm in question. They are used to supply data for such functions as KeyGen, or KeyPairGen. Unfortunately back in the very early days, we wanted to generally be able to determine the length of a secret key. A function was written in pk11wrap which would work very hard to determine that key length. On trick it would try is to read CKA_VALUE_LEN for the key. Softoken was modified to return this value. NSS does so misuse the other 3 _BITS functions, so I don't think we need to add them to the list. Now in going back and reviewing, I've identified the additional attributes that NSS does not currently used. They are: CKA_KEY_GEN_MECHANISM CKA_MECHANISM_TYPE and a whole list related to the hardware fetature object (CKO_HW_FEATURE): CKA_HW_FEATURE CKA_PIXEL_X CKA_PIXEL_Y CKA_RESOLUTION CKA_CHAR_ROWS CKA_CHAR_COLUMNS CKA_BITS_PER_PIXEL I think we can continue to ignore CKO_HW_FEATURE as irrelevant, but the other 2 attributes are standard PKCS #11 V2.20 attributes that we just aren't supporting yet. It would probably be a good idea to add those (CKA_JAVA_MDP_SECURITY_DOMAIN already fits this category) In Addition, there are the following attributes listed in Appendex A, list of constants, which look like they may be CK_ULONG, but I had not found them in the spec.... CKA_SECONDARY_AUTH CKA_AUTH_PIN_FLAGS CKA_MIME_TYPES The two 2 are deprecated attributes. bob
Comment 15•15 years ago
|
||
^two 2^top 2^ BTW it might be good to get a second pair of eyes on the list in Appendix A (page 376). bob
Assignee | ||
Comment 16•15 years ago
|
||
Bob, I think you're suggesting that the patch should be expanded to add cases for all the following attribute types: CKA_VALUE_LEN CKA_VALUE_BITS CKA_PRIME_BITS CKA_SUBPRIME_BITS CKA_MODULUS_BITS CKA_KEY_GEN_MECHANISM CKA_MECHANISM_TYPE Is that what you're suggesting? If so, I will submit another patch to do that.
Comment 17•15 years ago
|
||
I was just suggesting adding CKA_VALUE_LEN, CKA_KEY_GEN_MECHANISM, and CKA_MECHANISM_TYPE, but I wanted to get informed input on that proposal. (I'm OK with adding the others, but I don't think they are necessary). bob
Assignee | ||
Comment 18•15 years ago
|
||
This patch adds all the attributes I listed in a previous comment. It may includes ones that are not strictly needed now, but it doesn't hurt to have them (AFAIK) and listing all the possible candidates means we won't run into this problem in the future.
Attachment #356120 -
Attachment is obsolete: true
Attachment #357891 -
Flags: review?(rrelyea)
Comment 19•15 years ago
|
||
Comment on attachment 357891 [details] [diff] [review] patch v2, r=wtc.
Attachment #357891 -
Flags: review+
Comment 20•15 years ago
|
||
Comment on attachment 357891 [details] [diff] [review] patch v2, r+ Just a comment about the order. The original order was the order they appeared in the PKCS #11 spec. It's probably OK to them to be in Alphabetic order, though. bob
Attachment #357891 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 21•15 years ago
|
||
Checking in sftkdb.c; new revision: 1.17; previous revision: 1.16
Assignee: rrelyea → nelson
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.3
You need to log in
before you can comment on or make changes to this bug.
Description
•