339906 - sec_pkcs12_install_bags passes uninitialized variables to functions

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Coverity CID 649
In function sec_pkcs12_install_bags, two variables are not initialized.
They are keyType and keyUsage.
If variable certList gets assigned NULL (the value returned by 
sec_pkcs12_find_certs_for_key), then the uninitialized values of 
keyType and keyUsage are passed to sec_pkcs12_add_key. That's BAD.

I don't think the answer is as simple as initializing those two 
variables.  Perhaps, if certList is NULL, we should skip the call
to sec_pkcs12_add_key alltogether.

The thing is, this condition could be created by an unusual pkcs12
file.  Unusual input is all it takes for us to pass uninnitialized
values to this function.  Let's fix this for 3.11.2.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Also CID 648

Comment 2

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 3

[Neil Williams]

In this function a list of certs for each key is made, then the key is imported followed by it's associated certs. The key type and usage for the key are determined by the first cert in the list. If there is no cert for the key what do you do about the type and usage? Or do you just refuse to import the key?

One approach is to do the same thing when no DER cert exists as when the cert exists but can't be decoded. In that case the key is assumed to be RSA and usage is everything (CKA_UNWRAP, CKA_DECRYPT, CKA_SIGN, CKA_SIGN_RECOVER).

Comments?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

klocwork ID 87917 87918

Comment 5

[Neil Williams]

Attachment: add experimental option to build PFX file with key only

This won't be checked in. Its purpose is to build a test file for the real patch.

Comment 6

[Neil Williams]

Attachment: better patch to create key-only PFX files

Previous had a reference from pkcs12/p12d.c to pk12util.c. If you tried building the whole tree after patching it failed.

This won't be checked in either.

Comment 7

[Neil Williams]

Attachment: prevents use of the unitialized variables

It turns out that pk12util protects itself from this particular by requiring that all keys have a cert associated with them. Thus even when attempting to import a PFX file created with the above patch pk12util will fail with "PKCS12 decode validate bags failed: Not imported, already in database."

This patch also fixes a bogus error message when the cert named for export was not found; used to say something generic, like "out of memory."

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Neil, if the diagnostic message for a missing cert reports that the key is 
already in the database, rather than something like "can't import key without
cert", that's a bug that needs to be fixed.  It will surely waste a lot of
NSS developer time dealing with this misdiagnosis.  Please file a separate
bug on that (if there isn't already one) and develop a patch to fix it.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249206 [details] [diff] [review]
prevents use of the unitialized variables

Looks like this will prevent the use of the uninitialized variable.
r=nelson

Comment 10

[Neil Williams]

Checking in cmd/pk12util/pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.34; previous revision: 1.33
done
Checking in lib/pkcs12/p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.34; previous revision: 1.33
done