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)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.14.2

People

(Reporter: briansmith, Assigned: wtc)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
See Also: → 624042
The padding will be wrong in almost every erroneous case (wrong decryption key used, corrupted input data, etc).
Target Milestone: --- → 3.12.10
Target Milestone: 3.12.10 → ---
Target Milestone: --- → 3.12.10
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: nobody → wtc
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: 3.12.10 → 3.14
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-
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)
Target Milestone: 3.14 → 3.14.1
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+
Attachment #648914 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
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)
Attachment #690680 - Flags: review?(bsmith) → review+
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.

Attachment

General

Creator:
Created:
Updated:
Size: