191467 - Multipart signing and verifying broken for several mechanisms in softoken

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: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212

context->multi is incorrectly set to PR_FALSE for CKM_*_RSA_PKCS in
NSC_SignInit() and for CKM_DSA_SHA1 in NSC_VerifyInit(). At the beginning of the
function it is preset to PR_FALSE, then set to PR_TRUE in a case statement, but
later incorrectly set back to PR_FALSE after a fall-through/goto.

The effect is that SignFinal() and VerifyFinal() fail with
CKR_OPERATION_NOT_INITIALIZED. The second pair lines setting the value to
PR_FALSE should be removed.


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

Similar for the verify case.



Diffs for NSS 3.7.1. Should work as is or with small modifications for current
CVS version.

***
/export/home/as130790/ws/tests/nss/org/nss-3.7.1/mozilla/security/nss/lib/softoken/pkcs11c.c
Fri Dec 13 00:02:08 2002
--- pkcs11c.c	Fri Jan 31 16:18:48 2003
***************
*** 1895,1901 ****
  	    crv = CKR_KEY_TYPE_INCONSISTENT;
  	    break;
  	}
- 	context->multi = PR_FALSE;
  	privKey = pk11_GetPrivKey(key,CKK_RSA);
  	if (privKey == NULL) {
  	    if (info) PORT_Free(info);
--- 1895,1900 ----
***************
*** 2344,2350 ****
  	    crv = CKR_KEY_TYPE_INCONSISTENT;
  	    break;
  	}
- 	context->multi = PR_FALSE;
  	pubKey = pk11_GetPubKey(key,CKK_DSA);
  	if (pubKey == NULL) {
  	    crv = CKR_HOST_MEMORY;
--- 2343,2348 ----

Comment 1

[Wan-Teh Chang]

Bob, could you take a look at this?  Thanks.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

P2 (at least).  Nominating for 3.9.  
Bob, You can assign this to me if you want.

Comment 3

[Robert Relyea]

Attachment: Remove incorrect context->multi setting.

The DSA setting has already been removed in the NSS tip. removing the RSA
version.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

CKM_*_RSA_PKCS is defined to only allow single part, not multi-part. 
So, I'd think that the RIGHT change would make context->multi be consistently
PR_FALSE, not PR_TRUE.
The submittor's patch does that.  The patch attached above does not.
Bob, can you convince me that making context->multi be true is correct?
Otherwise, r-

Comment 5

[Robert Relyea]

Hmm. So I thought both problems were in SignInit... but there was one in
SignInit for RSA and one in VerifyInit for DSA. My patch for SignInit is
identical to the submitter in the case Nelson is talking about. I'll supply a
patch in a second that's identical to the submitters for both cases (except it's
targetting for the tip, not NSS 3.7.

context->multi is initialized to PR_FALSE before entering the switch statement.
Not all CKM_*_RSA_PKCS funtions should have context->multi set to PR_FALSE; only
CKM_RSA_PKCS and CKM_RSA_X_509. CKM_SHA1_RSA_PKCS should have context->multi set
to PR_TRUE, as these functions take aribrary input and hash the result
internally before signing.

The code path where the context->multi value was set explicitly set to PR_FALSE
in the switch statement is shared between the hashing and non-hashing functions
(and thus the bug the submitter describes).

I looked at the code more closely and released the DSA bug is not in SignInit,
but VerifyInit.... 

BTW if the context->multi was set incorrectly for the CKM_RSA_PKCS case, we
would see that pretty quickly in our test cases as all our signatures would
fail. NSS currently does not use the hash and sign mechanisms, which is why we
did not notice this (probably longstanding) bug.

Comment 6

[Robert Relyea]

Attachment: more complete version of the path

Amazing the overhead to get a 2 line patch into the system;). Also amazing how
much damage a 2 line patch can do if it was wrong;).

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132963 [details] [diff] [review]
more complete version of the path

sr=MisterSSL

Comment 8

[Robert Relyea]

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.51; previous revision: 1.50
done