Last Comment Bug 191470 - Multipart CKM_DSA_SHA1 signing broken if given large buffer
: Multipart CKM_DSA_SHA1 signing broken if given large buffer
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.7.1
: All All
: P2 normal (vote)
: 3.10.2
Assigned To: Robert Relyea
: Bishakha Banerjee
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-01-31 10:12 PST by Andreas Sterbenz
Modified: 2005-07-28 16:12 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Freebl should allow larger buffers. (949 bytes, patch)
2005-03-23 11:28 PST, Robert Relyea
nelson: review+
Details | Diff | Splinter Review

Description Andreas Sterbenz 2003-01-31 10:12:13 PST
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.

***
/export/home/as130790/ws/tests/nss/org/nss-3.7.1/mozilla/security/nss/lib/freebl/dsa.c
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)) {
  	PORT_SetError(SEC_ERROR_INVALID_ARGS);
  	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)) {
  	PORT_SetError(SEC_ERROR_INVALID_ARGS);
  	return SECFailure;
Comment 1 Wan-Teh Chang 2003-01-31 13:07:26 PST
Bob, could you look at this?  Thanks.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2003-05-09 18:28:54 PDT
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.
Comment 3 Andreas Sterbenz 2003-05-13 06:17:53 PDT
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 Robert Relyea 2003-05-13 08:12:57 PDT
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
garbage).

bob
Comment 5 Nelson Bolyard (seldom reads bugmail) 2003-05-13 19:43:50 PDT
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.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2005-03-13 00:18:34 PST
2+ year old bug on the correctness of softoken.  Bob, please fix for 3.10
Comment 7 Robert Relyea 2005-03-23 11:28:12 PST
Created attachment 178389 [details] [diff] [review]
Freebl should allow larger buffers.

Untested batch.
Comment 8 Robert Relyea 2005-03-23 11:30:42 PST
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.

bob
Comment 9 Nelson Bolyard (seldom reads bugmail) 2005-04-15 20:27:05 PDT
Comment on attachment 178389 [details] [diff] [review]
Freebl should allow larger buffers.

Looks good to me.  Bob, please check it in.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2005-05-05 11:42:17 PDT
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.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2005-05-21 14:36:26 PDT
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

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