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.

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

Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) #14242

Merged
merged 1 commit into from Feb 8, 2019

Conversation

Projects
None yet
7 participants
@practicalswift
Copy link
Member

commented Sep 17, 2018

Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...).

Background reading: memcpy (and friends) with NULL pointers

Steps to reproduce:

./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py
@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

Introduced in PR #11372 (119b0f8). Friendly ping @sipa – would you mind reviewing? :-)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

Shouldn't memory_cleanse be more robust in handling zero-length data?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

@MarcoFalke Sure! Added a commit. Please re-review :-)

Show resolved Hide resolved src/key_io.cpp
@promag

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

Shouldn't memory_cleanse be more robust in handling zero-length data?

@MarcoFalke the same for memset?

-0 ae17de5 because it changes memory_cleanse invariant.

IMO should avoid the call to memory_cleanse.

@practicalswift practicalswift force-pushed the practicalswift:ub-in-DecodeSecret branch Sep 18, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

@MarcoFalke I think @promag has a good point. Now reverted to the original version.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

@promag Thanks for reviewing. Now updated. Please re-review :-)

MarcoFalke added a commit that referenced this pull request Nov 5, 2018

Merge #14252: build: Run functional tests and benchmarks under the un…
…defined behaviour sanitizer (UBSan)

9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift)

Pull request description:

  Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan).

  This will make Travis automatically detect issues such as:
  * #14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`
  * #14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet)
  * #13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)`

  Addresses issue #14059.

Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Could remove the suppression from the file in contrib?

@bitcoin bitcoin deleted a comment from DrahtBot Nov 5, 2018

@practicalswift practicalswift force-pushed the practicalswift:ub-in-DecodeSecret branch Nov 6, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

@MarcoFalke Good point! Added commit 1cab4bb7e09c9d68e104460b67e5804848cd3c2e which removes UBSan suppression.

Please re-review :-)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14239 (Avoid dividing by zero (undefined behaviour) in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

Rebased!

@practicalswift practicalswift force-pushed the practicalswift:ub-in-DecodeSecret branch Nov 23, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 23, 2018

Show resolved Hide resolved src/key_io.cpp Outdated

@practicalswift practicalswift force-pushed the practicalswift:ub-in-DecodeSecret branch to 524a876 Jan 5, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

@luke-jr @promag @MarcoFalke Would you mind re-reviewing? :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@MarcoFalke Could this one get a release milestone? :-)

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Jan 15, 2019

Show resolved Hide resolved src/key_io.cpp Outdated

@laanwj laanwj added the Refactoring label Feb 6, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Agree with @laanwj, also (nit) could squash.

utACK 524a876 otherwise.

Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if …
…an invalid string is passed to DecodeSecret(...)

@practicalswift practicalswift force-pushed the practicalswift:ub-in-DecodeSecret branch from 524a876 to d855e4c Feb 7, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@laanwj @promag Good points. Fixed and squashed. Please re-review :-)

@promag
Copy link
Member

left a comment

utACK d855e4c.

@laanwj laanwj merged commit d855e4c into bitcoin:master Feb 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 8, 2019

Merge #14242: Avoid triggering undefined behaviour (std::memset(nullp…
…tr, 0, 0)) if an invalid string is passed to DecodeSecret(...)

d855e4c Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...) (practicalswift)

Pull request description:

  Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`.

  Background reading: [memcpy (and friends) with NULL pointers](https://www.imperialviolet.org/2016/06/26/nonnull.html)

  Steps to reproduce:

  ```
  ./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py
  ```

Tree-SHA512: b8325ced4f724d9c03065e0747af56b1f297a90d9fb09a24d46c3231a90dce3df6299f2c41f863b5cec18eaeded7b46ee4b93d9a52adc2541eb4c44d2c0965d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.