fuzz: BIP157 wire and on-disk parser harnesses (stacked on #351, #387) - #390
Open
xanimo wants to merge 13 commits into
Open
fuzz: BIP157 wire and on-disk parser harnesses (stacked on #351, #387)#390xanimo wants to merge 13 commits into
xanimo wants to merge 13 commits into
Conversation
xanimo
changed the title
Golomb-Coded Set construction and matching, the encoding BIP158 compact block filters are built from: Golomb-Rice encode/decode over a bitstream, the SipHash-2-4 based mapping of an element into the [0, N*M) range, and set construction and membership queries over a sorted delta list. This is the arithmetic layer only. It has no dependency on any P2P message type, on filter headers, or on the network at all -- it takes byte strings in and answers membership questions -- so it builds and tests standalone, and does so with WITH_NET=OFF. Placed in the unconditional source lists of both build systems rather than under WITH_NET, which is where filter code would otherwise land. Guarding it on net would have made the two build systems disagree (Makefile.am lists core sources unconditionally), and that disagreement is not theoretical: a source present in one build system and absent from the other is invisible until the other platform links. Verified src/*.c membership matches between Makefile.am and CMakeLists.txt after the change. 79/79 with test_golomb registered and running; CMake build clean with WITH_NET=OFF.
One filter header per CFCHECKPT_INTERVAL (1000) blocks, used to validate a peer's cfcheckpt response before any filter header it serves is trusted. Mainnet carries 6239 entries; testnet and regtest are empty for now. Kept out of chainparams.c on purpose. It is 6200 lines of generated data, which would bury the chain parameter definitions it would otherwise sit between, and it means adding block checkpoints and adding filter checkpoints touch different files -- so work on header sync and work on compact filters stop colliding in the same region of the same file. The regtest array carries a single zero entry with a count of 0: regtest chains are generated per test run, so there is no stable header to pin, and the entry exists only because a zero-length array is not valid C.
The BIP157 wire layer on top of the GCS encoding: getcfilters, getcfheaders and getcfcheckpt requests, the cfilter, cfheaders and cfcheckpt responses, filter header chaining, and validation of a served filter against the header committed to by that chain. Two things worth a reviewer's attention. The cfilter message follows BIP157 field order -- filter_type first, then block_hash, then the filter bytes. This is worth stating because it has been the other way round here before, and because Dogecoin Core's own compact filter work has been observed serialising block_hash first, which is not the specified order. A peer pairing this code with an implementation that swaps those two fields will parse filter_type out of block_hash[0], take the next 32 bytes as the hash, and then hit a bogus varint -- deserialisation failure at best, a silent header mismatch and a ban at worst. That failure mode does not show up in a round-trip test, because a swapped serialiser and a swapped deserialiser agree with each other perfectly. It needs a test against a fixed BIP157 payload with known bytes. Note also that the on-disk cfilters format is framed separately from the wire format, so a client reading filters from a bootstrap or cache exercises none of this. The counts in cfheaders and cfcheckpt are bounded before they size an allocation. Both are compact_size values straight off the wire, and vector_new() rounds its reservation up to a power of two, so 0xFFFFFFFF became a ~34 GB allocation before a single hash was read -- a memory exhaustion DoS from a ten byte message, the same class as the getheaders locator count. Each entry is a 32 byte hash, so any count the remaining buffer cannot hold is rejected, and cfheaders additionally takes BIP157's 2000 hash cap. Tests cover the wire format, header chaining, checkpoint validation, both unbounded-count regressions, and payload tampering: every single-bit flip of a filter payload, plus truncation and extension, must fail validation against the header computed from the original. That last one is the attack the validation exists to stop, since a peer able to strip a script from a filter makes the client skip the block that pays it. 80/80 with test_compact_filter registered and running.
The Windows build failed to link every executable with six unresolved
externals -- the three CF checkpoint arrays and their three counts:
spvnode.vcxproj : error LNK2019: unresolved external symbol
dogecoin_mainnet_cf_checkpoint_array
dogecoin.dll : fatal error LNK1120: 6 unresolved externals
The file was committed, listed in both build systems, and compiled
cleanly. The tell was in the build log: chainparams.c was compiled nine
times and cf_checkpoints.c once.
chainparams.c is declared TARGET_SOURCES(... PUBLIC ...), so CMake
compiles it into every consumer target as well as the library.
cf_checkpoints.c was PRIVATE, so only the library got it. That is
invisible on Linux, where a shared object resolves the data symbol for
the executables anyway, and fatal on a Windows DLL build, where data
symbols do not cross the boundary without dllimport -- and
LIBDOGECOIN_API expands to nothing for consumers.
So the existing PUBLIC on chainparams is deliberate rather than
incidental: it is what lets the CLI tools reference chain data arrays at
all on Windows. Filter checkpoints are the same kind of thing and need
the same treatment.
Verified by reproducing the Windows configuration locally
(-DBUILD_SHARED_LIBS=1): cf_checkpoints.c now compiles into 9 targets,
matching chainparams.c exactly, and the shared build links clean.
… does The two build systems disagreed. Makefile.am lists src/compact_filter.c in the unconditional sources, CMake had it inside IF(WITH_NET). Since test/compact_filter_tests.c is registered unconditionally in both, a CMake build configured with -DWITH_NET=OFF compiled the tests but not the code under test and failed to link with 55 undefined references. compact_filter.c has no net dependency to justify the guard: it includes only cf_checkpoints, chainparams, hash, mem, serialize and utils, and references no node, nodegroup or event_base symbol. It is message serialization and filter header arithmetic, in the same category as golomb.c, which is already unconditional. Aligning CMake with Makefile.am rather than gating the test, so the two build systems describe the same library. WITH_NET=OFF: 74/74, was failing to link. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 80/80, unchanged.
compact_filter.h referenced nothing from either: no dogecoin_headers_db, no dogecoin_headers_db_interface, no dogecoin_blockindex. headersdb.h in particular includes logdb/logdb.h, so anything including compact_filter.h also needed -I src/logdb/include on its command line, even when it never touched logdb. That is invisible inside the library, whose CPPFLAGS already carry that path, and only shows up when something outside it -- a fuzz harness, a consumer -- includes the header directly and fails with include/dogecoin/headersdb.h:37:10: fatal error: 'logdb/logdb.h' file not found The header now pulls only what it uses. Removing the two lines leaves it self-contained given the include path for dogecoin's own headers. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 80/80. WITH_NET=OFF: 74/74.
Adds the file-backed stores behind the BIP157 filter header chain: cfheaders.dat holding height+filter_header records, and cfilters.dat holding the raw filter payloads. The on-disk layout is segregated per network, <datadir>/filter/basic/<chainname>/. Sharing one directory across chains is not merely untidy: a regtest or testnet client loads the mainnet cache at startup, rescans it, matches mainnet blocks its peer has never heard of, and then blocks forever waiting for them -- while writing its own filters back into the mainnet files. A one-time rename migrates a pre-segregation mainnet cache into the new location, and only mainnet, because only mainnet data is worth keeping. Bound the cfilters record length before allocating. data_len is read straight off disk and was passed to dogecoin_malloc unchecked, so a corrupt or tampered file could request up to 4 GiB (CWE-400). A cfilter cannot legitimately exceed the P2P message that delivered it, so DOGECOIN_MAX_P2P_MSG_SIZE is the ceiling. The short read that follows would have failed the call anyway, but only after the allocation had already been attempted. Tests cover the round trip, the genesis header at offset 8, reset(), in-memory mode, a trailing partial record (dropped rather than misparsed), and the length bound. The bound test asserts on the allocator rather than the return value. An earlier version of it passed with the bound removed: malloc(4 GiB) succeeds under overcommit and the oversized read then fails, so iterate() returns false either way and the return value cannot tell the two apart. It now installs a recording dogecoin_mem_mapper and asserts no allocation exceeded the bound, which was confirmed by removing the bound and watching the test fail. 81/81, and verified against a -DBUILD_SHARED_LIBS=1 configure.
The test hardcoded /tmp/libdogecoin_cfheaders_test.dat. Neither Windows nor Android has /tmp, so x86_64-win-native and aarch64-android failed: cfheadersdb: cannot open /tmp/libdogecoin_cfheaders_test.dat: No such file or directory Use plain relative filenames in the working directory, as the wallet and spv tests already do. Both files are removed at the end of the test. Same portability class as the truncate()/_chsize() call this test already avoids -- caught there, missed here.
Wires the BIP157 message flow into the SPV client: getcfcheckpt to pin
the peer's checkpoints, getcfheaders to build and validate the filter
header chain, getcfilters to fetch filters, GCS matching against watched
scripts, and a getdata for the blocks that matched.
Filter headers are persisted through the cfheaders/cfilters DBs added in
the previous layer, so a restart resumes from the stored tip rather than
re-downloading. Cached filters are rescanned at startup, which covers
scripts registered after the filters were stored.
BIP37 and BIP157 are mutually exclusive here, by design and for privacy:
a bloom filter tells the peer which scripts are being watched, which is
the disclosure compact filters exist to avoid. Compact filters are on by
default and dogecoin_spv_client_filterload fails closed while they are,
so a BIP37 consumer opts out explicitly via
dogecoin_spv_enable_compact_filters(client, false). The test asserts both
halves of that.
headersdb gains a block-hash-by-height lookup with a resume cursor.
cfheaders batches request ascending heights, and rescanning the whole
file per lookup is O(N^2) over a 6.2M-record file; resuming from the last
match keeps it linear.
Deliberately not included, to keep this reviewable and because each
belongs elsewhere:
- parallel cfilter/cfheaders download. The state fields live in the
compact_filter layer; the logic lands in its own PR. Every path here
takes the sequential branch.
- the par_hdr parallel genesis header download, which is dogecoinfoundation#378.
- skip_pow and the stored-hash-on-load shortcut. Both were in the
prototype branch, both weaken what header loading verifies, and
neither is needed by filter sync. They belong with the bulk-load work
where their trust assumptions can be reviewed as a unit. Header
loading here is unchanged from 0.1.5-dev.
Report the height range actually covered on completion. cf_scan_start_height
was only set on one of the two scan-start paths, and the log falls back to 1
when it is unset, so a run that covered the checkpoint tail reported
"scanned heights 1..tip" -- overstating coverage in exactly the way that
hides a gap.
Verified against a local 1.14.99 node serving BIP157 with the corrected
cfilter wire order, run with an isolated HOME so the real filter cache was
untouched: 1442 cfilters parsed off the wire, full
getcfcheckpt/getcfheaders/getcfilters round trip, zero validation errors or
misbehaviour, and the completion line now reports 6314395..6315836 rather
than 1..6315836.
That run does not exercise GCS matching: -a is not wired to the filter
state until the CLI layer, so no scripts were watched. The matching block
is byte-identical to the prototype's sequential arm, which was separately
confirmed to match at a predicted height over the wire, and the unit tests
cover GCS matching directly -- but this layer's own end-to-end run proves
the wire path, not the match path.
WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 81/81. WITH_NET=OFF: 74/74.
Connects the BIP157 client to the command line, which is what makes the
feature reachable by a user rather than only by the library API.
-a/--address registers the address as a watched script with the
filter state, so cfilters are matched against it
-e/--no_cfilters disables compact filters, the documented opt-out
for a consumer that wants BIP37 instead
-o logs filter header checkpoints during sync
--cfheaders_path override cfheaders.dat location
--cfilters_path override cfilters.dat location
wallet gains dogecoin_wallet_add_watchonly_addr so a watched address is
persisted without a private key. It follows the existing add-address path
exactly, including that dogecoin_address_to_pubkey_hash returns a pointer
into a static buffer and is not freed by its existing caller either.
Deliberately excluded, because each is the seam with another feature and
not the CLI's to own:
--cf_from_genesis, --genesis_headers, --filter_hash_db, --cf_workers
--genesis_headers is the clearest case. It calls
dogecoin_spv_client_enable_genesis_headers, which initialises par_hdr
*and* resets cf_start_height and clears the CF databases -- one function
spanning parallel header download and compact filters. Exposing it here
would pull dogecoinfoundation#378 into this PR. The peer-list replication that gave each
parallel worker its own TCP connection goes with them.
End to end against a local 1.14.99 node serving BIP157, run with an
isolated HOME so the real filter cache was untouched:
[bip157] watching address: DJoQKLWJtM251NhzsR7DmbgT88TwqBbPX7
[bip157] MATCH at height 6315000
[bip157] all filters processed: scanned heights 6314418..6315859,
1 matched blocks
[bip157] requesting 1 matched full blocks across 1 peers
[bip157] processing 2 txs from matched block height=6315000
1442 cfilters parsed off the wire, zero validation errors. The address was
taken from block 6315000 via getrawtransaction before the run, so the
matched height was predicted rather than read back afterwards.
This closes the gap left open in the spv layer, where -a was not yet wired
to the filter state and so GCS matching was never exercised end to end.
WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 81/81. WITH_NET=OFF: 74/74.
Adds a libFuzzer harness for the compact filter deserializers, on the infrastructure from dogecoinfoundation#351. These parse peer-supplied bytes before anything is verified, so they are the attacker-reachable surface of filter sync: cfilter filter_type | block_hash | var_bytes(filter) cfheaders filter_type | stop_hash | prev_header | vec<filter_hash> cfcheckpt filter_type | stop_hash | vec<filter_header> plus gcs_filter_deserialize underneath them, which is the one that walks attacker-controlled bits: Golomb-Rice decoding accumulates deltas in a loop driven by the encoded data, so truncated and hostile streams are the interesting inputs rather than well-formed ones. The first input byte selects the target so one corpus covers all four. filter_type comes from the input rather than being fixed, because it selects the (M, P) parameter set and therefore how the payload decodes; the block hash is derived from the input too, since it keys the SipHash that maps elements into the filter range. Pinning either would hold the fuzzer to a single decode schedule. Result: 19,358,947 executions at ~160k/sec under -fsanitize=fuzzer, address,undefined, 228 new coverage units, no crashes. The harness was checked for reach rather than assumed to have it. With the length bound in dogecoin_p2p_msg_cfilter_deser removed: if (buf->len < filter_len) return false; it faults within seconds. Restored, it runs 11M executions clean. A harness that only ever exercises the early-return paths would pass both ways and look identical in the logs. Not covered here: the cfilters.dat record reader in cfheadersdb_file.c reads from disk rather than the wire and needs a file-shaped harness, and the BIP152 deserializers are on a separate branch. Both are follow-ups.
The wire deserializers have their own harness. This covers the other parser: the on-disk stores read back at startup. Their contents are not attacker-supplied over the network, but they are attacker-influenced -- every record was written from data a peer sent -- and a corrupt or tampered file is the case the readers have to survive rather than trust. cfilters.dat is the interesting one. Each record carries its own length and iterate() sizes an allocation directly from it: height (4) | block_hash (32) | data_len (4) | data[data_len] The harness is file-shaped because the readers take a path, not a buffer, so every execution writes and reopens a file. That caps throughput at ~1.5k/sec against ~160k/sec for the buffer harness; it is inherent to the target, not a defect in the harness. The iteration callback dereferences the first and last byte of each record rather than ignoring its arguments, so an out-of-bounds buffer is touched instead of merely being passed through. Result: 236,685 executions under -fsanitize=fuzzer,address,undefined, 136 new coverage units, no crashes. Reach was verified rather than assumed. With the bound in dogecoin_cfilters_db_iterate removed: if (data_len > DOGECOIN_MAX_P2P_MSG_SIZE) return false; the harness reports within seconds: ERROR: libFuzzer: out-of-memory (malloc(4294967295)) 0xFFFFFFFF, straight from the length field. Restored, it runs clean. That bound was added by reading the code; this is the first independent confirmation that it is load-bearing.
xanimo
force-pushed
the
0.1.5-dev-cf-fuzz
branch
from
August 5, 2026 21:04
f360463 to
eaf3bd5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| cstring *old_obj = NULL; | ||
| legacy_filter_path(&old_obj, filename); | ||
| if (stat(old_obj->str, &sb) == 0 && sb.st_size > 0) { | ||
| if (rename(old_obj->str, new_path) == 0) { |
| dogecoin_bool create = (stat(path, &sb) != 0) || (sb.st_size < (long)CF_HEADERS_FILE_HDR_LEN); | ||
|
|
||
| if (!create) { | ||
| db->file = fopen(path, "r+b"); |
| fclose(db->file); | ||
| db->file = NULL; | ||
| } | ||
| remove(path); |
| } | ||
|
|
||
| /* Create fresh v2 file */ | ||
| db->file = fopen(path, "w+b"); |
| fprintf(stderr, "Cannot open logfile '%s': %s\n", logfile, strerror(errno)); | ||
| } else { | ||
| client->nodegroup->log_write_cb = debug ? spvnode_log_both : spvnode_log_file; | ||
| } |
CodeQL cpp/world-writable-file-creation on test/cfheadersdb_tests.c. fopen(path, "wb") leaves the mode to the process umask, so the file can land world-writable and another local user could rewrite it between creation and read. Creates it 0600 via open()+fdopen() on POSIX; Windows keeps fopen, which does not have the same exposure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #351 (fuzz infrastructure) and #387 (CF stack). Review only the
top commit; everything below is those two PRs.
Adds a libFuzzer harness for the compact filter deserializers. These parse
peer-supplied bytes before anything is verified, so they are the
attacker-reachable surface of filter sync:
plus
gcs_filter_deserializeunderneath them, which is the one that actuallywalks attacker-controlled bits — Golomb-Rice decoding accumulates deltas in a
loop driven by the encoded data, so truncated and hostile streams are the
interesting inputs, not well-formed ones.
The first input byte selects the target so one corpus covers all four.
filter_typecomes from the input rather than being fixed, because it selectsthe
(M, P)parameter set and therefore how the payload decodes; the block hashis derived from the input too, since it keys the SipHash that maps elements into
the filter range. Pinning either would hold the fuzzer to one decode schedule.
Results
The harness was checked for reach, not assumed to have it
A harness that only ever hits early-return paths produces logs identical to one
with real coverage. So the bound in
dogecoin_p2p_msg_cfilter_deserwas removed:With it gone the fuzzer faults within seconds. Restored, it runs 11M
executions clean. That is the difference between "found no bugs" and "looked".
Second harness: the on-disk stores (
fuzz_cfdb)The wire deserializers are only half the parsing surface.
cfheaders.datandcfilters.datare read back at startup, and while their contents are notattacker-supplied over the network they are attacker-influenced — every record
was written from data a peer sent. A corrupt or tampered file is the case those
readers have to survive rather than trust.
cfilters.datis the interesting one: each record carries its own length anditerate()sizes an allocation directly from it.File-shaped, because the readers take a path rather than a buffer — so every
execution writes and reopens a file. That caps throughput at ~1.5k/sec against
~160k/sec for the buffer harness. Inherent to the target, not a defect in the
harness. The iteration callback dereferences the first and last byte of each
record rather than ignoring its arguments, so an out-of-bounds buffer gets
touched instead of passed through.
Reach validated the same way. With the bound in
dogecoin_cfilters_db_iterateremoved:it reports within seconds:
0xFFFFFFFF, straight from the length field. That bound was added in #384 byreading the code; this is the first independent confirmation it is load-bearing.
Still not covered
The BIP152 deserializers are on a separate branch (#379), so that harness is a
follow-up rather than an omission — worth stating so the coverage is not read as
broader than it is.
Note on #381
This PR sits on the header cleanup pushed to #381 (dropping the unused
headersdb.h/blockchain.hincludes fromcompact_filter.h). Without it,any consumer including that header outside the library needed
-I src/logdb/includeon its command line despite never touching logdb — whichis exactly how this harness first failed to compile.