Make size of output in mpi_mul_hlp() explicit #5701
Conversation
Signed-off-by: Hanno Becker <[email protected]>
The helper `mpi_mul_hlp()` performs a multiply-accumulate
operation `d += s * b`, where `d,b` are MPIs and `b` is a scalar.
Previously, only the length of `s` was specified, while `d` was
assumed to be 0-terminated of unspecified length.
This was leveraged at the end of the core multiplication steps
computingg the first `limb(s)` limbs of `d + s*b`: Namely, the
routine would keep on adding the current carry to `d` until none
was left. This can, in theory, run for an arbitrarily long time
if `d` has a tail of `0xFF`s, and hence the assumption of
`0`-termination.
This solution is both fragile and insecure -- the latter because
the carry-loop depends on the result of the multiplication.
This commit changes the signature of `mpi_mul_hlp()` to receive
the length of the output buffer, which must be greater or equal
to the length of the input buffer.
It is _not_ assumed that the output buffer is strictly larger
than the input buffer -- instead, the routine will simply return
any carry that's left. This will be useful in some applications
of this function. It is the responsibility of the caller to either
size the output appropriately so that no carry will be left, or
to handle the carry.
NOTE: The commit leaves the library in a state where it cannot
be compiled since the call-sites of mpi_mul_hlp() have
not yet been adjusted. This will be done in the subsequent
commits.
Signed-off-by: Hanno Becker <[email protected]>
The previous commit has changed the signature of mpi_mul_hlp(), making the length of the output explicit. This commit adjusts the call-site in mbedtls_mpi_mul_mpi() to this new signature. A notable change to the multiplication strategy had to be made: mbedtls_mpi_mul_mpi() performs a simple row-wise schoolbook multiplication, which however was so far computed iterating rows from top to bottom. This leads to the undesirable consequence that as lower rows are calculated and added to the temporary result, carry chains can grow. It is simpler and faster to iterate from bottom to top instead, as it is guaranteed that there will be no carry when adding the next row to the previous temporary result: The length of the output in each iteration can be fixed to len(B)+1. Signed-off-by: Hanno Becker <[email protected]>
A previous commit has changed the signature of mpi_mul_hlp(), making the length of the output explicit. This commit adjusts mbedtls_mpi_mul_int() to this change. Along the way, we make the code simpler and more secure by not calculating the minimal limb-size of A. A previous comment indicated that this was functionally necessary because of the implementation of mpi_mul_hlp() -- if it ever was, it isn't anymore. Signed-off-by: Hanno Becker <[email protected]>
A previous commit has changed the signature of mpi_mul_hlp, making the length of the output explicit. This commit adjusts mpi_montmul() accordingly. It also fixes a comment on the required size of the temporary value passed to mpi_montmul() (but does not change the call-sites). Signed-off-by: Hanno Becker <[email protected]>
hanno-becker
added
component-crypto
| (void) mpi_mul_hlp( d, n + 2, | ||
| N->p, n, | ||
| u1 ); | ||
| d++; | ||
| } | ||
|
|
||
| /* At this point, d is either the desired result or the desired result |
There was a problem hiding this comment.
The conditional subtraction can be simplified here to
borrow = mpi_sub_hlp( n, d, d, N->p );
borrow = ( d[n] < borrow );
(void) mpi_mul_hlp( d, n, N->p, n, borrow );
memcpy( A->p, d, n * ciL );which would be an example of a call to mpi_mul_hlp() where you expect and process the carry.
(@yanesca @mpg we touched on this approach in the chat, so FYI).
This paves the way for the helper to be used from the ECP module Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
|
This PR broke the CI because it calls To resolve, I removed the use of |
(In this case, there's nothing to do anyway since we only do a quasi-reduction to N+1 limbs) Signed-off-by: Hanno Becker <[email protected]>
library/bignum.c
Outdated
| mbedtls_mpi_uint c = 0; /* carry */ | ||
|
|
||
| /* Remember the excess of d over s for later */ | ||
| d_len -= s_len; |
There was a problem hiding this comment.
why not just use a separate variable for this, and name it more appropriately?
library/bignum.c
Outdated
| * \param i The number of limbs of \p s. | ||
| * \param[in,out] d The bignum to add to. | ||
| * \param d_len The number of limbs of \p d. This must be | ||
| * at least \p s_len. |
There was a problem hiding this comment.
@mpg does Mbed TLS have a mechanism for assert()ing requirements like this? or is it considered not worth it, even from a documentation-in-code perspective?
There was a problem hiding this comment.
From all I know, Mbed TLS does not yet have such a mechanism, though it would arguably be useful for debug builds.
There was a problem hiding this comment.
No, we don't assert(), for several reasons. One reason is that the code must be able to run in environments that don't have assert(), and we don't want to maintain yet another compilation option HAVE_ASSERT. Another is that we want to zeroize confidential data even if (especially if!) something goes wrong. So if a requirement is violated, the only recourse is to return an error status. But that is not possible in low-level bignum functions that return void.
So far our general approach in bignum is to have public functions that check requirements and return error codes (also those functions may resize objects), and low-level functions that don't check requirements and return void (and those functions may not resize objects).
There was a problem hiding this comment.
Another reason not to use assert is that it's typically disabled in production, which is where it's the most useful. If you want a debug-only assert, you can have extra code under MBEDTLS_TEST_HOOKS that should not change the execution of the function, but can mark the test as failed.
library/bignum.c
Outdated
| const mbedtls_mpi_uint *s, | ||
| mbedtls_mpi_uint *d, | ||
| mbedtls_mpi_uint b ) | ||
| mbedtls_mpi_uint mbedtls_mpi_core_mla( mbedtls_mpi_uint *d, size_t d_len , |
There was a problem hiding this comment.
trivial: space before comma
Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
library/ecp_curves.c
Outdated
| if( N->n < P255_WIDTH ) | ||
| /* Helper references for top part of N */ | ||
| mbedtls_mpi_uint * const NT_p = N->p + P255_WIDTH; | ||
| size_t const NT_n = N->n - P255_WIDTH; |
There was a problem hiding this comment.
trivial: if you're ever editing this file again, its way more common to put const first (this line and the one above)
There was a problem hiding this comment.
At least in the line above, moving const to the front would change the semantics to a non-const pointer to constant memory -- to avoid this distinction, I usually put the const last.
There was a problem hiding this comment.
If we were starting writing C in a vacuum, I'd agree that placing the const right next to the variable name is more consistent as it always means the same thing here - the variable is constant, regardless of whether it's a pointer or not. However, I agree with Tom that in the existing body of C already written, const size_t foo seems way more common. So, there's a conflict here, unfortunately, between your sensible argument, and existing usage.
There was a problem hiding this comment.
That's fine, I'll change it.
The variable is a local variable for the i386 bignum assembly only; introduce it as part of the start/finish macros. It can be noted that the variable is initialize to 0 within MULADDC_INIT, so there are no data dependencies across blocks of MULADDC_INIT/CORE/STOP. Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
The check was meant to precisely catch an underflow. Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
This reverts commit 808e666. Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
hanno-becker
force-pushed
the
mpi_mul_hlp
branch
from
2ede53d to
0dbf04a
Compare
|
Ok, I tried running Before: (branch here) After: (branch here) |
Removing the trimming has significant memory impact. While it is clearly what we want to do eventually for constant-time'ness, it should be fixed alongside a strategy to contain the ramifications on memory usage. Signed-off-by: Hanno Becker <[email protected]>
|
Updated results: This fixed the problem! |
mpg
added
approved
Context: The helper
mpi_mul_hlp()performs a multiply-accumulate operationd += s * b, whered,bare MPIs represented as plain integer arrays, andbis a scalar.Previously, only the length of
swas specified, whiledwas assumed to be 0-terminated of unspecified length.This was leveraged at the end of the core multiplication steps computing the first
limb(s)limbs ofd + s*b: Namely, the routine would keep on adding the current carry toduntil none was left. This can, in theory, run for an arbitrarily long time ifdhas a tail of0xFFs, and hence the assumption of0-termination.This solution is both fragile and insecure -- the latter because the carry-loop depends on the result of the multiplication.
Changes: This PR ...
mpi_mul_hlp()to receive the length of the output buffer, which must be greater or equal to the length of the input buffer.mbedtls_mpi_core_mla(), exposed inbignum_internal.hfor internal usagembedtls_mpi_core_mla()-- this was not originally intended to be done here, but turned out to be necessary since said reduction routine calledmbedtls_mpi_mul_int()with a stack-allocated MPI which caused difficulties with re-growing.Regarding the new signature of
mpi_mul_hlp(), note that it is not assumed that the output buffer is strictly larger than the input buffer -- instead, the routine will simply return any carry that's left. This will be useful in some applications of this function. It is the responsibility of the caller to either size the output appropriately so that no carry will be left, or to handle the carry.Note: This PR tries to be as minimal as possible. There are a number of instances where one is tempted to try to simplify call-sites of
mpi_mul_hlp()further, such asmbedtls_mpi_mul_int()or the conditional subtraction aftermpi_montmul(), but this is deliberately not tackled in this PR.