fuzz: wallet: fix crypter target #32118

brunoerg commented at 9:37 PM on March 21, 2025: contributor

The crypter target has an issue, it's calling DecryptKey with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the DecryptSecret call since this function is already (and only) called within DecryptKey.

fuzz: wallet: fix crypter target 28dc118001

DrahtBot commented at 9:38 PM on March 21, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32118.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko

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 Mar 21, 2025

in src/wallet/test/fuzz/crypter.cpp:23 in 28dc118001

  19 | @@ -20,6 +20,7 @@ void initialize_crypter()
  20 |  
  21 |  FUZZ_TARGET(crypter, .init = initialize_crypter)
  22 |  {
  23 | +    SeedRandomStateForTest(SeedRand::ZEROS);

maflcko commented at 8:07 AM on March 22, 2025:

Why is this needed?

brunoerg commented at 9:48 PM on March 22, 2025:

Without it I get:

[#47708](/bitcoin-bitcoin/47708/)	REDUCE cov: 475 ft: 1809 corp: 217/12024b lim: 98 exec/s: 3669 rss: 79Mb L: 77/98 MS: 2 PersAutoDict-EraseBytes- DE: "\034\000"-


The current fuzz target used the global random state.

This is acceptable, but requires the fuzz target to call
SeedRandomStateForTest(SeedRand::ZEROS) in the first line
of the FUZZ_TARGET function.

An alternative solution would be to avoid any use of globals.

Without a solution, fuzz instability and non-determinism can lead
to non-reproducible bugs or inefficient fuzzing.


==37661== ERROR: libFuzzer: deadly signal
    [#0](/bitcoin-bitcoin/0/) 0x0001071d358c in __sanitizer_print_stack_trace+0x14 (libclang_rt.ubsan_osx_dynamic.dylib:arm64+0xb58c)
    [#1](/bitcoin-bitcoin/1/) 0x0001059d8910 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
    [#2](/bitcoin-bitcoin/2/) 0x0001059beba8 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:231
    [#3](/bitcoin-bitcoin/3/) 0x000183ee1a20 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x3a20)
    [#4](/bitcoin-bitcoin/4/) 0xc1f800183eb1cbc  (<unknown module>)
    [#5](/bitcoin-bitcoin/5/) 0xbf78000183dbda3c  (<unknown module>)
    [#6](/bitcoin-bitcoin/6/) 0xcd75800104c013bc  (<unknown module>)
    [#7](/bitcoin-bitcoin/7/) 0x000104c011fc in CheckGlobals::~CheckGlobals() check_globals.cpp:57
    [#8](/bitcoin-bitcoin/8/) 0x000104c094b0 in LLVMFuzzerTestOneInput fuzz.cpp:205
    [#9](/bitcoin-bitcoin/9/) 0x0001059c0154 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
    [#10](/bitcoin-bitcoin/10/) 0x0001059bf9d4 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
    [#11](/bitcoin-bitcoin/11/) 0x0001059c0fe0 in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:760
    [#12](/bitcoin-bitcoin/12/) 0x0001059c1c40 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:905
    [#13](/bitcoin-bitcoin/13/) 0x0001059b234c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:915
    [#14](/bitcoin-bitcoin/14/) 0x0001059d9314 in main FuzzerMain.cpp:20
    [#15](/bitcoin-bitcoin/15/) 0x000183b310dc  (<unknown module>)
    [#16](/bitcoin-bitcoin/16/) 0x71087ffffffffffc  (<unknown module>)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 2 InsertByte-CopyPart-; base unit: a4f5fd02ccbbc73b36cc7b134ec52d21b18f25f0
0x0,0x0,0x94,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xff,0xff,0xff,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0x5f,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0x5f,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xdf,0xd9,0xdf,0xdf,0xdf,0xdf,0x34,
\000\000\224\000\000\000\000\000\000\000\000\377\377\377\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337_\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337_\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\337\331\337\337\337\3374
artifact_prefix='./'; Test unit written to ./crash-d072ccb4a7016c772755ac6021155420b6b0c3f7
Base64: AACUAAAAAAAAAAD////f39/f39/f39/f39/f39/f39/f399f39/f39/f39/f39/f39/f39/f39/f39/f39/f39/f39/f39/f399f39/f39/f39/f39/f39/f39/Z39/f3zQ=

maflcko commented at 3:48 PM on April 1, 2025:

to answer my own question. This is needed by

bool CKey::VerifyPubKey(const CPubKey& pubkey) const {
    if (pubkey.IsCompressed() != fCompressed) {
        return false;
    }
    unsigned char rnd[8];
    std::string str = "Bitcoin key verification\n";
    GetRandBytes(rnd);
    uint256 hash{Hash(str, rnd)};
    std::vector<unsigned char> vchSig;
    Sign(hash, vchSig);
    return pubkey.Verify(hash, vchSig);
}

via crypter_fuzz_target->DecryptKey->VerifyPubKey->GetRandBytes

maflcko commented at 8:08 AM on March 22, 2025: member

Would be nice to add a short line on how to test/observe this fix

yancyribbens commented at 6:22 PM on March 23, 2025: contributor

nit: the commit message is pretty sparse. Could add a "why" section to the commit.

brunoerg commented at 1:27 PM on March 24, 2025: contributor

Would be nice to add a short line on how to test/observe this fix

I would say that simply checking the coverage would be enough. Note that https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/crypter.cpp.gcov.html L143 and L144 are unreachable and probably never be reachable with the current harness.

fanquake requested review from marcofleon on Mar 29, 2025

brunoerg requested review from maflcko on Apr 1, 2025

maflcko approved

maflcko commented at 3:58 PM on April 1, 2025: member

lgtm ACK 28dc118001b061aed42880ff418fde7cc5f6255d 🥊

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 28dc118001b061aed42880ff418fde7cc5f6255d 🥊
4I/TpHdmCkXMvmGjyvdt/yy0JdNb++vAYKndipfSND2IMMRnLtRb7F1FhDnYv48E6GeOM0UcL2ecBZ+K7uarAg==

</details>

fanquake merged this on Apr 2, 2025

fanquake closed this on Apr 2, 2025

brunoerg deleted the branch on Apr 2, 2025

TheCharlatan referenced this in commit a9c46ce3c3 on Apr 24, 2025

stickies-v referenced this in commit 772a33e052 on May 23, 2025

yuvicc referenced this in commit 069643f094 on Jul 6, 2025

bug-castercv502 referenced this in commit 5b0af4b944 on Sep 28, 2025

fanquake added the label Fuzzing on Oct 30, 2025