pk12util takes friendly name from key, not cert



13 years ago
12 years ago


(Reporter: nelson, Assigned: nelson)


Firefox Tracking Flags

(Not tracked)



(2 attachments, 2 obsolete attachments)

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.


13 years ago
Priority: -- → P2
Target Milestone: --- → 3.11.2

Comment 1

13 years ago
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 
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
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
Attachment #238756 - Attachment description: working patch, NOT ready for checkin → working patch, v1
Attachment #238756 - Flags: review?(neil.williams)

Comment 4

12 years ago
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+


12 years ago
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
Attachment #250680 - Attachment is obsolete: true
Attachment #250686 - Flags: review?(neil.williams)
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)


12 years ago
Target Milestone: 3.11.3 → 3.11.5


12 years ago
Attachment #250686 - Flags: review?(neil.williams) → review+
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 10

12 years ago
Comment on attachment 250686 [details] [diff] [review]
updated patch for trunk, v3


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+


12 years ago
Target Milestone: 3.11.5 → 3.11.6


12 years ago
Target Milestone: 3.11.6 → 3.11.7
This was fixed on the trunk in January.  
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.