Closed Bug 471665 Opened 16 years ago Closed 15 years ago

NSS reports incorrect sizes for (AES) symmetric keys

Categories

(NSS :: Libraries, defect, P1)

3.12
x86
Windows Vista
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: david.konrad.stutzman, Assigned: nelson)

Details

(Whiteboard: FIPS)

Attachments

(1 file, 1 obsolete file)

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
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.
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)
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
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.
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...
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)?
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
(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.
> 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
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.
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 :)
Attached patch Patch v1 for NSS Trunk (obsolete) — Splinter Review
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)
Attachment #356120 - Flags: review?(rrelyea) → review+
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
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
^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
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.
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
Attached patch patch v2, Splinter Review
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 on attachment 357891 [details] [diff] [review]
patch v2, 

r=wtc.
Attachment #357891 - Flags: review+
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+
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.

Attachment

General

Created:
Updated:
Size: