fuzz: check http_request body matches framing#35759
fuzz: check http_request body matches framing#35759Ameen-Alam wants to merge 1 commit intobitcoin:masterbitcoin/bitcoin:masterfrom Ameen-Alam:fuzz-http-request-body-assertAmeen-Alam/bitcoin:fuzz-http-request-body-assertCopy head branch name to clipboard
Conversation
The http_request target asserted that ReadBody() returns an empty string. That held for the libevent-based http_libevent::HTTPRequest, where the harness only parsed the request line and headers and never populated a body. Commit 9c20859 (PR bitcoin#35182) replaced libevent with http_bitcoin::HTTPRequest, and the target was switched over in e427c22; its LoadBody() now decodes Content-Length and chunked bodies per RFC 9112, so any fully-parsed request carrying a body trips the stale assertion (e.g. "POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc"). Replace the emptiness check with a framing-consistency check that mirrors LoadBody()'s own branch logic: a chunked body is bounded by MAX_BODY_SIZE, a Content-Length body is exactly that many bytes, and a request with neither framing header has no body. This strengthens the target instead of dropping the assertion.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35759. ReviewsSee the guideline and AI policy for information on the review process. |
|
concept ack on improving the http fuzz test but I'd like to wait for #35735 which I think might also fix this same issue or otherwise require a fuzz test change. My assumption in #35182 was the readbody assertion was safe because of the unlikelihood that fuzz data produces a valid HTTP request. I fuzzed for days without a crash (bitcoin-core/qa-assets#280) But I think the authors intention here is correct, the assertion was safe not because of luck but because of libevent's architecture. |
|
Thanks Mr. Matthew Zipkin, that makes sense. I am happy to wait for #35735, then rebase and adapt this change based on the LoadBody semantics that land there. The reproducer currently triggers deterministically on master, so I will rerun it against #35735 to check whether a body assertion is still needed. I can also share the test results on that PR. |
Did a quick check with libFuzzer and a week of CPU, but it doesn't seem to be escaping the coverage hole. Haven't tried AFL/honggfuzz or other engines. I guess this was found with an LLM? E.g: |
|
Yes, AI assistance helped me spot the stale assumption, then I reproduced it, traced it back to the libevent removal in #35182 and the harness change in e427c22, and wrote and verified the fix. I think the fuzzer missed it because the input needs a valid request line, complete headers, and a Content-Length that exactly matches the remaining body. Random mutation is unlikely to produce all three together, so this seems more like a corpus reachability issue. I can add the repro input to qa-assets as a seed. I tested this on top of fb0a085 from #35735. Before applying this change, I could still trigger assert(body.empty()) with POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc. The cherry-pick applied without any conflicts. All five framing cases passed afterward, and I also ran the HTTP fuzz target for 10 minutes without a crash. From what I tested, the two changes seem independent, so either one should be able to merge first. Related testing and findings: #35735 (comment) |
The http_request target asserted that ReadBody() returns an empty string. That held for the libevent-based http_libevent::HTTPRequest, where the harness only parsed the request line and headers and never populated a body. Commit 9c20859 (PR #35182) replaced libevent with http_bitcoin::HTTPRequest, and the target was switched over in e427c22; its LoadBody() now decodes Content-Length and chunked bodies per RFC 9112, so any fully-parsed request carrying a body trips the stale assertion (e.g. "POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc").
Replace the emptiness check with a framing-consistency check that mirrors LoadBody()'s own branch logic: a chunked body is bounded by MAX_BODY_SIZE, a Content-Length body is exactly that many bytes, and a request with neither framing header has no body. This strengthens the target instead of dropping the assertion.
Steps to reproduce (old assertion):
Build the fuzz binary and pass this input as a file to the
http_requesttarget:POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc→
test/fuzz/http_request.cpp:49: Assertion 'body.empty()' failedTesting the fix:
Ran the updated target ~16 min under libFuzzer with ASAN/UBSAN
(14.2M execs, no crashes), plus targeted inputs for each branch:
Content-Length body, chunked,
Transfer-Encoding: identity+ Content-Length,no framing headers, and
Content-Length: 0. Happy to contribute the reproinput to qa-assets as a follow-up.