Skip to content

Commit

Permalink
[SECP256K1] Prevent arithmetic on NULL pointer if the scratch space i…
Browse files Browse the repository at this point in the history
…s too small

Summary:
```
If the scratch space is too small when calling
`secp256k1_ecmult_strauss_batch()`, the `state.pre_a` allocation will
fail and the pointer will be `NULL`. This causes `state.pre_a_lam` to be
computed from the `NULL` pointer.

It is also possible that the first allocation to fail is for `state.ps`,
which will cause the failure to occur when in
`secp256k1_ecmult_strauss_wnaf()`.

The issue has been detected by UBSAN using Clang 10:

CC=clang \
CFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" \
LDFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" \
../configure

UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1 make check
```

Backport of secp256k1 [[bitcoin-core/secp256k1#839 | PR839]].

Test Plan:
With Clang and UBSAN:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8265
  • Loading branch information
Fabcien committed Nov 4, 2020
1 parent 34e2cee commit fb908c8
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 3 deletions.
3 changes: 3 additions & 0 deletions src/secp256k1/.travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ env:
- AUTOTOOLS_EXTRA_FLAGS=CPPFLAGS=-DDETERMINISTIC CMAKE_EXTRA_FLAGS=-DCMAKE_C_FLAGS=-DDETERMINISTIC
- AUTOTOOLS_EXTRA_FLAGS=CFLAGS=-O0 CMAKE_EXTRA_FLAGS=-DCMAKE_BUILD_TYPE=Debug CTIMETEST=no
- AUTOTOOLS_TARGET=check-java CMAKE_TARGET=check-secp256k1-java JNI=yes ECDH=yes EXPERIMENTAL=yes WITH_VALGRIND=no CTIMETEST=no BENCH=no
- CFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" LDFLAGS="-fsanitize=undefined -fno-omit-frame-pointer"
UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1"
BIGNUM=no ASM=x86_64 ECDH=yes RECOVERY=yes EXPERIMENTAL=yes MULTISET=yes SCHNORRSIG=yes CTIMETEST=no
- ECMULTGENPRECISION=2
- ECMULTGENPRECISION=8
- RUN_VALGRIND=yes
Expand Down
6 changes: 3 additions & 3 deletions src/secp256k1/src/ecmult_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -595,11 +595,11 @@ static int secp256k1_ecmult_strauss_batch(const secp256k1_callback* error_callba
scalars = (secp256k1_scalar*)secp256k1_scratch_alloc(error_callback, scratch, n_points * sizeof(secp256k1_scalar));
state.prej = (secp256k1_gej*)secp256k1_scratch_alloc(error_callback, scratch, n_points * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_gej));
state.zr = (secp256k1_fe*)secp256k1_scratch_alloc(error_callback, scratch, n_points * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_fe));
state.pre_a = (secp256k1_ge*)secp256k1_scratch_alloc(error_callback, scratch, n_points * 2 * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_ge));
state.pre_a_lam = state.pre_a + n_points * ECMULT_TABLE_SIZE(WINDOW_A);
state.pre_a = (secp256k1_ge*)secp256k1_scratch_alloc(error_callback, scratch, n_points * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_ge));
state.pre_a_lam = (secp256k1_ge*)secp256k1_scratch_alloc(error_callback, scratch, n_points * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_ge));
state.ps = (struct secp256k1_strauss_point_state*)secp256k1_scratch_alloc(error_callback, scratch, n_points * sizeof(struct secp256k1_strauss_point_state));

if (points == NULL || scalars == NULL || state.prej == NULL || state.zr == NULL || state.pre_a == NULL) {
if (points == NULL || scalars == NULL || state.prej == NULL || state.zr == NULL || state.pre_a == NULL || state.pre_a_lam == NULL || state.ps == NULL) {
secp256k1_scratch_apply_checkpoint(error_callback, scratch, scratch_checkpoint);
return 0;
}
Expand Down

0 comments on commit fb908c8

Please sign in to comment.