Backport 2.2x: range-based constant-flow base64 #4835

gilles-peskine-arm · 2021-08-03T10:46:34Z

Compared to the 2.16 version:

The commit "New sample program to benchmark certificate loading" had some conflicts in build scripts due to refactorings that happened after 2.16. In all cases it was just a matter of adding the new file in the right place.
There are extra commits to unit-test the new internal functions (which isn't something we did in 2.16). The unit tests validate that the new internal functions are constant-flow when running under a properly instrumented valgrind or msan. There is still no validation of constant flow for mbedtls_base64_decode itself due to its logic relying on equality tests of string contents against non-digit values (e.g. src[i] == ' '); I haven't found a way to get rid of those equality tests without a significant cost in both performance and code size.

I expect the 3.0 forward-port to be identical except for further build script tweaks and for timing in load_roots needing adjustments (or maybe we can do the easy thing and remove it). I'll make it after the 2.2x version has at least one approval.

mpg

I've verified that the first commits (up to 4ac01d2) are a forward-port of the 2.16 PR, with the obvious conflict resolution, and the manually reviewed the other commits. I'm generally happy with the testing strategy, but have a few reservations about specific points of its implementation.

include/mbedtls/base64.h

tests/suites/test_suite_base64.function

mpg

LGTM

davidhorstmann-arm

Looks good apart from 2 nits that I also put on the other backport (since they apply to both I was unsure where to raise them).

davidhorstmann-arm · 2021-08-18T15:31:54Z

ChangeLog.d/base64-ranges.txt

+Changes
+   * Improve the performance of base64 constant-flow code. The result is still
+     slower than the original non-constant-flow implementation, but much faster
+     than the previous constant-flow implemenation. Fixes #4814.


typo: "implemenation" -> "implementation"

davidhorstmann-arm · 2021-08-18T15:33:05Z

library/base64.c

-    return (unsigned char) ( 1 ^ difference );
+    /* low_mask is: 0 if low <= c, 0x...ff if low > c */
+    unsigned low_mask = ( (unsigned) c - low ) >> 8;
+    /* high_mask is: 0 if c <= high, 0x...ff if high > c */


This comment is wrong: high > c should be c > high or high < c

n was used for two different purposes. Give it a different name the second time. This does not seem to change the generated code when compiling with optimization for size or performance. Signed-off-by: Gilles Peskine <[email protected]>

To test c <= high, instead of testing the sign of (high + 1) - c, negate the sign of high - c (as we're doing for c - low). This is a little easier to read and shaves 2 instructions off the arm thumb build with arm-none-eabi-gcc 7.3.1. Signed-off-by: Gilles Peskine <[email protected]>

I had originally thought to support directories with mbedtls_x509_crt_parse_path but it would have complicated the code more than I cared for. Remove a remnant of the original project in the documentation. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

Add unit tests for mask_of_range(), enc_char() and dec_value(). When constant-flow testing is enabled, verify that these functions are constant-flow. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

This is part of the definition of the encoding, not a choice of test parameter, so keep it with the test code. Signed-off-by: Gilles Peskine <[email protected]>

digits is also a local variable in host_test.function, leading to compilers complaining about that shadowing the global variable in test_suite_base64.function. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

mpg

Re-approving after rebase - I checked that the only differences are the ones announced.

davidhorstmann-arm

LGTM - thanks!

gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-reviewer This PR needs someone to pick it up for review labels Aug 3, 2021

gilles-peskine-arm requested a review from mpg August 3, 2021 10:46

gilles-peskine-arm added needs-ci Needs to pass CI tests needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Aug 3, 2021

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests needs-work labels Aug 3, 2021

gilles-peskine-arm linked an issue Aug 4, 2021 that may be closed by this pull request

Huge certificate parsing speed regression between mbedtls 2.16.9 and 2.16.10 (constant time base64). #4814

Closed

mpg requested changes Aug 6, 2021

View reviewed changes

include/mbedtls/base64.h Outdated Show resolved Hide resolved

tests/suites/test_suite_base64.function Outdated Show resolved Hide resolved

tests/suites/test_suite_base64.function Outdated Show resolved Hide resolved

daverodgman added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 6, 2021

gilles-peskine-arm requested a review from mpg August 6, 2021 12:57

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 6, 2021

gilles-peskine-arm assigned tom-daubney-arm Aug 6, 2021

mpg mentioned this pull request Aug 9, 2021

Backport 2.16: range-based constant-flow base64 #4819

Merged

mpg previously approved these changes Aug 9, 2021

View reviewed changes

davidhorstmann-arm requested changes Aug 18, 2021

View reviewed changes

mpg removed the needs-reviewer This PR needs someone to pick it up for review label Aug 19, 2021

gilles-peskine-arm added 15 commits October 25, 2021 21:24

Base64 decode: simplify local variables (n)

831fd76

n was used for two different purposes. Give it a different name the second time. This does not seem to change the generated code when compiling with optimization for size or performance. Signed-off-by: Gilles Peskine <[email protected]>

load_roots: fix no-argument detection

373c54e

Signed-off-by: Gilles Peskine <[email protected]>

load_roots: properly error out on an invalid option

38d41b9

Signed-off-by: Gilles Peskine <[email protected]>

Expose internal base64 functions for testing

2c78f98

Signed-off-by: Gilles Peskine <[email protected]>

Add unit tests for base64 internal functions

3b1f4c0

Add unit tests for mask_of_range(), enc_char() and dec_value(). When constant-flow testing is enabled, verify that these functions are constant-flow. Signed-off-by: Gilles Peskine <[email protected]>

Fix printf format signedness error

85f3165

Signed-off-by: Gilles Peskine <[email protected]>

Mark output as public before testing it

0f4e3fb

Signed-off-by: Gilles Peskine <[email protected]>

Fix the build of sample programs without mbedtls_strerror

3f0722c

Signed-off-by: Gilles Peskine <[email protected]>

Move declarations of testing-only base64 functions to their own header

a138802

Signed-off-by: Gilles Peskine <[email protected]>

Move the list of Base64 digits out of the test data

fd489f9

This is part of the definition of the encoding, not a choice of test parameter, so keep it with the test code. Signed-off-by: Gilles Peskine <[email protected]>

Rename variable to avoid a name clash

d3e5dd3

digits is also a local variable in host_test.function, leading to compilers complaining about that shadowing the global variable in test_suite_base64.function. Signed-off-by: Gilles Peskine <[email protected]>

Fix copypasta in comment

773f5b7

Signed-off-by: Gilles Peskine <[email protected]>

Fix typo in documentation

1222fed

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the base64-no-table-2.2x branch from 3e49b1a to 1222fed Compare October 25, 2021 19:24

gilles-peskine-arm requested review from davidhorstmann-arm and mpg October 25, 2021 19:27

gilles-peskine-arm removed the needs-work label Oct 25, 2021

gilles-peskine-arm mentioned this pull request Oct 25, 2021

range-based constant-flow base64 #5108

Merged

mpg approved these changes Oct 26, 2021

View reviewed changes

davidhorstmann-arm approved these changes Oct 27, 2021

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 27, 2021

mpg merged commit 8e8251a into Mbed-TLS:development_2.x Oct 27, 2021

nokute78 mentioned this pull request Jan 14, 2022

aws: fork mbedtls 2.25.0 base64 utility to avoid 2.26.0+ performance hit fluent/fluent-bit#4422

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport 2.2x: range-based constant-flow base64 #4835

Backport 2.2x: range-based constant-flow base64 #4835

gilles-peskine-arm commented Aug 3, 2021 •

edited

Loading

mpg left a comment

mpg left a comment

davidhorstmann-arm left a comment

davidhorstmann-arm Aug 18, 2021

davidhorstmann-arm Aug 18, 2021

mpg left a comment

davidhorstmann-arm left a comment

Backport 2.2x: range-based constant-flow base64 #4835

Backport 2.2x: range-based constant-flow base64 #4835

Conversation

gilles-peskine-arm commented Aug 3, 2021 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

davidhorstmann-arm Aug 18, 2021

Choose a reason for hiding this comment

davidhorstmann-arm Aug 18, 2021

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 3, 2021 •

edited

Loading