This PR adds fuzz coverage for wallet/crypter.
Motivation: Issue 27272
I ran this for a long time with Sanitizers on and had no crashes; the average exec/sec also looks good to me. However, I would really appreciate it if some of the reviewers could try it on their machines too, and give their feedback.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
In addition to this and CoinControl, I'm working on the FeeBumper and Spend files. The majority of my work in FeeBumper is finished, and I'll open a PR for it as soon as we reach a conclusion on this PR.
I'm currently working on the Spend file, which should be finished this week or next, hopefully.
37 | + 38 | + // Limiting the value of nRounds since it is otherwise uselessly expensive and causes a timeout when fuzzing. 39 | + crypt.SetKeyFromPassphrase(/*strKeyData=*/secure_string, 40 | + /*chSalt=*/ConsumeRandomLengthByteVector(fuzzed_data_provider), 41 | + /*nRounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000), 42 | + /*nDerivationMethod*/ nDerivationMethod);
/*nDerivationMethod=*/ nDerivationMethod);
Thanks! Updated.
ACK 6c98f4c137c9d557c78ebd52379711ebbd23e24a
non blocker, just an idea: Perhaps we could have encrypt/decrypt stuff outside of LIMITED_WHILE and then we could have a call just to fill them. I suppose this way we would have more chance of decrypting a previous encrypted thing. Just an example:
diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp
index 7ba43821b..03ad97a35 100644
--- a/src/wallet/test/fuzz/crypter.cpp
+++ b/src/wallet/test/fuzz/crypter.cpp
@@ -24,6 +24,9 @@ FUZZ_TARGET_INIT(crypter, initialize_crypter)
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
CCrypter crypt;
+ std::vector<unsigned char> cipher_text_ed;
+ CKeyingMaterial plain_text_ed;
+
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
{
@@ -49,14 +52,16 @@ FUZZ_TARGET_INIT(crypter, initialize_crypter)
},
[&] {
const std::vector<unsigned char> random_vector = ConsumeRandomLengthByteVector(fuzzed_data_provider);
- const CKeyingMaterial plain_text(random_vector.begin(), random_vector.end());
- std::vector<unsigned char> cipher_text;
- crypt.Encrypt(plain_text, cipher_text);
+ plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end());
},
[&] {
- CKeyingMaterial plain_text;
- const std::vector<unsigned char> cipher_text = ConsumeRandomLengthByteVector(fuzzed_data_provider);
- crypt.Decrypt(cipher_text, plain_text);
+ std::vector<unsigned char> cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);
+ },
+ [&] {
+ crypt.Encrypt(plain_text_ed, cipher_text_ed);
+ },
+ [&] {
+ crypt.Decrypt(cipher_text_ed, plain_text_ed);
},
[&] {
Thanks, @MarcoFalke and @brunoerg for the reviews.
Encrypt/Decrypt variables outside of LIMITED_WHILE and added two functions to update them regularly.
reACK e010fddf15a6d4ff84bfebdcb33c9c1d9cea7f0a
17 | +{ 18 | + static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(); 19 | + g_setup = testing_setup.get(); 20 | +} 21 | + 22 | +FUZZ_TARGET_INIT(crypter, initialize_crypter)
Needs rebase
Thanks, done!
Forced pushed updating changes in PR 28065.
reACK 7570e2228e22c3b22e119e159c6fa07c4c891d98
53 | + [&] { 54 | + const std::vector<unsigned char> random_vector = ConsumeRandomLengthByteVector(fuzzed_data_provider); 55 | + plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end()); 56 | + }, 57 | + [&] { 58 | + std::vector<unsigned char> cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider);
That's a no-op assignment due to the symbol shadow.
Thank you so much for noticing this. I don't know how I missed this.
Forced pushed fixing the Shadow variable issue.
34 | + fuzzed_data_provider, 35 | + [&] { 36 | + const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString(); 37 | + SecureString secure_string(random_string.begin(), random_string.end()); 38 | + 39 | + const unsigned int nDerivationMethod = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();
style nit, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
const unsigned int derivation_method = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();
40 | + 41 | + // Limiting the value of nRounds since it is otherwise uselessly expensive and causes a timeout when fuzzing. 42 | + crypt.SetKeyFromPassphrase(/*strKeyData=*/secure_string, 43 | + /*chSalt=*/ConsumeRandomLengthByteVector(fuzzed_data_provider), 44 | + /*nRounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000), 45 | + /*nDerivationMethod=*/ nDerivationMethod);
style nit:
/*nDerivationMethod=*/nDerivationMethod);
17 | +{ 18 | + static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(); 19 | + g_setup = testing_setup.get(); 20 | +} 21 | + 22 | +FUZZ_TARGET(crypter, .init = initialize_crypter)
I don't quite understand why the CI isn't failing here. You're using the new macro from #28065 but you haven't rebased past that PR, so this shouldn't work.
These are the errors that i get locally:
wallet/test/fuzz/crypter.cpp:22:22: error: too many arguments provided to function-like macro invocation
FUZZ_TARGET(crypter, .init = initialize_crypter)
^
./test/fuzz/fuzz.h:31:9: note: macro 'FUZZ_TARGET' defined here
#define FUZZ_TARGET(name) \
^
wallet/test/fuzz/crypter.cpp:22:1: error: a type specifier is required for all declarations
FUZZ_TARGET(crypter, .init = initialize_crypter)
^
wallet/test/fuzz/crypter.cpp:24:5: error: 'FuzzedDataProvider' does not refer to a value
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
^
./test/fuzz/FuzzedDataProvider.h:31:7: note: declared here
class FuzzedDataProvider {
^
wallet/test/fuzz/crypter.cpp:89:2: error: expected ';' after top level declarator
}
^
;
4 errors generated.
CI does a "rebase" before each run. Generally this detects more issues (silent merge conflicts), and I think this is the first case that CI missed a compile failure of this kind. (Generally it is assumed that developers at a minimum try to compile their changes locally, so it would be odd to have a dedicated CI check for that, but maybe there should be?)
Now that we are moving to persistent workers, we can even compile all commits in a pull request. Maybe as a separate task on a runner where it doesn't matter when the CI run takes a day or two?
Now that we are moving to persistent workers, we can even compile all commits in a pull request.
Sounds good but would that really take one or two days? With caching this should only take marginally longer, no?
I am not sure how much CPU we want to spend on this. It should only ever find an issue once every couple of months. And there could be quite a backlog easily. For example, 3 devs could each force push a 15 commit pull that touches serialize.h at the same time. So the cache would be useless and the CI would have to do 45 builds from scratch. (Assuming each takes half an hour, that'd be a day of backlog)
In any case, should be trivial to implement by removing the if: github.event.pull_request.commits != 1 and git checkout HEAD~ from #28497 (comment)
This is out of the blue, but the reason I struggle to compile after each change is that I'm currently using a Windows 10 OS. And I can't run Wallet Fuzz tests on my machine for some unknown reason. I've attempted Fuzz tests outside of Wallet code, and they run fine. I've also experimented with other Windows machines, and this issue seems to persists across all of them.
So, I've been using an Ubuntu VM for code testing. I've gone through Makefile.test.include, and everything appears to be in order. I'm uncertain about the reason of this problem here.
Seems fine to report an issue about this, with exact steps to reproduce.
Yes, I'm on it. Will try to reproduce it once again and create an issue then.
Thank you for pointing this up, @dergoegge. Actually, I thought it was just a one-line change and became overly relaxed about it. But I'm sorry, and from now on I'll compile before pushing every time.
I'm also going to address your style nits in the next push.
5 | +#include <test/fuzz/FuzzedDataProvider.h> 6 | +#include <test/fuzz/fuzz.h> 7 | +#include <test/fuzz/util.h> 8 | +#include <test/util/setup_common.h> 9 | +#include <wallet/crypter.h> 10 | +#include <wallet/test/util.h>
Is this needed?
From iwyu:
wallet/test/fuzz/crypter.cpp should add these lines:
#include <memory> // for unique_ptr
#include <optional> // for optional
#include <string> // for string
#include <vector> // for vector
#include "key.h" // for CKey
#include "pubkey.h" // for CPubKey
#include "support/allocators/secure.h" // for secure_allocator, SecureString
#include "uint256.h" // for uint256
wallet/test/fuzz/crypter.cpp should remove these lines:
- #include <wallet/test/util.h> // lines 10-10
No, it is not needed. Removing this, thanks!
56 | + }, 57 | + [&] { 58 | + cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider); 59 | + }, 60 | + [&] { 61 | + crypt.Encrypt(plain_text_ed, cipher_text_ed);
If you're not using the returned value, you could do:
(void)crypt.Encrypt(plain_text_ed, cipher_text_ed);
However, I think you could check if Encrypt returned true then you could try to decrypt it.
Yeah, I think that is better. I'll push these changes before the weekend (have college exams throughout this week, apologies for the delay). Thanks!
However, I think you could check if
Encryptreturnedtruethen you could try to decrypt it.
I believe it will attempt to Decrypt it, perhaps not instantly but eventually, so I suppose for a long Fuzz run it won't matter much. Should I simply (void) it?
<wallet/test/util.h> import.(void) before the Encrypt function call.
I think check failures are coming after I rebased the code. Because my changes are very basic to produce these fails.
lint failure is due to spelling mistakes in other files.fuzzer is failing, it seems unrelated to my file.In the meantime, I can pull up a PR correcting all the spelling mistakes in the lint. Or I can correct those in this PR only.
The lint task output seems confusing and fragile, but the error here is:
This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp
@@ -0,0 +1,90 @@
+ // These values are regularly updated within `CallOneOf`
I guess in the future it could make sense to clarify that the spelling output is only generated for people that care about fixing spelling mistakes, and can be ignored by everyone else.
Force pushed fixing the lint issue.
I'm currently looking into why the Fuzzer is failing. Will try to fix it ASAP.
The fuzz timeout can be ignored for now and will be fixed this week.
76 | + }, 77 | + [&] { 78 | + std::optional<CPubKey> random_pub_key = ConsumeDeserializable<CPubKey>(fuzzed_data_provider); 79 | + if (!random_pub_key) return; 80 | + const CPubKey pub_key = *random_pub_key; 81 | + const std::vector<unsigned char> random_key = ConsumeRandomLengthByteVector(fuzzed_data_provider);
Could also move them out of the loop, like the others?
Did it. I was thinking of moving master_key outside as well but since it is const it wouldn't make sense. Should I add a function in CallOneOf that updates the random_key occasionally as well?
lgtm
Can someone tell me why is the previous releases, qt5 dev package and depends packages, DEBUG test is failing?
I've ran this on my machine and it works fine.
It is using an old CI config. You can rebase on current master to get a fresh CI run.
59 | + }, 60 | + [&] { 61 | + (void)crypt.Encrypt(plain_text_ed, cipher_text_ed); 62 | + }, 63 | + [&] { 64 | + crypt.Decrypt(cipher_text_ed, plain_text_ed);
(void)crypt.Decrypt(cipher_text_ed, plain_text_ed);
72 | + const CKeyingMaterial master_key(random_key.begin(), random_key.end()); 73 | + const uint256 iv = ConsumeUInt256(fuzzed_data_provider); 74 | + DecryptSecret(master_key, cipher_text_ed, iv, plain_text_ed); 75 | + }, 76 | + [&] { 77 | + std::optional<CPubKey> random_pub_key = ConsumeDeserializable<CPubKey>(fuzzed_data_provider);
Since you're using ConsumeDeserializable, I do believe you should adopt the approach introduced in "fuzz: Avoid timeout and bloat in fuzz targets" fabb5046a7365af3079e6e45606d63576bc6ad12
Did the necessary changes. Thanks!
Up for grabs?
Up for grabs?
Updating in a day or too. Just got over with my final year college exams. Will address your changes and rebase to the main.
Same for the Spend file.
good_data approach to avoid unnecessary fuzz runs.
lint seems wrong. I couldn't find any trailing whitespaces in the file.
It is not wrong. No file in this repo uses Windows newlines and they are problematic when shown in git.
Fixed the lint issue
A soft ping. Can we move forward with this PR if this looks good?
utACK d7290d662f494503f28e087dd728b492c0bb2c5f
utACK d7290d662f494503f28e087dd728b492c0bb2c5f
My fuzzer is crashing when I run this. I'm going to look more into it tomorrow.
My fuzzer is crashing when I run this. I'm going to look more into it tomorrow.
Please indicate the traceback, and the sanitizers you used, also the fuzz input file.
I used ASan and UBSan. I think it might just be a mac specifc issue, I'm fuzzing on a mac with m3 chip.
Here's the error:
INFO: seed corpus: files: 111934 min: 1b max: 5242880b total: 2040057759b rss: 216Mb
[#8192](/bitcoin-bitcoin/8192/) pulse cov: 938 ft: 2162 corp: 81/730b exec/s: 4096 rss: 318Mb
[#16384](/bitcoin-bitcoin/16384/) pulse cov: 1049 ft: 2510 corp: 127/2096b exec/s: 5461 rss: 416Mb
[#32768](/bitcoin-bitcoin/32768/) pulse cov: 1571 ft: 4427 corp: 219/9334b exec/s: 4681 rss: 565Mb
[#65536](/bitcoin-bitcoin/65536/) pulse cov: 1774 ft: 7969 corp: 371/56Kb exec/s: 4681 rss: 565Mb
libc++abi: terminating due to uncaught exception of type std::bad_alloc: std::bad_alloc
==15322== ERROR: libFuzzer: deadly signal
[#0](/bitcoin-bitcoin/0/) 0x10824cee4 in __sanitizer_print_stack_trace+0x28 (libclang_rt.asan_osx_dynamic.dylib:arm64+0x5cee4)
[#1](/bitcoin-bitcoin/1/) 0x105025088 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
[#2](/bitcoin-bitcoin/2/) 0x10500ae70 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:231
[#3](/bitcoin-bitcoin/3/) 0x1944b3580 in _sigtramp+0x34 (libsystem_platform.dylib:arm64+0x4580)
[#4](/bitcoin-bitcoin/4/) 0xf96f000194482c1c (<unknown module>)
[#5](/bitcoin-bitcoin/5/) 0xda0700019438fa1c (<unknown module>)
[#6](/bitcoin-bitcoin/6/) 0x2560000194439d2c (<unknown module>)
[#7](/bitcoin-bitcoin/7/) 0x5f79800194429fc8 (<unknown module>)
[#8](/bitcoin-bitcoin/8/) 0x23020001940c81dc (<unknown module>)
[#9](/bitcoin-bitcoin/9/) 0x31568001944390f0 (<unknown module>)
[#10](/bitcoin-bitcoin/10/) 0x756e00019443c344 (<unknown module>)
[#11](/bitcoin-bitcoin/11/) 0x462e00019443c288 (<unknown module>)
[#12](/bitcoin-bitcoin/12/) 0x4867000102c4910c (<unknown module>)
[#13](/bitcoin-bitcoin/13/) 0x102c4b74c in std::__1::vector<unsigned char, secure_allocator<unsigned char>>::__vallocate[abi:ne180100](unsigned long) vector:741
[#14](/bitcoin-bitcoin/14/) 0x102c45a44 in wallet::(anonymous namespace)::crypter_fuzz_target(Span<unsigned char const>) crypter.cpp:34
[#15](/bitcoin-bitcoin/15/) 0x103361680 in LLVMFuzzerTestOneInput fuzz.cpp:179
[#16](/bitcoin-bitcoin/16/) 0x10500c298 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
[#17](/bitcoin-bitcoin/17/) 0x10500bb9c in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
[#18](/bitcoin-bitcoin/18/) 0x10500d9b8 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:829
[#19](/bitcoin-bitcoin/19/) 0x10500da74 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:867
[#20](/bitcoin-bitcoin/20/) 0x104ffe380 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:914
[#21](/bitcoin-bitcoin/21/) 0x105025970 in main FuzzerMain.cpp:20
[#22](/bitcoin-bitcoin/22/) 0x1940fa0dc (<unknown module>)
[#23](/bitcoin-bitcoin/23/) 0x810fffffffffffc (<unknown module>)
And here is the super long crash input https://gist.github.com/marcofleon/f2f50cc2e5eb3c2f5efabc511e4fb6ef
If I add limits (1024 for example) to ConsumeRandomLengthByteVector and ConsumeRandomLengthString the harness works. But that could be defeating the purpose of fuzzing.
I couldn't reproduce on MacOS 14.3 with that input file.
If I add limits (1024 for example) to ConsumeRandomLengthByteVector and ConsumeRandomLengthString the harness works. But that could be defeating the purpose of fuzzing.
Changing the harness can invalidate the input. Also, ConsumeRandomLengthByteVector is used in a lot of other targets without a limit. Does it happen for other targets as well, @marcofleon?
If I don't start with a seed corpus (inputs from scratch) then it seems to run fine. I let it go for a while and no crashes happen. It's only when I run something like this:
FUZZ=crypter src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 ../qa-assets/fuzz_corpus_copy/crypter ../qa-assets/fuzz_corpus_copy/*
Then it continuously fails to merge and that bad alloc error keeps coming up over and over. I tried this same command on other harnesses that contain ConsumeRandomLengthByteVector (crypto, crypto_aes256cbc, addrman, net, http_request, script, connman) and they all result in a normal output.
@brunoerg, if you don't mind could you try a couple of these other crash inputs too?
https://gist.github.com/marcofleon/b4f59f23c5b3e2a37da362f556fdbd4f
https://gist.github.com/marcofleon/62558bc4a18e90c92a164a7498f486c2
https://gist.github.com/marcofleon/8b44613426d50ada97f87db3c3195442
https://gist.github.com/marcofleon/dfdc88df73800617a19ca9f509f20417
https://gist.github.com/marcofleon/ff19b5a5b7dcde735e75fc583a1343c3
https://gist.github.com/marcofleon/77ac845c8d43d3b9375bb5163374fce7
set_cover_merge
This uses more memory than -merge=1, so my recommendation would be to try to add more memory or swap to your machine, or to use -merge=1 for now.
ACK d7290d662f494503f28e087dd728b492c0bb2c5f
I get that any coverage of these functions is better than no coverage, but it seems less useful to me when we aren't checking the output. There may not be a crash, but each function could be doing something unexpected, and we should be checking to make sure that they aren't. I'm going to merge this for now though just so there is coverage, but a followup which verifies the behavior would be welcomed.
60 | + }, 61 | + [&] { 62 | + (void)crypt.Encrypt(plain_text_ed, cipher_text_ed); 63 | + }, 64 | + [&] { 65 | + (void)crypt.Decrypt(cipher_text_ed, plain_text_ed);