Last Comment Bug 95135 - DSA private key storage/retrieval not working for certain public key values
: DSA private key storage/retrieval not working for certain public key values
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.2.1
: All All
: P1 blocker (vote)
: 3.2.2
Assigned To: Robert Relyea
: Sonja Mirtitsch
Depends on:
  Show dependency treegraph
Reported: 2001-08-13 12:56 PDT by Ian McGreer
Modified: 2001-12-20 14:57 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

workaround patch (1.01 KB, patch)
2001-08-13 14:17 PDT, Ian McGreer
no flags Details | Diff | Splinter Review
script that reveals bug by generating DSA certs until failure (520 bytes, text/plain)
2001-11-12 09:15 PST, Ian McGreer
no flags Details

Description Ian McGreer 2001-08-13 12:56:12 PDT
I had thought that the recent QA problems were some flutter in the way we test
things, but this no longer appears to be the case.  Thus I'm filing a bug.  Here
is what I know so far.

(I'm substituing "accessor" for "database key" to avoid confusion)

When certutil generates the DSA key pair, it stores the private key in the
database using the public key as the accessor. At this time, the public key does
not ever appear to have a leading 0.

When pk12util attempts to retrieve the key, it occassionally has a leading 0. 
When it does, retrieval fails.  Adding code that says "if you don't find the key
and the public key has a leading 0, try with an accessor that is the public key
without the leading 0" causes retrieval to work.  However, later on, pk12util
fails to import the key.

I know that somewhere deep in my memory is the reason why we sometimes add
leading 0's, and it has something to do with when the first byte exceeds a
certain value.  But I can't for the life of me remember exactly what it is.

At any rate, the code needs to be consistent.  It must either store the key with
a leading zero, or not expect it to be there when it goes looking for it.

Note, of course, that the prescence/absence of a leading 0 does not affect the math.
Comment 1 Wan-Teh Chang 2001-08-13 13:10:33 PDT
Ian, does this mean your previous fix of reading the password
file once and leaving the password in memory is not the right

Bob, could you help Ian investigate this bug?
Comment 2 Robert Relyea 2001-08-13 13:40:27 PDT
Way back in the early days, we had a bignum library which we had aquired from a
vendor. That bignum library was a signed library (it would treat the various
values as signed values), therefore all our code would be care to make sure that
all our values were positive. PKCS #11 uses unsigned values. There is code in
the PKCS #11 wrappers and softoken to convert from signed to unsigned and back.
Now our big num library operates with unsigned values, but our der decoder and
our legacy databases still store the signed values.

So how do we turn an unsigned value into a signed value? signed: if the most
significant bit is '1' then we add a leading '0'. (That will explain your 1 in
10 or so failure rate).

Comment 3 Ian McGreer 2001-08-13 13:49:36 PDT
I still have some questions:

1.  I know our RSA code did the same thing.  Why are we only having database
problems with DSA?

2.  How do we fix it?  Do we remove the code that prepends the leading 0?  I
assume that would have wide-ranging consequences.  Do you think my workaround is
correct?  If so, I will start investigating why it failed later on.
Comment 4 Robert Relyea 2001-08-13 13:55:20 PDT
The RSA Modulus always has the high bit set. I don't think that is necessary
true of DSA public keys.

Comment 5 Ian McGreer 2001-08-13 14:00:06 PDT
I may have jumped the gun when I said my workaround failed.  It did; but that
may have been local environment issues when I ran the test.  I have since set
the QA off and running, and it has completed over 50 times successfully. 
However, I forgot to put a log message in to let me know when my workaround was
used to verify that it functions correctly.  I'm redoing it now.

If it does work, I'll attach the patch showing what I did.
Comment 6 Ian McGreer 2001-08-13 14:13:19 PDT
ok, it didn't work.  But that is because of *another* bug.  See 95150.

I now believe that this workaround is correct for this case.  Thus, patch coming up.
Comment 7 Ian McGreer 2001-08-13 14:17:46 PDT
Created attachment 45692 [details] [diff] [review]
workaround patch
Comment 8 Wan-Teh Chang 2001-08-14 11:50:55 PDT
Comment 9 Robert Relyea 2001-08-14 13:07:01 PDT
see comment in bug 95150 

Comment 10 Ian McGreer 2001-08-14 13:29:02 PDT
checked in.  waiting for tomorrow's QA before resolving.
Comment 11 Wan-Teh Chang 2001-10-31 19:15:53 PST
Julien, please verify the target milestone and
mark this bug fixed.
Comment 12 Wan-Teh Chang 2001-10-31 20:00:01 PST
Sorry, I meant to say
    Ian, please verify the target milestone and
    mark this bug fixed.
Comment 13 Ian McGreer 2001-11-01 07:47:45 PST
fixed in 3.2.2.
Comment 14 Wan-Teh Chang 2001-11-08 17:20:18 PST
The fix is not in 3.3 but is in 3.3.1.
Comment 15 Ian McGreer 2001-11-12 07:42:41 PST
Reopening bug because it exists again, on the tip (it is the reason for the
intermittent QA failures, about 1 in 10 times again).  Bob, there are now four
calls to nsslowkey_FindKeyByPublicKey.  Either that function needs this patch,
or every place that calls it does.

Actually, it is needed for at least three functions:
Can you take care of this?
Comment 16 Robert Relyea 2001-11-12 08:46:06 PST
Yes, I want to take a look at when and how the failures happen. Many of the new
calls have a key read from the database now, not calculated. I think I need to
look more closely at the get public value calls.

Comment 17 Ian McGreer 2001-11-12 09:15:05 PST
Created attachment 57481 [details]
script that reveals bug by generating DSA certs until failure

You can use this script to find the bug (it's what I used)
Comment 18 Robert Relyea 2001-11-12 10:05:53 PST
OK, I've completed the fix. The problem in this case wasn't the lookup code, but
the storage code. I'm now up to 200 reps and counting.

Comment 19 Robert Relyea 2001-11-12 10:07:39 PST
OK the fix is checked in...
Comment 20 Nelson Bolyard (seldom reads bugmail) 2001-12-20 14:57:57 PST
Adding myself to cc list.  Wish I'd seen this bug long ago.  
I'll send comments about this bug and bug 115360 in private email

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