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)

All
Windows XP
defect

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)

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.
Priority: -- → P1
Attached patch patch v1 - untested (obsolete) — Splinter Review
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)
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
Attached patch patch v2 - tested (obsolete) — Splinter Review
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)
Whiteboard: SUN_MUST_HAVE
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)
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
(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.
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
Attachment #358345 - Flags: review?(rrelyea) → review+
Comment on attachment 358345 [details] [diff] [review]
patch v3 - tested

r+ with suggested change.
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?
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
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
> 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.