fuzz: Generate rpc fuzz targets individually

MarcoFalke commented at 12:58 pm on June 30, 2023: member

The rpc fuzz target was added more than two years ago in e45863166f5e44cc2c380f4667812fcd3cddc73b. 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).

fuzz: Generate rpc fuzz targets individually fa1e27fe8e

DrahtBot commented at 12:58 pm on June 30, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	brunoerg, 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 Jun 30, 2023

MarcoFalke commented at 1:05 pm on June 30, 2023: member

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 rpc.zip file.

brunoerg commented at 1:43 pm on June 30, 2023: contributor

Concept ACK

brunoerg approved

brunoerg commented at 6:19 pm on July 4, 2023: contributor

ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a

Running only with rpc, targets in generate_corpus becomes:

0[('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:

0['rpc']

fanquake requested review from dergoegge on Jul 5, 2023

dergoegge commented at 10:10 am on July 5, 2023: member

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.

MarcoFalke commented at 10:19 am on July 5, 2023: member

Otherwise we end up with this extra test runner edge case

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.

dergoegge commented at 10:38 am on July 5, 2023: member

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)

MarcoFalke commented at 10:45 am on July 5, 2023: member

so to me it seems like the overhead of having the individual targets makes sense in this case.

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 rpc target than the process_message target. I have some ideas on how to improve the rpc target, but this can be done later.

Happy to close this pull, and apply it locally, but I think something like this is useful.

dergoegge commented at 10:56 am on July 5, 2023: member

Happy to close this pull, and apply it locally, but I think something like this is useful.

Yea, leave it open. Will test today.

in test/fuzz/test_runner.py:201 in fa1e27fe8e

197@@ -198,29 +198,47 @@ def generate_corpus(*, fuzz_pool, src_dir, build_dir, corpus_dir, targets):
198     {corpus_dir}.
199     """
200     logging.info("Generating corpus to {}".format(corpus_dir))
201+    rpc_target = "rpc"

stickies-v commented at 2:15 pm on July 5, 2023:

nit: Would add a small docstring here:

0
1    # Instead of a single rpc target, replace it with a a target for each rpc function"""
2    rpc_target = "rpc"

MarcoFalke commented at 2:35 pm on July 5, 2023:

Thanks, there will be follow-ups either way, so I’ll save the nit for later, if that is ok.

in test/fuzz/test_runner.py:205 in fa1e27fe8e

197@@ -198,29 +198,47 @@ def generate_corpus(*, fuzz_pool, src_dir, build_dir, corpus_dir, targets):
198     {corpus_dir}.
199     """
200     logging.info("Generating corpus to {}".format(corpus_dir))
201+    rpc_target = "rpc"
202+    has_rpc = rpc_target in targets
203+    if has_rpc:
204+        targets.remove(rpc_target)
205+    targets = [(t, {}) for t in targets]

stickies-v commented at 2:16 pm on July 5, 2023:

doc nit:

0    targets = [(t, {}) for t in targets]  # expand to add dictionary for target-specific env variables

MarcoFalke commented at 2:35 pm on July 5, 2023:

Thanks, there will be follow-ups either way, so I’ll save the nit for later, if that is ok.

stickies-v commented at 2:26 pm on July 5, 2023: contributor

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):

0FUZZ=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)

MarcoFalke commented at 2:39 pm on July 5, 2023: member

Fuzzing is still a bit too magical to me to comment on whether or not this approach is the best we can take.

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.

dergoegge approved

dergoegge commented at 3:25 pm on July 6, 2023: member

ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a

MarcoFalke commented at 9:12 am on July 7, 2023: member

Is this rfm?

fanquake merged this on Jul 7, 2023

fanquake closed this on Jul 7, 2023

sidhujag referenced this in commit f6ab70052e on Jul 7, 2023

MarcoFalke deleted the branch on Jul 8, 2023

fuzz: Generate rpc fuzz targets individually #28015

Reviews