472749 - Softoken permits AES keys of ANY LENGTH to be created

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

+++ 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)]

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]

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]

It's almost certainly not a requirement to pass FIPS.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 for NSS Trunk

This patch is what I had in mind.  It seems to work. Bob, please review.

Comment 5

[Robert Relyea]

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 6

[Wan-Teh Chang]

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)]

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]

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

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v2 with Wan-Teh's desired change

Bob, please review

Comment 11

[Wan-Teh Chang]

Comment on attachment 357893 [details] [diff] [review]
Patch v2 with Wan-Teh's desired change

r=wtc.

Comment 12

[Robert Relyea]

Comment on attachment 357893 [details] [diff] [review]
Patch v2 with Wan-Teh's desired change

r+ This will do for now.

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Checking in pkcs11.c; new revision: 1.158; previous revision: 1.157