Reject Range and Content-Range with INT64_MAX last-pos

[kalt2212]

I hit this while fuzzing header parsing with UBSan on current
master.

Sending Range: bytes=0-9223372036854775807 makes
HttpHdrRangeSpec::parseInit() compute last_pos + 1 with
last_pos == INT64_MAX. That is signed overflow. UBSan aborts the
worker on it, so any client can crash the process with one request
header. In a normal build it wraps quietly and the range handling
silently degenerates.

The parser (httpHeaderParseOffset) accepts exactly INT64_MAX and
rejects anything bigger, so INT64_MAX is the one value that reaches
the addition and breaks it.

I verified it by compiling the real translation units with
-fsanitize=address,undefined -fno-sanitize-recover=all and calling
HttpHdrRange::ParseCreate("bytes=0-9223372036854775807") directly:

HttpHdrRange.cc:99: runtime error: signed integer overflow:
9223372036854775807 + 1 cannot be represented in type 'long int'

The same pattern exists in httpHdrRangeRespSpecParseInit() for the
Content-Range response header (HttpHdrContRange.cc), so I fixed
both. The fix just rejects last_pos == INT64_MAX before the + 1;
to prevent the overflow, and
bytes=0-9223372036854775806 still parses fine afterwards.

[rousskov]

Thank you for improving this code.

Since this is your first contribution to Squid (according to GitHub), please add your record to the CONTRIBUTORS file to pass our automated CI checks (or ask for manual help to bypass that check).

N.B. There is no need to squash commits or rebase your PR branch as you address change requests or resolve merge conflicts. Just push new commits as needed. They will be automatically squashed when staging and merging your PR.

[rousskov]

Let's avoid unnecessary and arguably incorrect claims about "real objects":

Suggested change

	// size_diff() below computes last_pos + 1, which overflows when last_pos
	// is INT64_MAX. No real object can be that large, so reject the spec.
	// prevent `last_pos + 1` overflows

Please adjust PR description to remove similar claims about offsets "reality". Just focus on overflow prevention.

[rousskov]

Since the value is syntactically valid and can even be parsed by Squid, let's adjust this diagnostics:

Suggested change

	debugs(68, 2, "invalid (last-byte-pos too large) resp-range-spec near: '" << field << "'");
	debugs(68, 2, "unsupported (last-byte-pos too large) resp-range-spec near: '" << field << "'");

[rousskov]

Please avoid type duplication (and conditionally present constants). An expression similar to the suggestion below should work. If we find ourselves doing this often, we will declare an AtMax() or similar wrapper.

Suggested change

	if (last_pos == INT64_MAX) {
	if (last_pos == std::numeric_limits<decltype(last_pos)>::max()) {

You may need to add #include <limits>. Do it after #include "HttpHeaderTools.h", leaving an empty line between them.

[rousskov]

Please apply my src/HttpHdrContRange.cc change requests to this file as well.

[yadij]

FYI, reduced the title length even further. It would hit these same Anubis complain on backport when that long.

[kalt2212]

Thanks for the review — all four points are addressed in the new commit:

• std::numeric_limits<decltype(last_pos)>::max() instead of INT64_MAX, with #include <limits> after #include "HttpHeaderTools.h" in both TUs;
• diagnostics now say "unsupported" instead of "invalid";
• the object-size claims are gone from the comments and from the PR description.

One thing I could not do: I cannot find a CREDENTIALS file anywhere in the tree (checked the repo root, .github, and current master) — where should the record go, or did you mean CONTRIBUTORS? Happy to take the manual bypass in the meantime.

[rousskov]

One thing I could not do: I cannot find a CREDENTIALS file anywhere in the tree — where should the record go, or did you mean CONTRIBUTORS?

Yes, please add yourself to CONTRIBUTORS. Sorry about that typo! I corrected it, but it was too late.

[kalt2212]

Done — added myself to CONTRIBUTORS in bd63c7e. Thanks for clarifying!

[rousskov]

FYI, reduced the title length even further. It would hit these same Anubis complain on backport when that long.

Just to clarify: Anubis does not differentiate backports from original work: If Anubis is happy with the original PR title, it will accept that same title in the backport.

Anubis does reject excessively long PR tittles, and we have started to see those rejections in backported PRs a few months(?) ago. AFAICT, what we observe is the effect of backporting software changing the original PR title (by adding a suffix with the original PR number). Those extra (and, IMO, unwanted) characters may indeed violate the title character limit, triggering a rejection (among other bad side effects!). The correct solution here is to fix that backporting software rather than to make the original PR titles shorter than they need to be.

[rousskov]

I adjusted PR title to emphasize that we are rejecting these Range headers. Perhaps we should say "Range and Content-Range" instead of just "Range" because we are rejecting both headers AFAICT?

[rousskov]

Please keep the human-friendly name but use one of the two emails that our CI sees for this PR. You can see those email inside the expanded log of the failed GitHub Action.

[kalt2212]

Agreed — both parsers reject these now (HttpHdrRange and HttpHdrContRange), so "Range and Content-Range" is accurate. I updated the title accordingly, keeping it short.