1459141 - Softoken CBC padding removal might have a timing side channel

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Martin Thomson [:mt:]]

NSC_Decrypt[Final] takes some steps to avoid creating a timing oracle, but I believe that it isn't perfect.

Here's the code: https://searchfox.org/nss/rev/07e15231b59638e9babc79d63493eab766d12595/lib/softoken/pkcs11c.c#1578

And the comparable code in our TLS code: https://searchfox.org/nss/rev/07e15231b59638e9babc79d63493eab766d12595/lib/ssl/ssl3con.c#11684

You will note that the padding removal in the TLS code always reads 255 octets (up to a limit of what is available), where softoken isn't anywhere near as careful.

Comment 1

[Deian Stefan]

Seems like NSC_Decrypt (non Final) has the same issue here:

        if (padding > context->blockSize || !padding) {      // <-- treated as public
            crv = CKR_ENCRYPTED_DATA_INVALID;
        } else {
            unsigned int i;
            unsigned int badPadding = 0; /* used as a boolean */
            for (i = 0; i < padding; i++) {       // <-- treated as public
                badPadding |= (unsigned int)pData[outlen - 1 - i] ^ padding; // <-- treated as if secret
            }

Comment 2

[Deian Stefan]

Attachment: 0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

Comment 3

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9098693 [details] [diff] [review]
0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

Review of attachment 9098693 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/pkcs11c.c
@@ +1617,5 @@
> +    unsigned int padSize = (unsigned int)pLastPart[blockSize - 1];
> +
> +    /* if ((padSize > blockSize) || (padSize == 0)) { */
> +    /*     goodPAD = ~0; */
> +    /* } */

Just a nit, but can we make the multi-line comment formatting match the one below?

Comment 4

[Deian Stefan]

Attachment: 0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

That was lingering code for testing the parts of the function as I was porting it to CT. Should not have been there, sorry.

Comment 5

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9100345 [details] [diff] [review]
0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

No problem, thank you!

r+ for check-in after clang-format.

Comment 6

[Deian Stefan]

Attachment: 0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

clang-format, carrying r+

Comment 7

[J.C. Jones [:jcj] (he/they)]

Try push failed on Windows, cannot land. (https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=08aee908a2c4)

c:/Users/task_1570825892/nss/lib/softoken/pkcs11c.c(1623): warning C4146: unary minus operator applied to unsigned type, result still unsigned

Furthermore, I have a review comment, sorry that I didn't review this sooner than now:

As changed, the NSC_DecryptFinal function modifies pulLastPartLen even on crv being a failure, which we generally try to avoid. Due to different construction NSC_Decrypt still appears to behave that way. We shouldn't regress that behavior for NSC_DecryptFinal

Comment 8

[J.C. Jones [:jcj] (he/they)]

I've already tagged NSS_3_47_BETA1 but I will be happy to take this for NSS_3_47_BETA2.

Comment 9

[Deian Stefan]

Attachment: 0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

• Since we just want CT_NOT_ZERO(X) to give us all-zeros for 0 and all-ones otherwise, I just replaced -(x) with (0-x) in the macro. Not sure if there is a different convention elsewhere in the codebase to follow though.

• Fixed NSC_DecryptFinal; copied a helper function from ssl3con.c. May be nice to share some of these helper functions/macros.

Comment 10

[Deian Stefan]

Due to different construction NSC_Decrypt still appears to behave that way.

With refactoring of the padding code out it's pretty easy to fix this too. Should we just do it in this patch?

Comment 11

[J.C. Jones [:jcj] (he/they)]

I don't feel strongly either way on expanding this patch. Kevin?

Comment 12

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9100791 [details] [diff] [review]
0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

Review of attachment 9100791 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Kevin, can you take one more look?

Try push is here: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=01f948011f38da9f374a7ae61b9b80e37b6e69e2

Comment 13

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9100791 [details] [diff] [review]
0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

Review of attachment 9100791 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me as-is.

Comment 14

[Kevin Jacobs [:kjacobs]]

^ Clearing NI.

Comment 15

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/474d62c9d0db3f8c3780e62a1a16c91c364fee14

Comment 16

[Franziskus Kiefer [:franziskus]]

This breaks u2f soft tokens in Firefox.

Comment 17

[J.C. Jones [:jcj] (he/they)]

I'm shortly going to back this back out for a BETA3 to keep our uplift train moving. We can make a BETA4 as necessary.

Comment 18

[J.C. Jones [:jcj] (he/they)]

Backed out: https://hg.mozilla.org/projects/nss/rev/f10c3e0757b7b2cef3fb6201b2a9be795c6aecd4

Comment 19

[Deian Stefan]

Attachment: 0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

u2f tests passes and all nss tests pass

Comment 20

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9101693 [details] [diff] [review]
0001-Bug-1459141-Rewrite-softoken-CBC-pad-check-to-be-con.patch

Review of attachment 9101693 [details] [diff] [review]:
-----------------------------------------------------------------

See vectors in https://phabricator.services.mozilla.com/D49503

Comment 21

[Deian Stefan]

Attachment: Rewrite softoken CBC pad check to be constant

Comment 22

[Deian Stefan]

Attachment: Bug 1459141 - A few more CBC padding tests.

This patch adds more test vectors for AES-CBC and 2DES-CBC padding.

Comment 23

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/d3c8638f85cdc4ed6c114406fca49314514bc097

Comment 24

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/38f1c92a5e1175bb8388768a209ac0efdabd1bd7