Flags for constant-time masking rely
on the values being exactly 0 or 1 rather
than 0 or true. Add VERIFY_CHECKs to enforce
in VERIFY builds as a preventative
measure and add documentation where relevant.
secp256k1_memclear has the side effect of undefining bytes for
valgrind checks. In some cases, we may want to zero bytes
but allow subsequent reads. So we split memclear into
memclear_explicit, which makes no guarantees about the content
of the buffer on return, and memzero_explicit, which guarantees
zero value on return.
Change the memset in partial_sign to use memzero_explicit.
There are two uses of the secp256k1_fe_clear() function that are now separated
into these two functions in order to reflect the intent:
1) initializing the memory prior to being used -> converted to fe_set_int( . , 0 )
2) zeroing the memory after being used such that no sensitive data remains. ->
remains as fe_clear()
In the latter case, 'magnitude' and 'normalized' need to be overwritten when
VERIFY is enabled.
Co-Authored-By: isle2983 <isle2983@yahoo.com>
By providing an uppercase variant of these verification functions, it is
better visible that it is test code and surrounding `#ifdef VERIFY`
blocks can be removed (if there is no other code around that could
remain in production mode), as they don't serve their purpose any more.
At some places intentional blank lines are inserted for grouping and
better readadbility.
Now that the `VERIFY_CHECK` compiles to empty in non-VERIFY mode, blocks
that only consist of these macros don't need surrounding `#ifdef VERIFY`
conditions anymore.
At some places intentional blank lines are inserted for grouping and
better readadbility.
Also update the operations count comments in each of the affected
functions accordingly and remove a redundant VERIFY_CHECK in
secp256k1_gej_add_ge (the infinity value range check [0,1] is already
covered by secp256k1_gej_verify above).
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Remove also the explicit magnitude restriction `a->x.magnitude <= 31`
in `secp256k1_gej_eq_x_var` (introduced in commit
07c0e8b82e), as this is implied by the
new limits.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f20266722a
(PR #1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba561fdb9d57a2cc9842ce041d9ba29a6189 of WIP-PR #1032 (which is
obviously not rebased on #1299 yet).
Also, for easier review, all functions handling group elements are
structured in the following wasy for easier review (idea suggested by
Tim Ruffing):
- on entry, verify all input ge, gej (and fe)
- empty line
- actual function body
- empty line
- on exit, verify all output ge, gej
Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
07c0e8b82e group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var` (Sebastian Falbesoner)
efa76c4bf7 group: remove unneeded normalize_weak in `secp256k1_ge_is_valid_var` (Sebastian Falbesoner)
Pull request description:
This PR removes unneeded normalize_weak calls in two group element functions:
* `secp256k1_ge_is_valid_var`: After calculating the right-hand side of the elliptic curve equation (x^3 + 7), the field element `x3` has a magnitude of 2 (1 as result of `secp256k1_fe_mul`, then increased by 1 due to `secp256k1_fe_add_int`). This is fine for `secp256k1_fe_equal_var`, as the second parameter only requires the magnitude to not exceed 31, and the normalize_weak call is hence not needed and can be dropped. Note that the interface description for `secp256k1_fe_equal` (which also applies to `secp256k1_fe_equal_var`) once stated that _both_ parameters need to have magnitude 1, but that was corrected in commit 7d7d43c6dd.
* `secp256k1_gej_eq_x_var`: By requiring that the input group element's X coordinate (`a->x`) has a magnitude of <= 31, the normalize_weak call and also the field element variable `r2` are not needed anymore and hence can be dropped.
ACKs for top commit:
sipa:
utACK 07c0e8b82e
jonasnick:
ACK 07c0e8b82e
Tree-SHA512: 9037e4af881ce7bf3347414d6da06b99e3d318733ba4f70e8b24d2320c2f26d022144e17bd6b95c1a4ef1be3825a4464e56ce2d2b3ae7bbced04257048832b7f
By requiring that the input group element's X coordinate (`a->x`) has a
magnitude of <= 31, the normalize_weak call and also the field element
variable `r2` are not needed anymore and hence can be dropped.
After calculating the right-hand side of the elliptic curve equation
(x^3 + 7), the field element `x3` has a magnitude of 2 (1 as result of
`secp256k1_fe_mul`, then increased by 1 due to `secp256k1_fe_add_int`).
This is fine for `secp256k1_fe_equal_var`, as the second parameter only
requires the magnitude to not exceed 31, and the normalize_weak call can
hence be dropped.
e089eecc1e group: Further simply gej_add_ge (Tim Ruffing)
ac71020ebe group: Save a normalize_to_zero in gej_add_ge (Tim Ruffing)
Pull request description:
As discovered by sipa in #1033.
See commit message for reasoning but note that the infinity handling will be replaced in the second commit again.
ACKs for top commit:
sipa:
ACK e089eecc1e
apoelstra:
ACK e089eecc1e
Tree-SHA512: fb1b5742e73dd8b2172b4d3e2852490cfd626e8673b72274d281fa34b04e9368a186895fb9cd232429c22b14011df136f4c09bdc7332beef2b3657f7f2798d66
The code currently switches to the alternative formula for lambda only if (R,M)
= (0,0) but the alternative formula works whenever M = 0: Specifically, M = 0
implies y1 = -y2. If x1 = x2, then a = -b this is the r = infinity case that we
handle separately. If x1 != x2, then the denominator in the alternative formula
is non-zero, so this formula is well-defined.
One needs to carefully check that the infinity assignment is still correct
because now the definition of m_alt at this point in the code has changed. But
this is true:
Case y1 = -y2:
Then degenerate = true and infinity = ((x1 - x2)Z == 0) & ~a->infinity .
a->infinity is handled separately.
And if ~a->infinity, then Z = Z1 != 0,
so infinity = (x1 - x2 == 0) = (a == -b) by case condition.
Case y1 != -y2:
Then degenerate = false and infinity = ((y1 + y2)Z == 0) & ~a->infinity .
a->infinity is handled separately.
And if ~a->infinity, then Z = Z1 != 0,
so infinity = (y1 + y2 == 0) = false by case condition.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
