Multipart CKM_DSA_SHA1 signing broken if given large buffer



15 years ago
12 years ago


(Reporter: Andreas Sterbenz, Assigned: Robert Relyea)


Firefox Tracking Flags

(Not tracked)



(1 attachment)

949 bytes, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review


15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212
Build Identifier: NSS 3.7.1

If NSC_SignFinal() is called with *pulSignatureLen larger than 40, signing fails
with CKR_DEVICE_ERROR. A check for "!=" should be changed to "<".

Reproducible: Always

Steps to Reproduce:
I used softoken via PKCS#11. I assume this is also reproducible using the NSS APIs.

1. via the PKCS#11 functionlist call C_SignInit()
2. C_SignUpdate()
3. C_SignFinal(handle, buffer, 41)

Diffs for NSS 3.7.1. Should apply as is to current CVS.

Thu Sep  5 20:44:10 2002
--- dsa.c	Fri Jan 31 15:05:56 2003
*** 184,190 ****
      /* FIPS-compliance dictates that digest is a SHA1 hash. */
      /* Check args. */
      if (!key || !signature || !digest ||
!         (signature->len != DSA_SIGNATURE_LEN) ||
  	(digest->len != SHA1_LENGTH)) {
  	return SECFailure;
--- 184,190 ----
      /* FIPS-compliance dictates that digest is a SHA1 hash. */
      /* Check args. */
      if (!key || !signature || !digest ||
!         (signature->len < DSA_SIGNATURE_LEN) ||
  	(digest->len != SHA1_LENGTH)) {
  	return SECFailure;

Comment 1

15 years ago
Bob, could you look at this?  Thanks.
Assignee: wtc → relyea
Ever confirmed: true
Andreas, you're suggesting that it's OK for the digest to be longer than 
a SHA1 digest.  That violates the FIPS spec.  In what situation is it 
desirable to do that?

I'm marking this P2, but I'm not sure it's valid.
Priority: -- → P2

Comment 3

14 years ago
Nelson, I am writing from the perspective of a generic PKCS#11 application. It
may not know that DSA always produces a 40 byte signature. Assume it calls
C_SignFinal() for all algorithms with a generously sized buffer (e.g. 2k) and
looks at the actual length returned by C_SignFinal(). The PKCS#11 spec seems to
imply that larger than necessary buffers are ok.

I do not know about the FIPS requirements. Still, note that I am not suggesting
to change the output that DSA produces, which would continue to be always 40
bytes long. The only change would be that DSA also produces output if the buffer
is larger than 40 bytes.

Comment 4

14 years ago
The fips checks should be on the outputted buffer, not on the input buffer size.
The application (including NSS) should be able to supply any buffer larger than
the requested amount. The code will only return the required amount. The check
is correct for Verify, because both input parameters are user supplied
signatures and hash functions, but for Sign, the check should be that the buffer
is large enough, then set the buffer length to the size of a DSA signature.
(NOTE the attached patch only does the first step, without setting
signature->len to the correct value the given test code would produce the wrong
out put (it would produce a 41 byte signature in with the 41st byte is random

I somehow misread Andreas's patch as suggesting a change to the test on 
digest->len rather than on signature->len.  

As Bob pointed out in the comment above, the proposed change doesn't 
complete the job.  The signature length would be wrong after the operation
finishes.  And the error returned for a too small buffer is still wrong.

IMO, softoken, not freebl, needs to check the length of the buffer, and 
return CKR_BUFFER_TOO_SMALL if it's too small, and set the output length
to the proper value, and invoke the freebl DSA function to do the actual
signing.  If softoken is validating its inputs, there will be no need to
change freebl's DSA code.  Freebl functions don't set CKR error codes.  
The sanity tests done by freebl functions on their inputs should not be 
used as a substitute for the tests required by PKCS 11, IMO.
2+ year old bug on the correctness of softoken.  Bob, please fix for 3.10
Target Milestone: --- → 3.10

Comment 7

13 years ago
Created attachment 178389 [details] [diff] [review]
Freebl should allow larger buffers.

Untested batch.

Comment 8

13 years ago
Here's a patch for the problem.

In looking into it I also noticed that softoken is doing some unnecessary copies
for DSA and ECDSA signatures in it's stub file. I'll write a separte bug about this.

Comment on attachment 178389 [details] [diff] [review]
Freebl should allow larger buffers.

Looks good to me.  Bob, please check it in.
Attachment #178389 - Flags: review+
Retargetting to 3.10.1.  
Bob, please check in your reviewed patch for this bug onto the trunk. 
Or if you'd like, I can do that.  Please check it in, or reassign the 
bug to me if you want me to do it. Thanks.
Target Milestone: 3.10 → 3.10.1
Version: unspecified → 3.7.1
I checked in Bob's patch so this will be fixed in NSS 3.10.1.

Checking in dsa.c;   new revision: 1.14; previous revision: 1.13
Last Resolved: 12 years ago
Resolution: --- → FIXED


12 years ago
Target Milestone: 3.10.1 → 3.10.2
You need to log in before you can comment on or make changes to this bug.