Reject a Range first-byte-pos that overflows ssize_t - #2580
Merged
Conversation
parse_range_header initializes first to the -1 sentinel that means "no first-byte-pos" and only overwrites it when detail::from_chars succeeds. On std::errc::result_out_of_range the assignment is skipped and -1 survives, so "bytes=9223372036854775808-100" is parsed as the suffix range "bytes=-100" and range_error serves the last 100 bytes instead of returning 416. Before the parser was rewritten onto detail::from_chars, std::stoll threw std::out_of_range on the same input, the catch arm added in 8f8761e for issue yhirose#705 returned false, and the request was answered with 416. The catch arm is still there but from_chars reports through an error code, so nothing reaches it any more. get_header_value_u64 and parse_port already reject an out-of-range value at their from_chars call sites; this was the remaining one that dropped the error. The last-byte-pos side is deliberately unchanged: -1 there is the documented RFC 9110 14.1.2 "remainder of the representation" value, so an oversized last-byte-pos stays accepted.
Parse the first-byte-pos straight into first, since a failed parse now returns before first is read, and fold the overflow test into the existing batch of rejected ranges. Also note on the last-byte-pos side why an overflow there deliberately keeps -1. Claude-Session: https://claude.ai/code/session_01JYPWKpbp4a881EdpEf2xSi
Owner
|
@youdie006 thank you for your contribution! |
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
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.
The bug
detail::parse_range_headeruses-1as the sentinel for "this range has nofirst-byte-pos" and only overwrites it when the parse succeeds
(
httplib.h:9049-9056):When
from_charsreturnsstd::errc::result_out_of_rangethe assignment isskipped,
firststays-1, and the(first == -1 && last == -1)guard a fewlines down does not fire because
lastwas parsed fine. The range reachesrange_erroras(-1, 100), which is exactly whatbytes=-100produces, sothe suffix-range branch at
httplib.h:9745-9748rewrites it to the last 100bytes and the server answers 206 with real content.
I built a small probe against a 1000-byte body (parse, then
range_error, thenthe emitted
Content-Range):The overflowing request is served byte-for-byte identically to
bytes=-100.This is a regression, not a missing check
Issue #705 ("[oss-fuzz] issue-26453 Invalid value of Range header") was the same
input class. Back then
std::stollthrewstd::out_of_range, and the fix in8f8761ec516db19b22657c872f696c2e667123f8wrapped the function body so thethrow unwound past every per-range branch:
falsemeantRangeNotSatisfiable_416athttplib.h:14340-14342. Thatcatch (...) { return false; }is still in the file today, butfrom_charsreports through an error code instead of throwing, so nothing reaches it and the
input now falls through into a suffix range.
The odd one out
Your own
get_header_value_u64athttplib.h:3430-3443already handles thisat its
from_charscall site, and its comment says why:So does
parse_portathttplib.h:831-837(if (r.ec != std::errc{} || ...) return false;).parse_range_headerwas the one remainingfrom_charscall site that droppedthe error. Given #2494 was settled with "I decided to use
detail::from_charsfor consistency", this makes that call site consistent too.
The fix
Treat a failed first-byte-pos parse the same way the function already treats an
invalid range, instead of letting
-1survive.The last-byte-pos side is deliberately not changed. There
-1is a realvalue, not a failure: RFC 9110 14.1.2 says a last-byte-pos greater than the
content length means the remainder of the representation, which is what the
comment at
httplib.h:9770-9781describes and whatrange_errorimplements.bytes=0-99999999999999999999therefore stays accepted as(0, -1).Tests
Two cases appended to
TEST(ParseHeaderValueTest, Range)-- the rejection, anda pin on the last-byte-pos behaviour that must not change.
Red/green plus mutation in both directions. I could not link the full
test/test.cchere because libcurl headers are not available in thisenvironment, so I extracted the
ParseHeaderValueTest.Rangebody verbatim intoa standalone TU compiled against the vendored gtest in
test/gtest/.httplib.hwas swapped by file copy and its md5 printed on every run, with a
touchbeforeeach build:
httplib.hmd543417965907311f276e55d910a73052d52fee63bbe19df497ca7000a42af3c4f43417965907311f276e55d910a73052db27e50a3f674824be9a5606b97e463a4The two failing sets are disjoint, so the second test genuinely pins the
RFC 9110 remainder behaviour rather than riding along with the first.
Other gates run locally:
python3 split.pyregenerates cleanly; the guard lands atout/httplib.cc:4405.g++ -std=c++11 -fsyntax-only -Wall -Wextra -Wtype-limits -Wshadowontest/include_httplib.cc: 0 warnings, same as the baseline.-fsanitize=address,undefined: passes.cd test && make style_checkwith clang-format 23.1.0 (the version pinned in.github/workflows/test.yaml): "All files are properly formatted."Notes
test/fuzzing/header_parser_fuzzer.cc:17-19does callparse_range_header, butit discards both the return value and the
rangesoutput, so a silent change ofparse result is invisible to it -- which is why this survived the fuzzer.
On 32-bit builds
ssize_tis 32-bit and the trigger drops tobytes=2147483648-2147483748; the repo runs.github/workflows/test-32bit.ymland has hit 32-bit range issues before (#1795, #2398).
Disclosure: this fix was found and prepared with AI assistance (Claude). The
probe, the red/green runs and every gate listed above were executed, not
inferred; the md5s and outputs above are from those runs.