+++ This bug was initially created as a clone of Bug #471665 +++ In NSS 3.12, it is possible to create AES keys with ANY LENGTH in softoken. For example, the command symkeyutil -K -n aesKey3 -t aes -s 128 -d . will create an AES key of length 128 BYTES (1024 bits). 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. It has 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.
I think this bug MUST be fixed in order to pass the FIPS validation, so I'm making this bug be P1. If fixing it is not a requirement to pass FIPS, then it is perhaps only a P2.
I'll keep it a must fix for FIPS, but it's clearly not a P1, probably not even a P2. I also think long term all key size information should live in a single place, or as few places as possible. Right now that information lives in NSS proper, softoken, and freebl. It should be general enough that we don't have to look though all these code paths when we create new ciphers (or extend the size of extisting ones.
It's almost certainly not a requirement to pass FIPS.
Created attachment 356116 [details] [diff] [review] Patch v1 for NSS Trunk This patch is what I had in mind. It seems to work. Bob, please review.
I'm not really excited about this patch. I really don't want to expand the amount of code that knows the length of an AES key is 'blah'... I would rather start colecting that knowledge in one place. I don't think it's wrong to have a check here for the length, but the explicit knowledge of the length shouldn't be here (ideally it would be good to have the check outside the switch statement for *ALL* algorithms so we don't have to keep makeing this kind of change). bob
Comment on attachment 356116 [details] [diff] [review] Patch v1 for NSS Trunk You also need to allow 192-bit AES keys (attribute->attrib.ulValueLen != 24).
Bob, if you look at the block of cases immediately preceding the new case added by my patch, case CKK_DES: case CKK_DES2: case CKK_DES3: case CKK_CDMF: You see that those cases exist purely to enforce the correct key length. They all call a function sftk_MapKeySize which returns the one and only correct key size for each of those key types. My patch is trying to mimic the behavior for those cases for AES. I would have simply added CKK_AES to that list of cases, but the AES key type seems to be unique among PKCS#11 key types, in that it allows several discrete sizes, but is not a truly variable key size (unlike RSA & RC2), so it cannot be handled by code that knows a single key size for the type, and it cannot be handled by code that merely enforces a minimum and maximum size. So, I think that where ever we put code to enforce symmetric key sizes, it will require a special case to implement AES. And I see no other place that is appropriate (or attempts) to enforce symmetric key sizes for new key objects except this code in validateSecretKey. Consequwently, I think this is the right place.
The other values still have ranges, which are not enforced in this function. Also, for some tokens (not ours), RSA, too, can be discrete (actually I think CAST can be too). It's really DES that seems to be unique. Anyway make wtc's change and I'll r+ it, but what I would really like is a call back into freebl to determine what the supported key sizes are (it's really the freebl implementation that determines this). bob
It's true that RSA sizes are not enforced in this function, because this function is for symmetric keys only. However, RSA sizes are constrained in detailm as are DSA and DH keys, in function sftk_handlePublicKeyObject. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.157&mark=854,859,266,871,876,880,889,894,898#840 sftk_handleSecretKeyObject and sftk_handlePublicKeyObject are, of course, peer functions, both in softoken, both called at the same depth from within sftk_HandleObject. Just as sftk_handlePublicKeyObject has knowledge of the acceptable key lengths for public keys, I think it is reasonable for sftk_handleSecretKeyObject to have knowledge of acceptable secret key lengths. I'll write an updated patch.
Created attachment 357893 [details] [diff] [review] Patch v2 with Wan-Teh's desired change Bob, please review
Comment on attachment 357893 [details] [diff] [review] Patch v2 with Wan-Teh's desired change r=wtc.
Comment on attachment 357893 [details] [diff] [review] Patch v2 with Wan-Teh's desired change r+ This will do for now. bob
Checking in pkcs11.c; new revision: 1.158; previous revision: 1.157