Better constant-flow idioms for TLS-CBC padding checks #3638
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpg
added
enhancement
mbed TLS team
needs-review
I do think we should have unit tests even for small functions. We don't want another ge/gt confusion, and we'll eventually run unit tests on M-class. But it doesn't need to be done in this PR. |
|
@gilles-peskine-arm So I think I'll postpone unit testing of the those utility functions to a later PR, then. Perhaps this could be done at the same time as unifying with other similar functions. |
|
Please fix the signoff lines. The wish for unifying constant-flow functions is recorded in #3649. |
The previous code used comparison operators >= and == that are quite likely to
be compiled to branches by some compilers on some architectures (with some
optimisation levels).
For example, take the following function:
void old_update( size_t data_len, size_t *padlen )
{
*padlen *= ( data_len >= *padlen + 1 );
}
With Clang 3.8, let's compile it for the Arm v6-M architecture:
% clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - |
sed -n '/^old_update:$/,/\.size/p'
old_update:
.fnstart
@ BB#0:
.save {r4, lr}
push {r4, lr}
ldr r2, [r1]
adds r4, r2, #1
movs r3, #0
cmp r4, r0
bls .LBB0_2
@ BB#1:
mov r2, r3
.LBB0_2:
str r2, [r1]
pop {r4, pc}
.Lfunc_end0:
.size old_update, .Lfunc_end0-old_update
We can see an unbalanced secret-dependant branch, resulting in a total
execution time depends on the value of the secret (here padlen) in a
straightforward way.
The new version, based on bit operations, doesn't have this issue:
new_update:
.fnstart
@ BB#0:
ldr r2, [r1]
subs r0, r0, #1
subs r0, r0, r2
asrs r0, r0, #31
bics r2, r0
str r2, [r1]
bx lr
.Lfunc_end1:
.size new_update, .Lfunc_end1-new_update
(As a bonus, it's smaller and uses less stack.)
While there's no formal guarantee that the version based on bit operations in
C won't be translated using branches by the compiler, experiments tend to show
that's the case [1], and it is commonly accepted knowledge in the practical
crypto community that if we want to sick to C, bit operations are the safest
bet [2].
[1] https://github.com/mpg/ct/blob/master/results
[2] https://github.com/veorq/cryptocoding
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
MSVC complains about it otherwise. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
According to https://www.bearssl.org/ctmul.html even single-precision multiplication is not constant-time on some older platforms. An added benefit of the new code is that it removes the somewhat mysterious constant 0x1ff - which was selected because at that point the maximum value of padlen was 256. The new code is perhaps a bit more readable for that reason. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
force-pushed
the
better-cf-padding-checks
branch
from
4953dff to
822b372
Compare
|
Note: the Travis failure is something that was fixed in development in the meantime. |
ronald-cron-arm
requested changes
Sep 23, 2020
|
@ronald-cron-arm Thanks for your review! I like your suggestions for improving comments, and I think this is an area where good comments are particularly important. I'll update the PR based on your suggestions and ping you when I'm done. |
mpg
added
needs-work
needs-review
ronald-cron-arm
requested changes
Sep 25, 2020
library/ssl_msg.c
Outdated
| @@ -1124,7 +1140,7 @@ static size_t mbedtls_ssl_cf_bool_eq( size_t x, size_t y ) | |||
| #pragma warning( pop ) | |||
| #endif | |||
|
|
|||
| /* diff1 = (x != y) in {0, 1} */ | |||
| /* diff1 = (x != y) : 1 : 0 */ | |||
There was a problem hiding this comment.
Typo: : instead of ? before 1.
library/ssl_msg.c
Outdated
| @@ -1630,7 +1646,8 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, | |||
| { | |||
| /* This is the SSL 3.0 path, we don't have to worry about Lucky | |||
| * 13, because there's a strictly worse padding attack built in | |||
| * the protocol (known as part of POODLE), so branches are OK. */ | |||
| * the protocol (known as part of POODLE), so we don't care if the | |||
| * code is not constant-time, in particualar branches are OK. */ | |||
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
force-pushed
the
better-cf-padding-checks
branch
from
636be2c to
6d6f8a4
Compare
ronald-cron-arm
approved these changes
Sep 28, 2020
|
Regarding CI results:
|
gilles-peskine-arm
approved these changes
Oct 5, 2020
gilles-peskine-arm
added
needs-ci
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #3472
Note on scope: as a follow-up it would be interesting to see if code can be shared with other places that do constant-flow comparisons, for example in the SSL module there's also ssl_parse_encrypted_pms(), but I decided it was out of scope of this PR, in order to avoid design questions. In the longer term, it would probably even be interesting to have a small library of such functions that could be shared with (PSA) Crypto and perhaps even other tf.org projects, but that's obviously even more out of scope. For now I've just tried to avoid too much repetition among the functions used by CBC (which btw is mainly about code maintainability, not code size as the compile will likely inline a lot even with -Os/-Oz).
Note on testing: I don't think it would be that useful to add a unit-test for say mbedtls_ssl_cf_mask_ge() with MemSan/Valgrind constant-time checking, because those checks would be run on x86 or A-class, where the previous code was already constant-time anyway, and the function is so small I'm not sure it makes sense.
I'm undecided whether it would make sense to have a unit test for this function (or other similar small functions) just to check correctness of the results, though, because it's so small. Opinions welcome.
Status
READY
Requires Backporting
NO
Migrations
NO