Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent arithmetic on NULL pointer if the scratch space is too small #839

Merged
merged 2 commits into from Nov 4, 2020
Merged

Prevent arithmetic on NULL pointer if the scratch space is too small #839

merged 2 commits into from Nov 4, 2020

Conversation

Fabcien
Copy link
Contributor

@Fabcien Fabcien commented Oct 26, 2020

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

@real-or-random
Copy link
Contributor

real-or-random commented Oct 26, 2020 •
edited

Thanks for the PR!

Before I have to time a real reply, let me note for anyone reading here that this is not something to worry about because scratch spaces are used nowhere in the public API currently. This is dead code used for future extensions.

real-or-random
real-or-random reviewed Oct 26, 2020
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/ecmult_impl.h Outdated Show resolved Hide resolved
.travis.yml Outdated
@@ -31,6 +31,7 @@ env:
- BUILD=distcheck WITH_VALGRIND=no CTIMETEST=no BENCH=no
- CPPFLAGS=-DDETERMINISTIC
- CFLAGS=-O0 CTIMETEST=no
- CFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" LDFLAGS="-fsanitize=undefined -fno-omit-frame-pointer" UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" CTIMETEST=no
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should go to a different PR (if we want this at all). Not sure how long this takes on Travis, and it may not run on the GCC version on Travis etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the result on Travis: https://travis-ci.org/github/Fabcien/secp256k1-1/builds/738981454
UBSAN has a low overhead so the duration is only slightly impacted.

@real-or-random real-or-random mentioned this pull request Oct 26, 2020
Closed
@Fabcien Fabcien force-pushed the fix_ubsan_ecmult branch 2 times, most recently from 2fca70c to 61dffd1 Compare October 26, 2020 16:01
@Fabcien
Copy link
Contributor Author

Fabcien commented Oct 26, 2020

@real-or-random Now calling secp256k1_scratch_alloc for state.pre_a_lam.
Let me know if you still want me to move the Travis change to its own PR. I personally prefer to keep the changes and the tests together.

@real-or-random
Copy link
Contributor

real-or-random commented Oct 26, 2020 •
edited

I'm ok with adding it here (can't speak for the others) but I think then it should look more like this line then (enable all the stuff)
BIGNUM=no ASM=x86_64 ECDH=yes RECOVERY=yes EXPERIMENTAL=yes SCHNORRSIG=yes

And should we add print_summary=1? By the way, I can't find official docs on UBSAN_OPTIONS. Do you have a good resource?

@Fabcien
Copy link
Contributor Author

Fabcien commented Oct 26, 2020

The best resource I could find is the documentation from LLVM. The GCC documentation explains the halt_on_error behavior.
I didn't know the print_summary option but it seems to be useless with halt_on_error=1, which needs to be set for the build to fail if UBSAN finds an issue.

@sipa
Copy link
Contributor

sipa commented Oct 27, 2020 •
edited

Concept ACK.

I think CI tests with -fsanitize=undefined is a good idea in any case. Can you do that in a separate commit though? It's an unrelated change.

@Fabcien
Prevent arithmetic on NULL pointer if the scratch space is too small
7506e06 
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
```
@Fabcien
Run the undefined behaviour sanitizer on Travis
29a299e 
Run UBSAN with both GCC and Clang, on Linux and macOS.
The `halt_on_error=1` option is required to make the build fail if the
sanitizer finds an issue.
@Fabcien
Copy link
Contributor Author

Fabcien commented Oct 27, 2020

@sipa Sure, done.

@sipa
Copy link
Contributor

sipa commented Oct 27, 2020

ACK 29a299e. Reviewed the code changes and verified that building with these sanitizer flags catches the existing error, as well as a signed integer overflow if introduced.

@sipa
Copy link
Contributor

sipa commented Oct 28, 2020

There is an issue with Travis' s390x build, which I've cancelled. CI succeeded for all other builds.

@real-or-random
Copy link
Contributor

ACK 29a299e code inspection

jonasnick
jonasnick approved these changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 29a299e

@jonasnick jonasnick merged commit d0a83f7 into bitcoin-core:master Nov 4, 2020
@real-or-random real-or-random mentioned this pull request Nov 4, 2020
Open
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 4, 2020
@Fabcien
[SECP256K1] Prevent arithmetic on NULL pointer if the scratch space i…
fb908c8 
…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
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Nov 5, 2020
@Fabcien
[SECP256K1] Prevent arithmetic on NULL pointer if the scratch space i…
533268e 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@real-or-random real-or-random real-or-random approved these changes

@jonasnick jonasnick jonasnick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Fabcien @real-or-random @sipa @jonasnick