Backport 2.7: Remove a secret-dependent branch in Montgomery multiplication #3412
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
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]>
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]>
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]>
Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
bug
mbed TLS team
needs-review
mpg
approved these changes
Jun 9, 2020
yanesca
approved these changes
Jun 9, 2020
yanesca
added
approved
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.
Straightforward backport of #3398
This includes all patches from the original, including the parts that are not strictly necessary to fix the security issue but that make the code more readable and (a little) smaller and faster.
This is a rebase of the original. There were conflicts, but not meaningful ones:
CHECK_PARAMSin 2.7.i++in the carry propagation part ofmpi_sub_hlpwas dead code that was removed between 2.7 and 2.16. “Move carry propagation out of mpi_sub_hlp” removes it.