fuzz: fix key size in `crypter` #30373

brunoerg commented at 4:37 PM on July 1, 2024: contributor

Fixes #30251

This PR:

Limits cipher_text_ed and random_string (SecureString) size.
Replace ConsumeRandomLengthByteVector for keys to ConsumeFixedLengthByteVector with WALLET_CRYPTO_KEY_SIZE.
Replace ConsumeRandomLengthByteVector for chSalt to ConsumeFixedLengthByteVector with WALLET_CRYPTO_SALT_SIZE.

DrahtBot commented at 4:37 PM on July 1, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	marcofleon, dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jul 1, 2024

brunoerg commented at 5:50 PM on July 1, 2024: contributor

CI failure is unrelated to this PR. It's the same from https://github.com/bitcoin/bitcoin/issues/30368

brunoerg force-pushed on Jul 2, 2024

brunoerg commented at 12:53 PM on July 2, 2024: contributor

Rebased to fix CI

marcofleon commented at 2:44 PM on July 2, 2024: contributor

I'm still getting an error with this patch.

FUZZ=crypter src/test/fuzz/fuzz crash-fee5221434486fefe1ec12410e7546625f82823a 
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 4226801841
INFO: Loaded 1 modules   (378324 inline 8-bit counters): 378324 [0x556f6632fb80, 0x556f6638c154), 
INFO: Loaded 1 PC tables (378324 PCs): 378324 [0x556f6638c158,0x556f66951e98), 
src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: crash-fee5221434486fefe1ec12410e7546625f82823a
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
==83550== ERROR: libFuzzer: deadly signal
    [#0](/bitcoin-bitcoin/0/) 0x556f65081ab4 in __sanitizer_print_stack_trace (/root/bitcoin/src/test/fuzz/fuzz+0x99dab4) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#1](/bitcoin-bitcoin/1/) 0x556f650579e8 in fuzzer::PrintStackTrace() (/root/bitcoin/src/test/fuzz/fuzz+0x9739e8) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#2](/bitcoin-bitcoin/2/) 0x556f6503de03 in fuzzer::Fuzzer::CrashCallback() (/root/bitcoin/src/test/fuzz/fuzz+0x959e03) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#3](/bitcoin-bitcoin/3/) 0x7fa770cdc04f  (/lib/x86_64-linux-gnu/libc.so.6+0x3c04f) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
    [#4](/bitcoin-bitcoin/4/) 0x7fa770d2ae2b  (/lib/x86_64-linux-gnu/libc.so.6+0x8ae2b) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
    [#5](/bitcoin-bitcoin/5/) 0x7fa770cdbfb1 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x3bfb1) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
    [#6](/bitcoin-bitcoin/6/) 0x7fa770cc6471 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x26471) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
    [#7](/bitcoin-bitcoin/7/) 0x7fa77109d918  (/lib/x86_64-linux-gnu/libstdc++.so.6+0x9d918) (BuildId: 0c47cec75226c7736517d5acb61e373d541a5023)
    [#8](/bitcoin-bitcoin/8/) 0x7fa7710a8e19  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa8e19) (BuildId: 0c47cec75226c7736517d5acb61e373d541a5023)
    [#9](/bitcoin-bitcoin/9/) 0x7fa7710a8e84 in std::terminate() (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa8e84) (BuildId: 0c47cec75226c7736517d5acb61e373d541a5023)
    [#10](/bitcoin-bitcoin/10/) 0x7fa7710a90d7 in __cxa_throw (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa90d7) (BuildId: 0c47cec75226c7736517d5acb61e373d541a5023)
    [#11](/bitcoin-bitcoin/11/) 0x556f65098105 in secure_allocator<unsigned char>::allocate(unsigned long) src/./support/allocators/secure.h:31:13
    [#12](/bitcoin-bitcoin/12/) 0x556f650967bd in std::allocator_traits<secure_allocator<unsigned char>>::allocate(secure_allocator<unsigned char>&, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/alloc_traits.h:318:20
    [#13](/bitcoin-bitcoin/13/) 0x556f650967bd in std::_Vector_base<unsigned char, secure_allocator<unsigned char>>::_M_allocate(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_vector.h:378:20
    [#14](/bitcoin-bitcoin/14/) 0x556f650967bd in void std::vector<unsigned char, secure_allocator<unsigned char>>::_M_range_initialize<__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char>>>>(__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char>>>, __gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char>>>, std::forward_iterator_tag) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_vector.h:1687:14
    [#15](/bitcoin-bitcoin/15/) 0x556f650967bd in std::vector<unsigned char, secure_allocator<unsigned char>>::vector<__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char>>>, void>(__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char>>>, __gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char>>>, secure_allocator<unsigned char> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_vector.h:706:4
    [#16](/bitcoin-bitcoin/16/) 0x556f650967bd in wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_7::operator()() const src/wallet/test/fuzz/crypter.cpp:73:39
    [#17](/bitcoin-bitcoin/17/) 0x556f650967bd in unsigned long CallOneOf<wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_0, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_1, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_2, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_3, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_4, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_5, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_6, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_7, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_8>(FuzzedDataProvider&, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_0, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_1, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_2, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_3, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_4, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_5, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_6, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_7, wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>)::$_8) src/./test/fuzz/util.h:42:27
    [#18](/bitcoin-bitcoin/18/) 0x556f650967bd in wallet::(anonymous namespace)::crypter_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) src/wallet/test/fuzz/crypter.cpp:34:9
    [#19](/bitcoin-bitcoin/19/) 0x556f6539bfd7 in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:591:9
    [#20](/bitcoin-bitcoin/20/) 0x556f6539bfd7 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:202:5
    [#21](/bitcoin-bitcoin/21/) 0x556f6503f210 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/bitcoin/src/test/fuzz/fuzz+0x95b210) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#22](/bitcoin-bitcoin/22/) 0x556f65029502 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/bitcoin/src/test/fuzz/fuzz+0x945502) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#23](/bitcoin-bitcoin/23/) 0x556f6502edca in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/bitcoin/src/test/fuzz/fuzz+0x94adca) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#24](/bitcoin-bitcoin/24/) 0x556f650582b2 in main (/root/bitcoin/src/test/fuzz/fuzz+0x9742b2) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)
    [#25](/bitcoin-bitcoin/25/) 0x7fa770cc7249  (/lib/x86_64-linux-gnu/libc.so.6+0x27249) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
    [#26](/bitcoin-bitcoin/26/) 0x7fa770cc7304 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x27304) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
    [#27](/bitcoin-bitcoin/27/) 0x556f650242f0 in _start (/root/bitcoin/src/test/fuzz/fuzz+0x9402f0) (BuildId: 8a648100d03cea9c2eb716b3fa93582a652e4d80)

I was able to reproduce this on a remote machine as well. Here is the crash file: cryptercrash.txt

brunoerg commented at 3:01 PM on July 2, 2024: contributor

Yes, just noticed more places need to be changed, working on it.

brunoerg force-pushed on Jul 2, 2024

brunoerg renamed this:
~~fuzz: fix ciphertext size in `crypter`~~
fuzz: fix key size in `crypter`
on Jul 2, 2024

brunoerg commented at 4:05 PM on July 2, 2024: contributor

Force-pushed changing more sizes, I updated the PR description with the changes.

in src/wallet/test/fuzz/crypter.cpp:51 in 9d2e2d8fc0 outdated

  50 |              },
  51 |              [&] {
  52 | -                const std::vector<unsigned char> random_vector = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32);
  53 | +                const std::vector<unsigned char> random_vector = ConsumeFixedLengthByteVector(fuzzed_data_provider, WALLET_CRYPTO_KEY_SIZE);
  54 |                  const CKeyingMaterial new_key(random_vector.begin(), random_vector.end());
  55 |                  const std::vector<unsigned char>& new_IV = ConsumeFixedLengthByteVector(fuzzed_data_provider, 16);

marcofleon commented at 1:35 PM on July 4, 2024:

If we're using constants from crypter.h, then this can be changed from 16 to WALLET_CRYPTO_IV_SIZE for consistency.

brunoerg commented at 1:37 PM on July 4, 2024:

Sure, I'll address it.

in src/wallet/test/fuzz/crypter.cpp:37 in 9d2e2d8fc0 outdated

  34 |      {
  35 |          CallOneOf(
  36 |              fuzzed_data_provider,
  37 |              [&] {
  38 | -                const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString();
  39 | +                const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString(100);

marcofleon commented at 1:36 PM on July 4, 2024:

Is there a specifc reason for 100 here or is it not that important?

brunoerg commented at 1:38 PM on July 4, 2024:

I used the size we reserve in the RPC (e.g. walletpassphrasechange) for this as reference.

brunoerg force-pushed on Jul 4, 2024

brunoerg commented at 1:40 PM on July 4, 2024: contributor

Force-pushed addressing #30373 (review). Thanks, @marcofleon

marcofleon commented at 2:12 PM on July 4, 2024: contributor

It still crashes with the same bad_alloc error. But changing this line https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/wallet/test/fuzz/crypter.cpp#L85 to ConsumeRandomLengthByteVector(fuzzed_data_provider, 64) fixes it for me. I thnk this is fine because crypted_secret ends up playing the same role as cipher_text_ed does in Decrypt.

fuzz: fix key size in crypter target

Set a max length for some previous
`ConsumeRandomLengthByteVector` usage.

4383dc90ba

brunoerg commented at 2:34 PM on July 4, 2024: contributor

to ConsumeRandomLengthByteVector(fuzzed_data_provider, 64) fixes it for me. I thnk this is fine because crypted_secret ends up playing the same role as cipher_text_ed does in Decrypt.

Nice, I missed this but surely it's fine to limit it as well. Gonna change it.

brunoerg force-pushed on Jul 4, 2024

brunoerg commented at 2:36 PM on July 4, 2024: contributor

Force-pushed addressing #30373#pullrequestreview-2159032605

Now, we basically got rid of ConsumeRandomLengthByteVector with no specified max_length.

marcofleon approved

marcofleon commented at 8:58 PM on July 5, 2024: contributor

Tested ACK 4383dc90bac1b5def73352fe222f99807d8ca4dd. I ran this:

FUZZ=crypter src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 ../qa-assets/fuzz_seed_corpus/crypter ../qa-assets/fuzz_seed_corpus/*

It successfully merges in 1 attempt with no errors.

dergoegge approved

dergoegge commented at 10:02 AM on July 15, 2024: member

utACK 4383dc90bac1b5def73352fe222f99807d8ca4dd

fanquake merged this on Jul 15, 2024

fanquake closed this on Jul 15, 2024

brunoerg deleted the branch on Jul 15, 2024

bitcoin locked this on Jul 15, 2025