-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove a secret-dependent branch in Montgomery multiplication #3398
Remove a secret-dependent branch in Montgomery multiplication #3398
Conversation
Signed-off-by: Gilles Peskine <[email protected]>
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 <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
50b5719
to
d55bfe9
Compare
From the CI: MSVC 2013 would like to be sure that you know what you're doing with one cast:
The comment above states that the value being cast is always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of questions and a minor suggestion for improvement.
Let code analyzers know that this is deliberate. For example MSVC warns about the conversion if it's implicit. Signed-off-by: Gilles Peskine <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just releasing comments so far because otherwise I can't participate in the conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this PR, except for one place where I think the comments need to be improved.
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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"” . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the code now, but found a few issues in some comments.
library/bignum.c
Outdated
* \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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the second part, I was a bit surprised check-doxy-blocks
didn't complain. I checked the CI results, and it did complain.
library/bignum.c
Outdated
/* 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean d - N
here.
library/bignum.c
Outdated
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 ); */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it your intention to leave this code commented, or did you mean to remove it?
Signed-off-by: Gilles Peskine <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the comments. Looks all good to me now.
Merge pull request ARMmbed#3398 from gilles-peskine-arm/montmul-cmp-b…
MPI_VALIDATE_RET( X != NULL ); | ||
MPI_VALIDATE_RET( A != NULL ); | ||
MPI_VALIDATE_RET( B != NULL ); | ||
|
||
if( mbedtls_mpi_cmp_abs( A, B ) < 0 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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.