Can't call NSC_Sign with a NULL pSignature



14 years ago
13 years ago


(Reporter: wtc, Assigned: wtc)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



14 years ago
I found this bug by code inspection. I haven't
confirmed it with a test program.

NSC_Sign, the C_Sign method of our softoken, will
crash if the pSignature argument is NULL.  I
determined this by tracing the code for RSA, DSA,
and ECDSA signing.  They all eventually pass
pSignature as the second argument to mp_to_fixlen_octets,
which will dereference it.

One can use a NULL pSignature to determine the
length of the signature.

Comment 1

14 years ago
Created attachment 191411 [details] [diff] [review]
Preliminary patch for ECDSA_SignDigestWithSeed
Comment on attachment 191411 [details] [diff] [review]
Preliminary patch for ECDSA_SignDigestWithSeed

This patch is correct, but I'd say it doesn't go far enough.

If there's no place to put the signature, we shouldn't waste 
the CPU time to compute it, only to then discard it.  
We should short-cut as much of the code as possible, and 
just return the length as quickly as possible.	I'm not sure it 
should even be necessary to go from softoken into freebl just 
to get the length, but I don't object to doing so.

Comment 3

14 years ago
Comment on attachment 191411 [details] [diff] [review]
Preliminary patch for ECDSA_SignDigestWithSeed


Thank you for reviewing this patch voluntarily.  I came to
the same conclusion.  I still attached this old patch because
I was removing low-priority changes from my source tree.
I agree the proper patch should shortcut as much code as
Attachment #191411 - Attachment description: Patch for ECDSA_SignDigestWithSeed → Preliminary patch for ECDSA_SignDigestWithSeed
This bug is very much like bug 320187.

Wan-Teh, does this bug apply only to ECDSA Signing?
I ask because that is all your patch appears to address.
Whiteboard: ECC

Comment 5

13 years ago
This bug is invalid.  NSC_Sign (and NSC_SignFinal) doesn't
crash if pSignature is NULL.  It handles a NULL pSignature
like this:

    if (!pSignature) {
        *pulSignatureLen = context->maxLen;
        goto finish;

The only issue with the above code is that context->maxLen
may be a generous estimate of the signature's length.  This
is the case for ECDSA signatures, for which context->maxLen
is set to MAX_ECKEY_LEN * 2, regardless of the curve used.
For RSA and DSA signatures, context->maxLen is the exact
signature length (nsslowkey_PrivateModulusLen(privKey) and
DSA_SIGNATURE_LEN, respectively).

I have a better version of my
"Preliminary patch for ECDSA_SignDigestWithSeed" in bug 320589.
Last Resolved: 13 years ago
Resolution: --- → INVALID
Whiteboard: ECC
You need to log in before you can comment on or make changes to this bug.