191470 - Multipart CKM_DSA_SHA1 signing broken if given large buffer

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Andreas Sterbenz]

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]

Bob, could you look at this?  Thanks.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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]

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]

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)]

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)]

2+ year old bug on the correctness of softoken.  Bob, please fix for 3.10

Comment 7

[Robert Relyea]

Attachment: Freebl should allow larger buffers.

Untested batch.

Comment 8

[Robert Relyea]

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)]

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)]

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)]

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