The opportunity for this change is more evident now that multiple variables are multiplied by the “special” value secp256k1_scalar_one.
Observing the calls, we notice that the variables were previously initialized to infinity via a call to secp256k1_gej_set_infinity().
secp256k1_gej_mul_scalar() handles the special case by also setting infinity in the result. This would happen every time, hence we can remove the calls.
Secp256k1 version 44c2452fd387 is the one that got into bitcoin v24.0 and v24.1.
It got there via https://github.com/bitcoin/bitcoin/commit/913b1f2a5eb2ee5cd02113791764b23ded4c1c94
Between secp256k1 0559fc6e41b6 and 44c2452fd387 the code that enabled the
various submodules got slightly changed.
This commit ports those changes to the frost module, too.
The upstream function secp256k1_ge_set_xo_var() in src/group_impl.h returns an
int signalling an error condition, but it is not marked with
SECP256K1_WARN_UNUSED_RESULT.
Thus, when compiling the Frost module, no warning was generated when
deserialize_frost_signature() had no check covering the case that
secp256k1_ge_set_xo_var() could fail.
The GCC static analyzer found two potential execution paths in
secp256k1_frost_verify() that would lead to undefined behaviour. For brevity,
the issues are not reported here, but only in the Github PR associated with
this commit (https://github.com/bancaditalia/secp256k1-frost/pull/5).
As of today (2023-05-29), in bitcoin-core secp256k1, secp256k1_ge_set_xo_var()
is still not marked SECP256K1_WARN_UNUSED_RESULT, see:
https://github.com/bitcoin-core/secp256k1/blob/908e02d596b66203788e8945b1f9c93ff28a4536/src/group_impl.h#L280
It may make sense to propose upstream to add that annotation.
In order to replicate the issues fixed by this commit, compile with:
./configure SECP_CFLAGS="-fanalyzer -fanalyzer-transitivity" --disable-tests --disable-exhaustive-tests --disable-benchmark --enable-experimental --enable-module-frost
The leak presented if compute_binding_factors() failed. In that case the error
reporting path would not deallocate the binding factor that were allocated just
above.
Found with gcc 13.1 via:
./configure SECP_CFLAGS="-fanalyzer -fanalyzer-transitivity" --disable-tests --disable-exhaustive-tests --disable-benchmark --enable-experimental --enable-module-frost
The analyzer found 3 leaks at main_impl.h:1403, all of them solved by this
change. The branch analysis of the first leak was:
```
In file included from src/secp256k1.c:767:
src/modules/frost/main_impl.h: In function 'secp256k1_frost_aggregate':
src/modules/frost/main_impl.h:1454:1: warning: leak of 'binding_factors.binding_factors' [CWE-401] [-Wanalyzer-malloc-leak]
1454 | }
| ^
'secp256k1_frost_aggregate': events 1-2
|
| 1365 | SECP256K1_API int secp256k1_frost_aggregate(const secp256k1_context *ctx,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to 'secp256k1_frost_aggregate'
[...]
| 1394 | binding_factors.binding_factors = (secp256k1_scalar *)
| 1395 | checked_malloc(&default_error_callback, num_signers * sizeof(secp256k1_scalar));
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (10) calling 'checked_malloc' from 'secp256k1_frost_aggregate'
|
+--> 'checked_malloc': events 11-15
|
|src/util.h:118:31:
| 118 | static SECP256K1_INLINE void *checked_malloc(const secp256k1_callback* cb, size_t size) {
| | ^~~~~~~~~~~~~~
| | |
| | (11) entry to 'checked_malloc'
| 119 | void *ret = malloc(size);
| | ~~~~~~~~~~~~
| | |
| | (12) allocated here
[...]
'secp256k1_frost_aggregate': events 29-30
|
| 1403 | if (compute_binding_factors(&binding_factors, msg32, 32, num_signers, commitments) == 0) {
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (29) ...to here
|......
| 1454 | }
| | ~
| | |
| | (30) 'binding_factors.binding_factors' leaks here; was allocated at (12)
|
```
In the next commit, this will allow us to call it from inside
secp256k1_frost_sign().
No functional changes.
In this way we have a single way of clearing binding factors across the code
base. Before this change, secp256k1_frost_sign() was the only function that
rolled its own code.
No functional changes.
A reference per the pkg-config sytax can be found in "man 5 pc", or online at
https://man.freebsd.org/cgi/man.cgi?query=pc&sektion=5&n=1
EXAMPLES
An example .pc file:
# This is a comment
prefix=/home/kaniini/pkg # this defines a variable
exec_prefix=${prefix} # defining another variable with a substitution
libdir=${exec_prefix}/lib
includedir=${prefix}/include
Name: libfoo # human-readable name
Description: an example library called libfoo # human-readable description
Version: 1.0
URL: http://www.pkgconf.org
Requires: libbar > 2.0.0
Conflicts: libbaz <= 3.0.0
Libs: -L${libdir} -lfoo
Libs.private: -lm
Cflags: -I${includedir}/libfoo
As per C89 standard (https://www.open-std.org/JTC1/sc22/wg14/www/docs/n1256.pdf,
section 6.5.3 "Unary operators") we are only required to parenthesize a sizeof()
call if the argument is a type name, which is not the case here.
However, it is less surprising to err on the side of caution, and uniformly
enclose in parentheses every sizeof() invocation across the code base.
No functional changes.
Given that - at least for now - we are allocating memory dynamically, let's free
it as soon as it is no longer needed.
Valgrind shows no regressions.
Remove rho_input as output variable of compute_binding_factor.
Currently, rho_input is not used outside compute_binding_factor, so we avoid
keeping unuseful data on the stack.
Note: the official test vectors of FROST by IETF include tests on rho_input.
Eventually, rho_input will be introduced again.
Two test runs are performed:
1. Build with coverage enabled and no -Werror; run tests; generate summary of
code coverage
2. Build with coverage disabled and using -Werror; run tests
Enabling converage translates in compiling additional code; the different
build configurations aim at checking that code changes do not introduce
regressions.
In order to save time on dependency installation the tests are performed
sequentially in a single job.
This mimics what has been done in itcoin-core at
https://github.com/bancaditalia/itcoin-core/commit/a6d27e63c8ed22694df293163e51a8e3e8748a13
Update official secp256k1 library taking bitcoin changes (from v23 to v24) and accordingly update the frost module integration. Pull request #3 introduced this merge commit.
The previous name was a leftover from a previous version. Now a single workflow
performs both:
- tests with -Werror and no coverage
- tests with coverage and no -Werror
As of current secp version, enabling coverage spits out some warnings, and
prevents us to use -Werror everywhere.
This CI workflow builds the minimum code necessary to run the Frost test suite
and the Frost example, and runs them under Valgrind, exiting if it detects any
problems.
The example is run first because it takes way less time.
Closes #19
Closes #20
In v0.3.0, secp256k1 also added support for building via CMake. Let's add a CI
workflow to exercise it.
Co-authored-by: Antonio Muci <antonio.muci@bancaditalia.it>
Closes #21
Closes #23
Closes #18
Closes #24
Workflow verify-version-consistency was the last one written, and already used
checkout@v4.
Let's align all the remaining workflows to use the most recent action version.
No functional changes.
This should be a very low risk change, since the only CI job that directly uses
the base image is .github/workflows/verify-version-consistency.yml, which only
depends on bash.
All the other workflows run in containers and should be unaffected; we will
update the base image in the next commit.
Installed software on the two platforms:
- https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md
- https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md
v0.3.0 slightly modified the way modules are enabled in `configure.ac`.
We integrated v0.3.0 into frost on 2023-11-21, with b701c4abaf0e, but we forgot
to make our `FROST_SPECIFIC` code in `configure.ac` conformant to the new
convention.
We do it now.
This is not a breaking change: both old and new version should still be correct.
inspired by:
- 654246c63585 refactor: take use of `secp256k1_scalar_{zero,one}` constants
- a1bd4971d6c6 refactor: take use of `secp256k1_scalar_{zero,one}` constants (part 2)
This gets rid of the local variable scalar_unit that was used only for that
purpose.
inspired by:
- 654246c63585 refactor: take use of `secp256k1_scalar_{zero,one}` constants
- a1bd4971d6c6 refactor: take use of `secp256k1_scalar_{zero,one}` constants (part 2)
This gets rid of the local variable scalar_unit that was used only for that
purpose.
inspired by:
- 654246c63585 refactor: take use of `secp256k1_scalar_{zero,one}` constants
- a1bd4971d6c6 refactor: take use of `secp256k1_scalar_{zero,one}` constants (part 2)
Before returning, secp256k1_frost_keygen_dkg_begin() clears private data in
order to reduce the risk of leaks.
This PR replaces secp256k1_scalar_set_int() with secp256k1_scalar_clear(). The
effect is almost identical, but we use a centralized function, with a clear
intent, and no additional free parameters.
Note for validating the change: the current implementation is in
src/scalar_low_impl.h and is the following:
SECP256K1_INLINE static void secp256k1_scalar_clear(secp256k1_scalar *r) { *r = 0; }
In that same file, immediately below, there is the implementation of
secp256k1_scalar_set_int():
SECP256K1_INLINE static void secp256k1_scalar_set_int(secp256k1_scalar *r, unsigned int v) { *r = v; }
The performance-optimized overloads are in src/scalar_4x64_impl.h and
src/scalar_8x32_impl.h. Their effect is the same, except for operating on 32 or
64 bits chunks.
For an analogous usage of secp256k1_scalar_clear(), see for example
secp256k1_schnorrsig_sign_internal() in src/modules/schnorrsig/main_impl.h.
The opportunity for this change is more evident now, that multiple variables are
multiplied by the "special" value secp256k1_scalar_one. Observing the calls, we
notice that the variables were previously initialized to infinity via a call to
secp256k1_gej_set_infinity().
secp256k1_gej_mul_scalar() handles the special case of infinity by also setting
infinity in the result. This would happen every time, hence we can remove the
calls.