fuzz harness for SanitizeString and x-reference fixes #22171

pull baptistapedro wants to merge 1 commits into bitcoin:master from baptistapedro:fuzz-fix-SanitizeStr changing 1 files +90 −0

baptistapedro commented at 4:39 PM on June 6, 2021: none
Isolated harness for SanitizeString(), need a little edit to fit the fuzz coding pattern, I believe the extern "C" int LLVMFuzzerTestOneInput() would be translated to:
```
FUZZ_TARGET(sanitizestring)
{
    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    [...]
}
```
I am not that familiar with the whole fuzz codebase pattern yet to follow with the proper syntax.

The recommended fix from @laawnj is to " make the second argument of SanitizeString an "SafeChars" enum. This prevents a unhandled value from ever being passed in."

[1] and [2] point to so some references where the second argument is not passed as an enum but there is more cases under the same conditions so I recommend to do some x-reference to double check them.

Create sanitizestring.cpp

Isolated harness for SanitizeString(), need a little edit to fit the fuzz coding pattern, I believe the extern "C" int LLVMFuzzerTestOneInput() would be translated to:
```
FUZZ_TARGET(sanitizestring)
{
    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    [...]
}
I am not that familiar with the whole fuzz codebase pattern yet to follow with the proper syntax.

The recommended fix from @laawnj is to " make the second argument of SanitizeString an "SafeChars" enum. This prevents a unhandled value from ever being passed in.
"

[1] and [2] point to so some references where the second argument is not passed as an enum but there is more cases under the same conditions so I recommend to do some x-reference to double check them.

06fb6c5ce1

baptistapedro renamed this:
~~Create sanitizestring.cpp~~
fuzz harness for SanitizeString and x-reference fixes
on Jun 6, 2021
DrahtBot added the label Tests on Jun 6, 2021
in src/test/fuzz/sanitizestring.cpp:61 in 06fb6c5ce1
```
  56 | +extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
  57 | +{
  58 | +    std::string fuzzdata(reinterpret_cast<const char*>(data), size);
  59 | +    fuzzdata.push_back('\0');
  60 | +
  61 | +    SanitizeString(fuzzdata, fuzzdata.size());
```
MarcoFalke commented at 6:22 AM on June 7, 2021:

the second arg is an enum, not a size. See #6647 (review)
MarcoFalke commented at 6:25 AM on June 7, 2021:

Also, this method is already fuzzed in

src/test/fuzz/string.cpp: (void)SanitizeString(random_string_1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 3));
MarcoFalke commented at 6:30 AM on June 7, 2021:

See https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#quickstart-guide on how to compile. To run, you'd have to pass FUZZ=string ./src/test/fuzz/fuzz. Let me know if you have any questions.

baptistapedro commented at 9:13 AM on June 7, 2021:

I wrote this harness and reported to security@bitcoin without realizing you guys already wrote one However the second arg is passed as a size i.e. on [1] and [2] and other methods. See where SanitizeString is called and you can see different cases where the second arg is not an enum and thats why I referenced that the fix must be on the callers. This issue was already confirmed by laawnj and he also confirmed that SanitizeString()'s second arg must be called as an enum but it does not happen in some cases but such cases are not so reliable as attack vectors so is not a priority fix.

MarcoFalke commented at 11:44 AM on June 7, 2021:

I am not sure what [1] and [2] refer to, but if it was called anywhere with size, we could tell by existing tests crashing.

MarcoFalke commented at 11:46 AM on June 7, 2021:

The only places I could find where the second parameter is passed at all is URI handling and UA comment handling, both of which look fine.

MarcoFalke commented at 11:47 AM on June 7, 2021:

(Unrelated: Commit 26c06f24e5dcc32a7599abb8d670d22993c82bc2 removed the last use of FILENAME, so I guess that constant can be removed now)
MarcoFalke changes_requested
MarcoFalke commented at 6:22 AM on June 7, 2021: member

I haven't looked at this in detail, but it looks wrong from a first glance.
MarcoFalke commented at 2:10 PM on June 9, 2021: member

Closing for now, because we already have a fuzz target that covers this function. Other improvements and specially improvements of the fuzz tests are welcome.
MarcoFalke closed this on Jun 9, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

baptistapedro

MarcoFalke

Labels

Tests