Closed
Bug 618418
Opened 14 years ago
Closed 12 years ago
Softoken returns unhelpful error code from C_DecryptFinal when PKCS7 padding is wrong
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.14.2
People
(Reporter: briansmith, Assigned: wtc)
References
Details
Attachments
(2 files, 2 obsolete files)
2.53 KB,
patch
|
Details | Diff | Splinter Review | |
3.39 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
in NSC_DecryptFinal: ... if ((padSize > context->blockSize) || (padSize == 0)) { rv = SECFailure; } else { ... } ... return (rv == SECSuccess) ? CKR_OK : sftk_MapDecryptError(PORT_GetError()); Since the code does not PORT_SetError(), the result of PORT_GetError will be unpredictable which means the return value of NSC_DecryptFinal will be unpredictable.
Reporter | ||
Comment 1•13 years ago
|
||
The padding will be wrong in almost every erroneous case (wrong decryption key used, corrupted input data, etc).
Target Milestone: --- → 3.12.10
Updated•13 years ago
|
Target Milestone: 3.12.10 → ---
Reporter | ||
Updated•13 years ago
|
Target Milestone: --- → 3.12.10
Assignee | ||
Comment 2•12 years ago
|
||
In Chromium bug 124434 (http://code.google.com/p/chromium/issues/detail?id=124434) I discovered that C_DecryptFinal does not check all the padding bytes. bsmith reported a similar problem with libSSL in bug 571796. The patch I wrote for that problem also fixed this bug. So I'm going to just use this bug report for both problems. Note that SEC_ERROR_BAD_DATA is mapped to CKR_ENCRYPTED_DATA_INVALID: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11c.c&rev=1.124&mark=138,141-142#136
Attachment #621222 -
Flags: review?(bsmith)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: 3.12.10 → 3.14
Reporter | ||
Comment 3•12 years ago
|
||
Comment on attachment 621222 [details] [diff] [review] Patch (also fix softoken's equivalent of bug 571796) Review of attachment 621222 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/security/nss/lib/softoken/pkcs11c.c @@ +1205,5 @@ > + padSize) { > + badPadding = PR_TRUE; > + break; > + } > + } It would be better to use a constant-time way of checking the padding, to minimize the likelihood that timing attacks would be effective, like I did in the libssl patch. In fact, it may even be good to factor out this kind of check into a reusable function, like Adam Langley did in Go's "subtle" library. If it's never important, for some reason, to worry about timing issues here, then this patch is reasonable and I can switch my review to r+. But, it isn't clear why timing attacks can be ruled out regardless of the caller of this code.
Attachment #621222 -
Flags: review?(bsmith) → review-
Assignee | ||
Comment 4•12 years ago
|
||
Use a constant-time way of checking the padding. It is thought provoking whether the timing of checking the padding can be exploited by attackers. It is less work to just implement your suggestion and not have to worry about it.
Attachment #621222 -
Attachment is obsolete: true
Attachment #648914 -
Flags: review?(bsmith)
Assignee | ||
Updated•12 years ago
|
Target Milestone: 3.14 → 3.14.1
Reporter | ||
Comment 5•12 years ago
|
||
Comment on attachment 648914 [details] [diff] [review] Patch (also fix softoken's equivalent of bug 571796), v2 Wan-Teh, this patch works as it is. I realized that if we expand the context a little bit, in NSC_DecryptFinal we're effectively using PR_SetError() and PR_GetError() as a local variable, which is kind of ugly. If we were to redo the code in CK_RV NSC_DecryptFinal to avoid doing that, then the code would be more similar between the two functions and could possibly be factored out into a separate function to reduce the duplication. it's up to you if you want to do this.
Attachment #648914 -
Flags: review?(bsmith) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #648914 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
Brian: I made the change you suggested -- use the local variable 'crv' instead of PORT_SetError() and PORT_GetError() in NSC_DecryptFinal. But I don't have time to factor out the common code between NSC_DecryptFinal and NSC_Decrypt into a new function. If you don't have time to review this new patch, I'll just check in patch v2. Thanks.
Attachment #690680 -
Flags: review?(bsmith)
Reporter | ||
Updated•12 years ago
|
Attachment #690680 -
Flags: review?(bsmith) → review+
Assignee | ||
Comment 8•12 years ago
|
||
Patch v3 checked in on the NSS trunk (NSS 3.14.2). Checking in pkcs11c.c; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v <-- pkcs11c.c new revision: 1.133; previous revision: 1.132 done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: 3.14.1 → 3.14.2
You need to log in
before you can comment on or make changes to this bug.
Description
•