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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32118.
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.
19@@ -20,6 +20,7 @@ void initialize_crypter()
20
21 FUZZ_TARGET(crypter, .init = initialize_crypter)
22 {
23+ SeedRandomStateForTest(SeedRand::ZEROS);
Without it I get:
0[#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"-
1
2
3The current fuzz target used the global random state.
4
5This is acceptable, but requires the fuzz target to call
6SeedRandomStateForTest(SeedRand::ZEROS) in the first line
7of the FUZZ_TARGET function.
8
9An alternative solution would be to avoid any use of globals.
10
11Without a solution, fuzz instability and non-determinism can lead
12to non-reproducible bugs or inefficient fuzzing.
13
14
15==37661== ERROR: libFuzzer: deadly signal
16 [#0](/bitcoin-bitcoin/0/) 0x0001071d358c in __sanitizer_print_stack_trace+0x14 (libclang_rt.ubsan_osx_dynamic.dylib:arm64+0xb58c)
17 [#1](/bitcoin-bitcoin/1/) 0x0001059d8910 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
18 [#2](/bitcoin-bitcoin/2/) 0x0001059beba8 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:231
19 [#3](/bitcoin-bitcoin/3/) 0x000183ee1a20 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x3a20)
20 [#4](/bitcoin-bitcoin/4/) 0xc1f800183eb1cbc (<unknown module>)
21 [#5](/bitcoin-bitcoin/5/) 0xbf78000183dbda3c (<unknown module>)
22 [#6](/bitcoin-bitcoin/6/) 0xcd75800104c013bc (<unknown module>)
23 [#7](/bitcoin-bitcoin/7/) 0x000104c011fc in CheckGlobals::~CheckGlobals() check_globals.cpp:57
24 [#8](/bitcoin-bitcoin/8/) 0x000104c094b0 in LLVMFuzzerTestOneInput fuzz.cpp:205
25 [#9](/bitcoin-bitcoin/9/) 0x0001059c0154 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
26 [#10](/bitcoin-bitcoin/10/) 0x0001059bf9d4 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
27 [#11](/bitcoin-bitcoin/11/) 0x0001059c0fe0 in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:760
28 [#12](/bitcoin-bitcoin/12/) 0x0001059c1c40 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:905
29 [#13](/bitcoin-bitcoin/13/) 0x0001059b234c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:915
30 [#14](/bitcoin-bitcoin/14/) 0x0001059d9314 in main FuzzerMain.cpp:20
31 [#15](/bitcoin-bitcoin/15/) 0x000183b310dc (<unknown module>)
32 [#16](/bitcoin-bitcoin/16/) 0x71087ffffffffffc (<unknown module>)
33
34NOTE: libFuzzer has rudimentary signal handlers.
35 Combine libFuzzer with AddressSanitizer or similar for better crash reports.
36SUMMARY: libFuzzer: deadly signal
37MS: 2 InsertByte-CopyPart-; base unit: a4f5fd02ccbbc73b36cc7b134ec52d21b18f25f0
380x0,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,
39\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
40artifact_prefix='./'; Test unit written to ./crash-d072ccb4a7016c772755ac6021155420b6b0c3f7
41Base64: 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=
to answer my own question. This is needed by
0bool CKey::VerifyPubKey(const CPubKey& pubkey) const {
1 if (pubkey.IsCompressed() != fCompressed) {
2 return false;
3 }
4 unsigned char rnd[8];
5 std::string str = "Bitcoin key verification\n";
6 GetRandBytes(rnd);
7 uint256 hash{Hash(str, rnd)};
8 std::vector<unsigned char> vchSig;
9 Sign(hash, vchSig);
10 return pubkey.Verify(hash, vchSig);
11}
via crypter_fuzz_target->DecryptKey->VerifyPubKey->GetRandBytes
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.
lgtm ACK 28dc118001b061aed42880ff418fde7cc5f6255d 🥊
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 28dc118001b061aed42880ff418fde7cc5f6255d 🥊
34I/TpHdmCkXMvmGjyvdt/yy0JdNb++vAYKndipfSND2IMMRnLtRb7F1FhDnYv48E6GeOM0UcL2ecBZ+K7uarAg==
