Closed
Bug 191467
Opened 22 years ago
Closed 21 years ago
Multipart signing and verifying broken for several mechanisms in softoken
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.9
People
(Reporter: andreas.st, Assigned: rrelyea)
Details
Attachments
(1 file, 1 obsolete file)
710 bytes,
patch
|
nelson
:
superreview+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
Bob, could you take a look at this? Thanks.
Assignee: wtc → relyea
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•21 years ago
|
||
P2 (at least). Nominating for 3.9. Bob, You can assign this to me if you want.
Priority: -- → P2
Target Milestone: --- → 3.9
Assignee | ||
Comment 3•21 years ago
|
||
The DSA setting has already been removed in the NSS tip. removing the RSA version.
Assignee | ||
Updated•21 years ago
|
Attachment #132935 -
Flags: superreview?(jpierre)
Attachment #132935 -
Flags: review?(MisterSSL)
Updated•21 years ago
|
Attachment #132935 -
Flags: superreview?(jpierre) → superreview+
Comment 4•21 years ago
|
||
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-
Assignee | ||
Comment 5•21 years ago
|
||
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.
Assignee | ||
Comment 6•21 years ago
|
||
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;).
Attachment #132935 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #132963 -
Flags: superreview?(MisterSSL)
Attachment #132963 -
Flags: review?(jpierre)
Updated•21 years ago
|
Attachment #132935 -
Flags: review?(MisterSSL) → review-
Comment 7•21 years ago
|
||
Comment on attachment 132963 [details] [diff] [review] more complete version of the path sr=MisterSSL
Attachment #132963 -
Flags: superreview?(MisterSSL) → superreview+
Assignee | ||
Comment 8•21 years ago
|
||
Checking in pkcs11c.c; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v <-- pkcs11c.c new revision: 1.51; previous revision: 1.50 done
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Attachment #132963 -
Flags: review?(jpierre)
You need to log in
before you can comment on or make changes to this bug.
Description
•