Closed
Bug 474532
Opened 15 years ago
Closed 15 years ago
Softoken cannot import certs with empty subjects and non-empty nicknames
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: nelson, Assigned: nelson)
Details
(Keywords: regression, Whiteboard: SUN_MUST_HAVE)
Attachments
(1 file, 2 obsolete files)
5.20 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
In trunk builds, PK11_ImportCert cannot import a cert with an empty subject name and a non-empty private key into the softoken. NSC_CreateObject returns CKR_TEMPLATE_INCOMPLETE. This is an unexpected change in behavior, and hence is viewed as a regression. The problem appears to be caused by the new function sftkdb_checkConflicts which was committed to fix bug 439115. That new function seems to assume that a zero-length subject is an error. I am working on a patch.
Assignee | ||
Updated•15 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•15 years ago
|
||
I think this will fix the problem. Bob, please review. In the mean time, I will try to get the folks who ran into this to test this patch.
Attachment #357902 -
Flags: review?(rrelyea)
Comment 2•15 years ago
|
||
The patch looks good as it is, but it does bring up an interesting point. Certs with NULL subjects are a special case. They are supposed to be referenced by their Subject Alt Name. This brings up an interesting questions.... should be set the CKA_SUBJECT to the Alt Name in the pk11wrapers? As it is, all certs with a NULL subject will have the same nickname (which is one of the invariants this code is trying to enforce). bob
Assignee | ||
Comment 3•15 years ago
|
||
I have a test program that reproduces the problem, and have tested it with this patch to my satisfaction. Bob, please review.
Assignee: rrelyea → nelson
Attachment #357902 -
Attachment is obsolete: true
Attachment #358341 -
Flags: review?(rrelyea)
Attachment #357902 -
Flags: review?(rrelyea)
Assignee | ||
Updated•15 years ago
|
Whiteboard: SUN_MUST_HAVE
Assignee | ||
Comment 4•15 years ago
|
||
One more fix. Note that I increment the attribute length before allocating a buffer to receive the attribute, so that a) I won't be attempting to allocate zero bytes, and b) I won't be asking sdb_GetAttributeValue to return zero bytes, even if that's what it will ultimately do.
Attachment #358341 -
Attachment is obsolete: true
Attachment #358345 -
Flags: review?(rrelyea)
Attachment #358341 -
Flags: review?(rrelyea)
Comment 5•15 years ago
|
||
But it looks like you are incrementing the template value as well, which may have a global effect! Are you sure you want to do that? bob
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #5) > But it looks like you are incrementing the template value as well, which may > have a global effect! Are you sure you want to do that? Yes. The function will replace the ulValueLen member of the template with the actual count returned, if the actual count is not greater than the value passed in. I just want to be sure that the value passed in is not zero.
Comment 7•15 years ago
|
||
Change:
> if ((findTemplate[0].ulValueLen != attr2->ulValueLen) ||
> - (PORT_Memcmp(findTemplate[0].pValue,attr2->pValue,attr2->ulValueLen) != > 0)) {
> + (attr2->ulValueLen > 0 &&
> + PORT_Memcmp(findTemplate[0].pValue, attr2->pValue, attr2->ulValueLen)
> + != 0)) {
To:
if ((attr2->ulValueLen <= 0) ||
(findTemplate[0].ulValueLen != attr2->ulValueLen) ||
(PORT_Memcmp(findTemplate[0].pValue,attr2->pValue,attr2->ulValueLen) != > 0)) {
If it's true that that attr2->ulValueLen really could be less then zero at this point (drop the change it can't be). for an r+
bob
Updated•15 years ago
|
Attachment #358345 -
Flags: review?(rrelyea) → review+
Comment 8•15 years ago
|
||
Comment on attachment 358345 [details] [diff] [review] patch v3 - tested r+ with suggested change.
Assignee | ||
Comment 9•15 years ago
|
||
Bob, I'm not sure what you're suggesting in comment 7. attr2->ulValueLen cannot be less than zero at this point because that condition was checked for much earlier. But it can be zero. So, is any change needed for my patch?
Comment 10•15 years ago
|
||
Ah, then we probably should make your existing check != 0. The > 0 leads to confusing assumptions (I'm also surprised that Memcpy doesn't handle the == 0 case, or is this just paranoia). bob
Assignee | ||
Comment 11•15 years ago
|
||
Checking in sftkdb.c; new revision: 1.18; previous revision: 1.17 To answer comment 10, I couldn't find a definition of memcpy that stated what it returns when count == 0, so I coded the test to avoid that case. In this particular spot in the code, tests of != 0 and > 0 are equivalent, but > 0 better conveys the intent, IMO. Thanks for your review.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 12•15 years ago
|
||
> In this particular spot in the code, tests of != 0 and > 0 are equivalent,
> but > 0 better conveys the intent, IMO. Thanks for your review.
I agree with the first part (that they are equivalent). However, I know at least one person was confused by the intent of > 0 because it implied that ulValueLen could be negative here, in which case the first part of the test is invalid (You don't want to match on 2 errors), thus my suggestion to change to !=0 to make it clear a negative result isn't possible.
It's just a clarity issue, not a code correctness issue, however.
bob
You need to log in
before you can comment on or make changes to this bug.
Description
•