552954 - NSC_UnwrapKey incorrectly unwraps symmetric keys starting with null bytes

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect)

Description

[Alex Dupre]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; it-IT; rv:1.8.1.20) Gecko/20090615 Firefox/2.0.0.20
Build Identifier: 3.12.4

While I was working on Bug 528467, I found other two bugs in the C_UnwrapKey function. One is this: if the key to unwrap is a SecretKey (like an AES key) starting with null bytes (the cleartext key, not the wrapped one), the C_UnwrapKey function create a key with wrong length, stripping all the leading null bytes.

Reproducible: Always

Steps to Reproduce:
1. Generate a wrapping key on NSS softoken
2. encrypt an AES key formed by 32 null bytes with the wrapping key
3. unwrap the wrapped key
Actual Results:  
A 1-byte AES key!

Expected Results:  
A 32-byte AES key.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I invite you to contribute a patch that continues to remove leading zeros 
for key types where that is desirable, and not for key types where it is not.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This bug concerns code inside the FIPS boundary.

Comment 3

[Alex Dupre]

I haven't looked at the code, but can you tell me a wrapping key format that
requires the removal of leading zeros? Raw secret keys don't, PKCS#8 private
keys don't, why the removal is needed at all?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

NSS Softoken's internal storage of bignums for RSA, DSA, DH, etc., wants 
them stored with all leading zero bytes stripped.  It is an internal 
requirement.  

In the early days of NSS, there was no persistent storage of symmetric keys.
In PKCS#11 speak, there were no "token keys" that were also symmetric keys.
When support for symmetric token keys was first added, a symmetric key was 
internally stored as if it was the modulus of an RSA key.  That's how the 
stripping of leading zeros got applied to those symmetric keys.  It's a bug.

The decision to package symmetric keys inside a PKCS#8 structure, as if they
were the modulus of an RSA key, was done to avoid creating a new DB record
type in the cert8 (or maybe it was cert7) DBs.  It meant that a key DB created by a new new browser that contained these new records would be usable by old browsers, because it did not introduce any new record types that would make 
the old browsers barf.  

But when we created the new SQLITE3-based cert9.db, there was no further need
to continue to package symmetric keys as phony RSA keys.  I hope we don't 
do that for cert9 DBs.  

In any case, we should strip leading zeros where we need to do so (for keys that contain bignums) and not for keys where we have no need to do so.
I repeat my invitation (see comment 1).

Comment 5

[Alex Dupre]

Tested with SQLITE3 backend, and it works. It's a BDB-only problem.

Comment 6

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last months and this bug has severity 'major'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Marco Castelluccio [:marco]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change

Comment 8

[Robert Relyea]

Thanks mark, I wondered why I thought I was inactive. This bug can probably be closed. It's a FIPS only dbm bug, and we 1) are moving away from dbm and 2) no longer FIPS validate dbm.