Last Comment Bug 472749 - Softoken permits AES keys of ANY LENGTH to be created
: Softoken permits AES keys of ANY LENGTH to be created
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P2 normal (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
Depends on:
  Show dependency treegraph
Reported: 2009-01-08 14:25 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-01-21 17:19 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 for NSS Trunk (999 bytes, patch)
2009-01-08 18:29 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
Patch v2 with Wan-Teh's desired change (1.11 KB, patch)
2009-01-20 17:12 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
wtc: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2009-01-08 14:25:00 PST
+++ 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.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-01-08 14:26:33 PST
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.
Comment 2 Robert Relyea 2009-01-08 16:09:15 PST
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.
Comment 3 Robert Relyea 2009-01-08 16:10:17 PST
It's almost certainly not a requirement to pass FIPS.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-01-08 18:29:11 PST
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.
Comment 5 Robert Relyea 2009-01-09 12:20:32 PST
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).

Comment 6 Wan-Teh Chang 2009-01-09 14:28:16 PST
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).
Comment 7 Nelson Bolyard (seldom reads bugmail) 2009-01-09 14:58:52 PST
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.
Comment 8 Robert Relyea 2009-01-12 11:36:05 PST
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).

Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-01-12 12:52:47 PST
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.  

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.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2009-01-20 17:12:33 PST
Created attachment 357893 [details] [diff] [review]
Patch v2 with Wan-Teh's desired change

Bob, please review
Comment 11 Wan-Teh Chang 2009-01-20 17:34:09 PST
Comment on attachment 357893 [details] [diff] [review]
Patch v2 with Wan-Teh's desired change

Comment 12 Robert Relyea 2009-01-21 11:09:24 PST
Comment on attachment 357893 [details] [diff] [review]
Patch v2 with Wan-Teh's desired change

r+ This will do for now.

Comment 13 Nelson Bolyard (seldom reads bugmail) 2009-01-21 17:19:10 PST
Checking in pkcs11.c; new revision: 1.158; previous revision: 1.157

Note You need to log in before you can comment on or make changes to this bug.