fuzz: Generate rpc fuzz targets individually #28015
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. |
|
Would be nice if someone also implemented this for OSS-Fuzz: It would require building a fuzz exe for each RPC method, and then probably also adjust https://github.com/bitcoin-core/qa-assets/blob/main/download_oss_fuzz_inputs.py to somehow concatenate all RPC methods into one |
brunoerg
left a comment
brunoerg
left a comment
There was a problem hiding this comment.
ACK fa1e27f
Running only with rpc, targets in generate_corpus becomes:
[('rpc', {'LIMIT_TO_RPC_COMMAND': 'analyzepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'clearbanned'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinerawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'converttopsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createmultisig'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createpsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createrawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'decodepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'decoderawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'decodescript'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'deriveaddresses'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'descriptorprocesspsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'disconnectnode'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'echo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'echojson'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'estimaterawfee'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'estimatesmartfee'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'finalizepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'generate'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'generateblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getaddednodeinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getbestblockhash'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockchaininfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockcount'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockfilter'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockfrompeer'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockhash'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockheader'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblockstats'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getblocktemplate'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getchaintips'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getchaintxstats'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getconnectioncount'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getdeploymentinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getdescriptorinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getdifficulty'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getindexinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getmemoryinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getmempoolancestors'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getmempooldescendants'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getmempoolentry'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getmempoolinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getmininginfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getnettotals'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getnetworkhashps'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getnetworkinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getnodeaddresses'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getpeerinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getprioritisedtransactions'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getrawmempool'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getrawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'getrpcinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'gettxout'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'gettxoutsetinfo'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'gettxspendingprevout'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'help'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'invalidateblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'joinpsbts'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'listbanned'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'logging'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'mockscheduler'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'ping'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'preciousblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'prioritisetransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'pruneblockchain'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'reconsiderblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'scanblocks'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'scantxoutset'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'sendrawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'setmocktime'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'setnetworkactive'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'signmessagewithprivkey'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'signrawtransactionwithkey'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'submitblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'submitheader'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'submitpackage'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'syncwithvalidationinterfacequeue'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'testmempoolaccept'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'uptime'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'utxoupdatepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'validateaddress'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'verifychain'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'verifymessage'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'verifytxoutproof'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'waitforblock'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'waitforblockheight'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'waitfornewblock'})]
same on master:
['rpc']
|
If we want to fuzz RPCs individually through the test runner and oss-fuzz then wouldn't it make more sense to actually have a separate target per RPC (similar to what we used to have with process_message)? Otherwise we end up with this extra test runner edge case and another oss-fuzz hack. |
Extra code needs to be written to support individual targets in the test runner. As you say this can either be achieved by compiling separate targets, or by adding code to the test runner (as is done in this patch). Unless there are any benefits of your approach, it seems backward, because it will bring back all the issues that #27766 fixed. |
I think the difference is that process_message has not shown to be too complex for the fuzzers. Any bug found via process_message_* was also found via process_message, so having the individual targets didn't make sense considering the overhead. For the rpc target that isn't true as we have seen with #27913, so to me it seems like the overhead of having the individual targets makes sense in this case. We could also consider trimming the number of RPCs we are fuzzing down to the important ones (e.g. frequently used RPCs, RPCs lightning impls rely on, etc.), to reduce the number of targets we have. (Just my preference and not an approach nack) |
Have you looked at the RPC generating helper? IIUC you may generate a hex-CBlock for a psbt RPC, which may lead to coverage feedback, but not actually useful coverage. I'd expect the overhead and downsides to be more pronounced for the Happy to close this pull, and apply it locally, but I think something like this is useful. |
Yea, leave it open. Will test today. |
There was a problem hiding this comment.
From an empty corpus, was able to find this bug in approx. 2 hours with LIMIT_TO_RPC_COMMAND (on my pretty old i5-6500T CPU @ 2.50GHz / 8GB RAM machine):
FUZZ=rpc FUZZ=rpc LIMIT_TO_RPC_COMMAND=analyzepsbt UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz ~/fuzz/rpc -fork=4
I'm letting it run again overnight without LIMIT_TO_RPC_COMMAND (from an empty corpus) to see if I can find it in a day or so, but given that oss-fuzz took so long I'm not expecting much. (edit: 5 days later, still nothing)
Fuzzing is still a bit too magical to me to comment on whether or not this approach is the best we can take, but it seems sensible, and the code looks fine to me. This is probably as far as I can go in my review now.
(Also thank you @dergoegge for all the offline help)
| {corpus_dir}. | ||
| """ | ||
| logging.info("Generating corpus to {}".format(corpus_dir)) | ||
| rpc_target = "rpc" |
There was a problem hiding this comment.
nit: Would add a small docstring here:
| rpc_target = "rpc" | |
| # Instead of a single rpc target, replace it with a a target for each rpc function""" | |
| rpc_target = "rpc" |
There was a problem hiding this comment.
Thanks, there will be follow-ups either way, so I'll save the nit for later, if that is ok.
| has_rpc = rpc_target in targets | ||
| if has_rpc: | ||
| targets.remove(rpc_target) | ||
| targets = [(t, {}) for t in targets] |
There was a problem hiding this comment.
doc nit:
| targets = [(t, {}) for t in targets] | |
| targets = [(t, {}) for t in targets] # expand to add dictionary for target-specific env variables |
There was a problem hiding this comment.
Thanks, there will be follow-ups either way, so I'll save the nit for later, if that is ok.
For reference, within the suggested solutions (either compile into a separate binary for each fuzz target, or select via env var), all should be identical in the eyes of the fuzz engine. Obviously there are other improvements that can be done, but they are orthogonal to this pull. |
|
Is this rfm? |
fa1e27f fuzz: Generate rpc fuzz targets individually (MarcoFalke) Pull request description: The `rpc` fuzz target was added more than two years ago in e458631. However, the bug bitcoin#27913 was only found recently. Thus, it is pretty clear that fuzz engines can't deal with a search space that is too broad and can be extended in too many directions. Fix that by limiting the search space to each RPC method name and then iterate over all names, instead of letting the fuzz engine do the iteration. With this, the bug can be found in seconds, as opposed to years of CPU time (or never). ACKs for top commit: brunoerg: ACK fa1e27f dergoegge: ACK fa1e27f Tree-SHA512: 45ccba842367650d010320603153276b1b303deda9ba8c6bb31a4d2473b00aa5bca866db95f541485d65efd8276e2575026968c037872ef344fa33cf45bcdcd7
6 participants
The
rpcfuzz target was added more than two years ago in e458631. However, the bug #27913 was only found recently. Thus, it is pretty clear that fuzz engines can't deal with a search space that is too broad and can be extended in too many directions.Fix that by limiting the search space to each RPC method name and then iterate over all names, instead of letting the fuzz engine do the iteration.
With this, the bug can be found in seconds, as opposed to years of CPU time (or never).