Note: There are a few cases of duplicates in user autocompletion which are being worked on.

sec_pkcs12_install_bags passes uninitialized variables to functions

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Neil Williams)

Tracking

({coverity, klocwork})

3.11.1
3.12
coverity, klocwork

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CID 648 649])

Attachments

(2 attachments, 1 obsolete attachment)

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.
(Reporter)

Updated

11 years ago
Priority: -- → P2
Target Milestone: --- → 3.11.2
(Reporter)

Comment 1

11 years ago
Also CID 648
Whiteboard: [CID 648 649]

Comment 2

11 years ago
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
(Assignee)

Comment 3

11 years ago
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?
Status: NEW → ASSIGNED
(Reporter)

Comment 4

11 years ago
klocwork ID 87917 87918
Keywords: klocwork
(Reporter)

Updated

11 years ago
Blocks: 353745
(Reporter)

Updated

11 years ago
Target Milestone: 3.11.3 → 3.11.4
(Assignee)

Comment 5

11 years ago
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.
(Assignee)

Comment 6

11 years ago
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.
Attachment #245698 - Attachment is obsolete: true
(Assignee)

Comment 7

11 years ago
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."
Attachment #249206 - Flags: review?
(Reporter)

Comment 8

11 years ago
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.
Target Milestone: 3.11.4 → 3.12
(Reporter)

Comment 9

11 years ago
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
Attachment #249206 - Flags: review? → review+
(Assignee)

Comment 10

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.