Apparently, in PKCS12 files, private key can have friendly names and certs can have friendly names, and the private key's frienly name can be different from the cert's friendly name. It is also possible for the private key to have a friendly name, but the corresponding cert to not have a friendly name. MS Windows typically gives each key an ASCII-enocded UUID/GUID for its friendly name (which isn't very friendly, as names go, imo), and gives each cert no name, by default. Users may give their certs friendly names if they wish. In such cases, where the key's friendly name doesn't match the cert's friendly name, when pk12util imports the key and cert, the cert is given the key's friendly name, not the cert's friendly name. I believe this is always a mistake. The cert should alawys be given the cert's friendly name, and if the cert has no friendly name, then it should be treated as any other case of missing friendly name.
Priority: -- → P2
Target Milestone: --- → 3.11.2
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
Created attachment 238756 [details] [diff] [review] working patch for trunk, v1 This patch fixes a multitude of sins, and probably commits a few new ones. It documents some previously undocumented dependency on the fact that all bags share a single common arena, the decoder's arena. It fixes a classic problem of losing the address of the previously allocated buffer when realloc returns NULL. It fixes an infinite loop that I've experienced before when CERT_NewTempCertificate cannot import a cert for some (any) reason. It tries to fix many problems in sec_pkcs12_install_bags(). - It eliminates gotos - It eliminates leaks - It uses the nickname of a cert (when one exists), rather than the key's nickname, as the CKA_LABEL of the key when installing the key. (that's the subject of this bug, IIRC) The remaining problems are in the areas of a) deciding when a cert should be disqualified from being installed, and b) when it should stop trying to install any more keys or certs. I think the logic for those decisions needs to be reworked in a major way. Consider a p12 file with multiple private keys and multiple certs, where no certs are relevant to both private keys. IOW, two independent sets of certs and keys in a common p12 file. If we cannot import one of those sets, should we stop and not try to import the other independent set? I think we should continue on, to try to install the independent sets, but that is not what the code does today. Consider a p12 file with multiple private keys and multiple certs, some of which are common to multiple keys. If one key cannot be installed, but it shares certs in common with another key, should we prevent the installation of the other key, and the certs they share in common, because the first key failed? Again, I think not, but the code does stop that today. I think it is reasonable to not install certs that were associated with a key that could not be installed, when those certs are not associated with any other key. And I think it is reasonable to install certs that are not associated with ANY key in the p12 file. But I think a cert should be installed if it is associated with any key that is imported, and if the cert itself is capable of being imported. The code attempts to predict whether the cert can be imported by decoding it. But that is no longer a sufficient predictor. We should try harder to fix the prediction code. Treating each key and its related certs as a set, IMO we should try to import all or NONE of each such set. A failure to import one component should not leave the set partially imported. Can we import a key for which we have no cert? Should we attempt to do so? The code tries to do it now, but I think it will always fail, due to the lack of a "public value" which (IIRC) we use as the basis for the CKA_ID attribute. There's another bug that suggests that our softoken must permit private keys to be imported without requiring the caller to know the corresponding public key value. Maybe we should finally implement that in softoken, and stop giving the private key different behavior depending on its presence. Acts of commission: - changed a bunch of while loops to for loops. This found and eliminated one infinite loop, but I carried on and did this to many while loops besides the one that needed it. - i did a lot of common subexpression elimination, removing lots of repeated array subscript evaluations, replacing them with a pointer whose address is evaluated once and reused many times. This makes the code smaller and easier to read (shorter expressions), but probably will draw criticism as being unnecssary change.
Assignee: neil.williams → nelson
Status: NEW → ASSIGNED
Comment on attachment 238756 [details] [diff] [review] working patch for trunk, v1 I originally said this patch was "NOT ready for checkin", but now I don't remember why. Neil, please review
Comment on attachment 238756 [details] [diff] [review] working patch for trunk, v1 There is one problem in a comment. In sec_pkcs12_validate_bags you added the comment "/* Now take a second pass over the safebags and install any certs..." I believe it should read "and mark any certs for installation." You might have been thinking of bug 339906 when you wrote that this patch wasn't ready for commitment. That bug needs more investigation (which I am in the middle of) before we can be sure we're fixing it correctly.
Attachment #238756 - Flags: review?(neil.williams) → review+
Attachment #238756 - Attachment description: working patch, v1 → working patch for trunk, v1
Created attachment 250680 [details] [diff] [review] updated patch for trunk, v2 This file has changed on the trunk since I wrote the previous patch, so the previous patch no longer applies cleanly. This new patch does, but now I need to retest it. We need a test case, a .p12 file that reproduces this.
Attachment #238756 - Attachment is obsolete: true
Created attachment 250681 [details] .p12 test file, no password This test file is useful for testing multiple different pk12util bugs. It has no password. It has a UUID for the friendly name of the key, and common English names as the friendly names for the certs.
Created attachment 250686 [details] [diff] [review] updated patch for trunk, v3 This patch also fixes a problem that caused pk12util to report SEC_ERROR_INVALID_ARGS for an incorrect password. Neil, please review (again). Compared to v1 of this patch two functions have changed: SEC_PKCS12DecoderVerify and sec_pkcs12_install_bags
Comment on attachment 250686 [details] [diff] [review] updated patch for trunk, v3 Need two reviews for 3.11 branch
Attachment #250686 - Flags: review?(alexei.volkov.bugs)
Target Milestone: 3.11.3 → 3.11.5
Fix committed on trunk. Awaiting second review for branch. > Bug 335019. When importing certs from PKCS12 files, and the cert and the > private key both have different nicknames, import the cert with the > nickname from the file's cert, not from the file's private key. > Also, fix an infinite loop and certain other bugs. r=neil.williams.
Comment on attachment 250686 [details] [diff] [review] updated patch for trunk, v3 r=alexei.volkov Parch looks ok. Thx for comments: they are provide big help during review. I reviewed code on the subject of bags arenas. I agree with your comments to sec_pkcs12_add_item_to_bag_list function, but I've concluded that all bags are allocated on only one arena of p12 decoder context, which was expected.
Attachment #250686 - Flags: review?(alexei.volkov.bugs) → review+
Target Milestone: 3.11.5 → 3.11.6
Target Milestone: 3.11.6 → 3.11.7
This was fixed on the trunk in January.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.7 → 3.12
You need to log in before you can comment on or make changes to this bug.