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.
Also CID 648
Retargetting all P2s to 3.11.3 .
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?
klocwork ID 87917 87918
Created attachment 245698 [details] [diff] [review] 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.
Created attachment 249203 [details] [diff] [review] 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.
Created attachment 249206 [details] [diff] [review] 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."
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 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
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