Remove a secret-dependent branch in Montgomery multiplication #3398

gilles-peskine-arm · 2020-06-04T19:54:47Z

Remove a secret-dependent branch in Montgomery multiplication that could expose some bits of a private RSA key.

The potential leak was documented in https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-lee-sangho.pdf and again in https://arxiv.org/pdf/2005.11516.pdf and reported in #3394.

Fix #3394.

In this pull request, I reverted commit 2cc69ff from #457, partly because the only real value of the check it added was to silence a false positive from clang-analyzer, and partly because half of the check was wrong anyway. I think this reversal should be backported with the rest, because it has the benefit that it more than compensates the additional code size from this security fix, and the risk of removing a redundant check is minimal.

Needs backports to all supported branches.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

This reverts commit 2cc69ff. A check was added in mpi_montmul because clang-analyzer warned about a possibly null pointer. However this was a false positive. Recent versions of clang-analyzer no longer emit a warning (3.6 does, 6 doesn't). Incidentally, the size check was wrong: mpi_montmul needs T->n >= 2 * (N->n + 1), not just T->n >= N->n + 1. Given that this is an internal function which is only used from one public function and in a tightly controlled way, remove both the null check (which is of low value to begin with) and the size check (which would be slightly more valuable, but was wrong anyway). This allows the function not to need to return an error, which makes the source code a little easier to read and makes the object code a little smaller. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Separate out a version of mpi_safe_cond_assign that works on equal-sized limb arrays, without worrying about allocation sizes or signs. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In mpi_montmul, an auxiliary function for modular exponentiation (mbedtls_mpi_mod_exp) that performs Montgomery multiplication, the last step is a conditional subtraction to force the result into the correct range. The current implementation uses a branch and therefore may leak information about secret data to an adversary who can observe what branch is taken through a side channel. Avoid this potential leak by always doing the same subtraction and doing a contant-trace conditional assignment to set the result. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg · 2020-06-05T08:44:29Z

From the CI: MSVC 2013 would like to be sure that you know what you're doing with one cast:

library\bignum.c(2054): warning C4244: 'function' : conversion from 'mbedtls_mpi_uint' to 'unsigned char', possible loss of data [C:\builds\ws\3398-merge-OAXQDFVLLPBJL62OKBYHC\worktrees\tmpf950cwbj\cmake_solution\library\mbedcrypto.vcxproj]

The comment above states that the value being cast is always 0 or 1, so that cast is safe, but we probably want to make it explicit to let MSVC know that we carefully considered it.

yanesca

I have a couple of questions and a minor suggestion for improvement.

library/bignum.c

Let code analyzers know that this is deliberate. For example MSVC warns about the conversion if it's implicit. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg

Just releasing comments so far because otherwise I can't participate in the conversation.

library/bignum.c

mpg

I'm happy with this PR, except for one place where I think the comments need to be improved.

library/bignum.c

mpi_sub_hlp performs a subtraction A - B, but took parameters in the order (B, A). Swap the parameters so that they match the usual mathematical syntax. This has the additional benefit of putting the output parameter (A) first, which is the normal convention in this module. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The function mpi_sub_hlp had confusing semantics: although it took a size parameter, it accessed the limb array d beyond this size, to propagate the carry. This made the function difficult to understand and analyze, with a potential buffer overflow if misused (not enough room to propagate the carry). Change the function so that it only performs the subtraction within the specified number of limbs, and returns the carry. Move the carry propagation out of mpi_sub_hlp and into its caller mbedtls_mpi_sub_abs. This makes the code of subtraction very slightly less neat, but not significantly different. In the one other place where mpi_sub_hlp is used, namely mpi_montmul, this is a net win because the carry is potentially sensitive data and the function carefully arranges to not have to propagate it. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

There was some confusion during review about when A->p[n] could be nonzero. In fact, there is no need to set A->p[n]: only the intermediate result d might need to extend to n+1 limbs, not the final result A. So never access A->p[n]. Rework the explanation of the calculation in a way that should be easier to follow. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The function mbedtls_mpi_sub_abs first checked that A >= B and then performed the subtraction, relying on the fact that A >= B to guarantee that the carry propagation would stop, and not taking advantage of the fact that the carry when subtracting two numbers can only be 0 or 1. This made the carry propagation code a little hard to follow. Write an ad hoc loop for the carry propagation, checking the size of the result. This makes termination obvious. The initial check that A >= B is no longer needed, since the function now checks that the carry propagation terminates, which is equivalent. This is a slight performance gain. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

library/bignum.c

gilles-peskine-arm · 2020-06-08T21:15:29Z

This change is more extensive than I'd like for a security improvement. Is it acceptable to backport as is?

Note that in terms of code size, there's a slight increase in the semantically necessary commits, which is more than compensated by the non-mandatory commit “Revert "Shut up a clang-analyzer warning"” .

mpg

I'm happy with the code now, but found a few issues in some comments.

mpg · 2020-06-09T08:11:02Z

library/bignum.c

+ * \param n             Number of limbs of \p d and \p s.
+ * \param[in,out] d     On input, the left operand.
+ *                      On output, the result of the subtraction:
+ * \param[s]            The right operand.


Syntax error: I think you mean param[in] s.

(Btw, less important, but you're using doxygen syntax in something that's not a doxygen comment. Maybe it would be cleaner to start the comment with /**, so that various tools such as clang's -Wdocumentation can check it.)

Regarding the second part, I was a bit surprised check-doxy-blocks didn't complain. I checked the CI results, and it did complain.

mpg · 2020-06-09T08:21:05Z

library/bignum.c

+    /* Copy the n least significant limbs of d to A, so that
+     * A = d if d < N (recall that N has n limbs). */
+    memcpy( A->p, d, n * ciL );
+    /* If d >= N then we want to set A to N - d. To prevent timing attacks,


I think you mean d - N here.

mpg · 2020-06-09T08:24:36Z

library/bignum.c

-    if( mbedtls_mpi_cmp_abs( A, B ) < 0 )
-        return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE );
+    /* if( mbedtls_mpi_cmp_abs( A, B ) < 0 ) */
+    /*     return( MBEDTLS_ERR_MPI_NEGATIVE_VALUE ); */


Was it your intention to leave this code commented, or did you mean to remove it?

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg

Thanks for fixing the comments. Looks all good to me now.

Merge pull request ARMmbed#3398 from gilles-peskine-arm/montmul-cmp-b…

gilles-peskine-arm · 2021-01-18T21:40:56Z

library/bignum.c

    MPI_VALIDATE_RET( X != NULL );
    MPI_VALIDATE_RET( A != NULL );
    MPI_VALIDATE_RET( B != NULL );

-    if( mbedtls_mpi_cmp_abs( A, B ) < 0 )


The initial check that A >= B is no longer needed, since the function now checks that the carry propagation terminates, which is equivalent.

Oops. The carry propagation check is equivalent, but that's assuming it is reached. If B->n > A->n, or more precisely if B without leading null limbs is larger than A (with its leading null limbs), then mpi_sub_hlp overflows X->p.

Add a const annotation to the non-changing argument of mpi_sub_mul

742f1a4

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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 labels Jun 4, 2020

gilles-peskine-arm added this to the June 2020 Sprint milestone Jun 4, 2020

gilles-peskine-arm requested review from mpg and yanesca June 4, 2020 19:54

gilles-peskine-arm self-assigned this Jun 4, 2020

gilles-peskine-arm added 5 commits June 4, 2020 21:55

Document some internal bignum functions

2a82f72

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Separate out low-level mpi_safe_cond_assign

f04d11e

Separate out a version of mpi_safe_cond_assign that works on equal-sized limb arrays, without worrying about allocation sizes or signs. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add changelog entry: fix Mbed-TLS#3394

d55bfe9

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the montmul-cmp-branch-development branch from 50b5719 to d55bfe9 Compare June 4, 2020 19:55

yanesca reviewed Jun 5, 2020

View reviewed changes

library/bignum.c Show resolved Hide resolved

library/bignum.c Show resolved Hide resolved

library/bignum.c Outdated Show resolved Hide resolved

yanesca added the needs-ci Needs to pass CI tests label Jun 5, 2020

Explicitly cast down from mbedtls_mpi_uint to unsigned char

026f555

Let code analyzers know that this is deliberate. For example MSVC warns about the conversion if it's implicit. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg reviewed Jun 5, 2020

View reviewed changes

library/bignum.c Show resolved Hide resolved

library/bignum.c Outdated Show resolved Hide resolved

library/bignum.c Outdated Show resolved Hide resolved

mpg added the needs-backports Backports are missing or are pending review and approval. label Jun 5, 2020

mpg self-requested a review June 5, 2020 11:04

mpg previously approved these changes Jun 8, 2020

View reviewed changes

library/bignum.c Outdated Show resolved Hide resolved

danh-arm added the mbed TLS team label Jun 8, 2020

gilles-peskine-arm added 4 commits June 8, 2020 22:05

gilles-peskine-arm dismissed mpg’s stale review via 0e5faf6 June 8, 2020 21:00

gilles-peskine-arm commented Jun 8, 2020

View reviewed changes

library/bignum.c Outdated Show resolved Hide resolved

gilles-peskine-arm commented Jun 8, 2020

View reviewed changes

library/bignum.c Outdated Show resolved Hide resolved

gilles-peskine-arm requested review from yanesca and mpg June 8, 2020 21:20

mpg requested changes Jun 9, 2020

View reviewed changes

mpg added the needs-work label Jun 9, 2020

Clean up some comments

09ec10a

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm requested a review from mpg June 9, 2020 08:40

gilles-peskine-arm removed the needs-work label Jun 9, 2020

mpg approved these changes Jun 9, 2020

View reviewed changes

yanesca approved these changes Jun 9, 2020

View reviewed changes

yanesca removed the needs-review Every commit must be reviewed by at least two team members, label Jun 9, 2020

This was referenced Jun 9, 2020

Backport 2.16: Remove a secret-dependent branch in Montgomery multiplication #3411

Merged

Backport 2.7: Remove a secret-dependent branch in Montgomery multiplication #3412

Merged

mpg removed the needs-ci Needs to pass CI tests label Jun 9, 2020

yanesca added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Jun 9, 2020

yanesca merged commit 3c4a46c into Mbed-TLS:development Jun 9, 2020

fengjixuchui referenced this pull request in fengjixuchui/mbedtls Jun 9, 2020

Merge pull request #17 from ARMmbed/development

589bf88

Merge pull request ARMmbed#3398 from gilles-peskine-arm/montmul-cmp-b…

gilles-peskine-arm commented Jan 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove a secret-dependent branch in Montgomery multiplication #3398

Remove a secret-dependent branch in Montgomery multiplication #3398

gilles-peskine-arm commented Jun 4, 2020

mpg commented Jun 5, 2020 •

edited

Loading

yanesca left a comment

mpg left a comment

mpg left a comment

gilles-peskine-arm commented Jun 8, 2020

mpg left a comment

mpg Jun 9, 2020

mpg Jun 9, 2020

mpg Jun 9, 2020

mpg Jun 9, 2020

mpg left a comment

gilles-peskine-arm Jan 18, 2021

Remove a secret-dependent branch in Montgomery multiplication #3398

Remove a secret-dependent branch in Montgomery multiplication #3398

Conversation

gilles-peskine-arm commented Jun 4, 2020

mpg commented Jun 5, 2020 • edited Loading

yanesca left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 8, 2020

mpg left a comment

Choose a reason for hiding this comment

mpg Jun 9, 2020

Choose a reason for hiding this comment

mpg Jun 9, 2020

Choose a reason for hiding this comment

mpg Jun 9, 2020

Choose a reason for hiding this comment

mpg Jun 9, 2020

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Jan 18, 2021

Choose a reason for hiding this comment

mpg commented Jun 5, 2020 •

edited

Loading