Last Comment Bug 474532 - Softoken cannot import certs with empty subjects and non-empty nicknames
: Softoken cannot import certs with empty subjects and non-empty nicknames
Status: RESOLVED FIXED
SUN_MUST_HAVE
: regression
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All Windows XP
: P1 critical (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-20 19:06 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-01-27 13:48 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 - untested (5.19 KB, patch)
2009-01-20 19:26 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
patch v2 - tested (5.20 KB, patch)
2009-01-22 22:33 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
patch v3 - tested (5.20 KB, patch)
2009-01-22 23:09 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2009-01-20 19:06:19 PST
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.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-01-20 19:26:25 PST
Created attachment 357902 [details] [diff] [review]
patch v1 - untested

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.
Comment 2 Robert Relyea 2009-01-21 12:41:11 PST
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
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-01-22 22:33:50 PST
Created attachment 358341 [details] [diff] [review]
patch v2 - tested

I have a test program that reproduces the problem, and have tested it 
with this patch to my satisfaction.  Bob, please review.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-01-22 23:09:56 PST
Created attachment 358345 [details] [diff] [review]
patch v3 - tested

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.
Comment 5 Robert Relyea 2009-01-23 15:00:26 PST
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
Comment 6 Nelson Bolyard (seldom reads bugmail) 2009-01-23 16:02:52 PST
(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 Robert Relyea 2009-01-23 16:10:58 PST
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
Comment 8 Robert Relyea 2009-01-23 16:12:48 PST
Comment on attachment 358345 [details] [diff] [review]
patch v3 - tested

r+ with suggested change.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-01-23 16:24:49 PST
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 Robert Relyea 2009-01-26 11:08:12 PST
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
Comment 11 Nelson Bolyard (seldom reads bugmail) 2009-01-26 18:21:26 PST
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.
Comment 12 Robert Relyea 2009-01-27 13:48:42 PST
> 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

Note You need to log in before you can comment on or make changes to this bug.