301394 - Can't call NSC_Sign with a NULL pSignature

Status: RESOLVED INVALID

Description

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Created attachment 191411 [details] [diff] [review]
Preliminary patch for ECDSA_SignDigestWithSeed

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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

Nelson,

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
possible.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 5

[Wan-Teh Chang]

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.