Last Comment Bug 339906 - sec_pkcs12_install_bags passes uninitialized variables to functions
: sec_pkcs12_install_bags passes uninitialized variables to functions
Status: RESOLVED FIXED
[CID 648 649]
: coverity, klocwork
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.1
: All All
: P2 normal (vote)
: 3.12
Assigned To: Neil Williams
:
Mentors:
Depends on:
Blocks: 353745
  Show dependency treegraph
 
Reported: 2006-05-31 21:01 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-01-03 15:02 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add experimental option to build PFX file with key only (3.49 KB, patch)
2006-11-15 15:11 PST, Neil Williams
no flags Details | Diff | Splinter Review
better patch to create key-only PFX files (6.24 KB, patch)
2006-12-19 19:24 PST, Neil Williams
no flags Details | Diff | Splinter Review
prevents use of the unitialized variables (1.79 KB, patch)
2006-12-19 19:39 PST, Neil Williams
nelson: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2006-05-31 21:01:25 PDT
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) 2006-06-10 21:48:32 PDT
Also CID 648
Comment 2 Julien Pierre 2006-06-20 16:30:58 PDT
Retargetting all P2s to 3.11.3 .
Comment 3 Neil Williams 2006-06-28 19:04:34 PDT
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) 2006-09-21 22:44:35 PDT
klocwork ID 87917 87918
Comment 5 Neil Williams 2006-11-15 15:11:56 PST
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.
Comment 6 Neil Williams 2006-12-19 19:24:20 PST
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.
Comment 7 Neil Williams 2006-12-19 19:39:44 PST
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."
Comment 8 Nelson Bolyard (seldom reads bugmail) 2006-12-20 05:35:47 PST
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) 2006-12-20 05:36:36 PST
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 2007-01-03 15:02:46 PST
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

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