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

tests: Add fuzzing harness for strprintf(…) #18009

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 27, 2020

Add fuzzing harness for strprintf(…).

Update FuzzedDataProvider.h.

Avoid hitting some issues in tinyformat (reported upstreams in c42f/tinyformat#70).


Found issues in tinyformat:

Issue 1. The following causes a signed integer overflow followed by an allocation of 9 GB of RAM (or an OOM in memory constrained environments):

strprintf("%.777777700000000$", 1.0);

Issue 2. The following causes a stack overflow:

strprintf("%987654321000000:", 1);

Issue 3. The following causes a stack overflow:

strprintf("%1$*1$*", -11111111);

Issue 4. The following causes a NULL pointer dereference:

strprintf("%.1s", (char *)nullptr);

Issue 5. The following causes a float cast overflow:

strprintf("%c", -1000.0);

Issue 6. The following causes a float cast overflow followed by an invalid integer negation:

strprintf("%*", std::numeric_limits<double>::lowest());

@maflcko
Copy link
Member

maflcko commented Jan 27, 2020

When updating FuzzedDataProvider, please include the exact commit id that it was updated to

@practicalswift
Copy link
Contributor Author

@MarcoFalke Good point! Done! :)

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK

nit: in the commit that bumps the provider: Could limit the commit subject to a reasonable length, i.e. move the url to the commit body

src/test/fuzz/strprintf.cpp Show resolved Hide resolved
src/test/fuzz/strprintf.cpp Outdated Show resolved Hide resolved
src/test/fuzz/strprintf.cpp Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor Author

@MarcoFalke Thanks for reviewing! Feedback addressed. Please re-review :)

Copy link

@chamajcpradel chamajcpradel left a comment

Choose a reason for hiding this comment

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

Sirs. This is very good test.

@Empact
Copy link
Member

Empact commented Jan 30, 2020

Would be helpful to comment on "why" in each commit message - e.g. I was curious to look into why the FuzzedDataProvider.h update was necessary and found that the ConsumeFloatingPoint was introduced recently. https://chris.beams.io/posts/git-commit/#why-not-how

maflcko pushed a commit that referenced this pull request Jan 30, 2020
cc668d0 tests: Add fuzzing harness for strprintf(...) (practicalswift)
ccc3c76 tests: Add fuzzer strprintf to FUZZERS_MISSING_CORPORA (temporarily) (practicalswift)
6ef0491 tests: Update FuzzedDataProvider.h from upstream (LLVM) (practicalswift)

Pull request description:

  Add fuzzing harness for `strprintf(…)`.

  Update `FuzzedDataProvider.h`.

  Avoid hitting some issues in tinyformat (reported upstreams in c42f/tinyformat#70).

  ---

  Found issues in tinyformat:

  **Issue 1.** The following causes a signed integer overflow followed by an allocation of 9 GB of RAM (or an OOM in memory constrained environments):

  ```
  strprintf("%.777777700000000$", 1.0);
  ```

  **Issue 2.** The following causes a stack overflow:

  ```
  strprintf("%987654321000000:", 1);
  ```

  **Issue 3.** The following causes a stack overflow:

  ```
  strprintf("%1$*1$*", -11111111);
  ```

  **Issue 4.** The following causes a `NULL` pointer dereference:

  ```
  strprintf("%.1s", (char *)nullptr);
  ```

  **Issue 5.** The following causes a float cast overflow:

  ```
  strprintf("%c", -1000.0);
  ```

  **Issue 6.** The following causes a float cast overflow followed by an invalid integer negation:

  ```
  strprintf("%*", std::numeric_limits<double>::lowest());
  ```

Top commit has no ACKs.

Tree-SHA512: 9b765559281470f4983eb5aeca94bab1b15ec9837c0ee01a20f4348e9335e4ee4e4fecbd7a1a5a8ac96aabe0f9eeb597b8fc9a2c8faf1bab386e8225d5cdbc18
@maflcko maflcko merged commit cc668d0 into bitcoin:master Jan 30, 2020
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 20, 2020
Summary:
```
Add fuzzing harness for strprintf(…).

Update FuzzedDataProvider.h.

Avoid hitting some issues in tinyformat (reported upstreams in
c42f/tinyformat#70).
```

Backport of core [[bitcoin/bitcoin#18009 | PR18009]].

Test Plan:
  ninja bitcoin-fuzzers
  ./src/test/fuzz/strprintf

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8004
@practicalswift practicalswift deleted the fuzzers-strprintf branch April 10, 2021 19:39
kwvg added a commit to kwvg/dash that referenced this pull request Feb 27, 2022
kwvg added a commit to kwvg/dash that referenced this pull request Feb 27, 2022
kwvg added a commit to kwvg/dash that referenced this pull request Feb 28, 2022
kwvg added a commit to kwvg/dash that referenced this pull request Feb 28, 2022
kwvg added a commit to kwvg/dash that referenced this pull request Feb 28, 2022
kwvg added a commit to kwvg/dash that referenced this pull request Mar 13, 2022
kwvg added a commit to kwvg/dash that referenced this pull request Mar 24, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants