]> git.ipfire.org Git - thirdparty/squid.git/commit
Fix infinite recursion when parsing HTTP chunks (#1553)
authorAlex Rousskov <rousskov@measurement-factory.com>
Tue, 31 Oct 2023 11:35:02 +0000 (11:35 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Tue, 31 Oct 2023 11:35:06 +0000 (11:35 +0000)
commit50c5af88bd74208103722f87593a974bb0f0b8e9
tree2dfd5c233d552928bbdbe55d4205afbf395c1382
parent1f16bc27dd4c595628579110dae2082e59d75c29
Fix infinite recursion when parsing HTTP chunks (#1553)

This change stops infinite HttpStateData recursion with at-max-capacity
inBuf. Such inBuf prevents progress in the following call chain:

* processReply()
* processReplyBody() and decodeAndWriteReplyBody()
* maybeReadVirginBody()
* maybeMakeSpaceAvailable() -- tries but fails to quit processing
* processReply()

HttpStateData::maybeMakeSpaceAvailable() no longer calls processReply(),
preventing recursion.

maybeReadVirginBody() now aborts transactions that would otherwise get
stalled due to full read buffer at its maximum capacity. This change
requires that all maybeReadVirginBody() callers do actually need more
response data to make progress. AFAICT, that (natural) invariant holds.

We moved transaction stalling check from maybeMakeSpaceAvailable() into
its previous callers. Without that move, maybeMakeSpaceAvailable() would
have to handle both abortTransaction() and delayRead() cases. Besides
increased code complexity, that would trigger some premature delayRead()
calls (at maybeReadVirginBody() time). Deciding whether to delay socket
reads is complicated, the delay mechanism is expensive, and delaying may
become unnecessary by the time the socket becomes readable, so it is
best to continue to only delayRead() at readReply() time, when there is
no other choice left.

maybeReadVirginBody() mishandled cases where progress was possible, but
not _immediately_ -- it did nothing in those cases, probably stalling
transactions when maybeMakeSpaceAvailable() returned false but did not
call processReply(). This is now fixed: maybeReadVirginBody() now starts
waiting for the socket to be ready for reading in those cases,
effectively passing control to readReply() that handles them.

maybeReadVirginBody() prematurely grew buffer for future socket reads.
As a (positive) side effect of the above refactoring, we now delay
buffer growth until the actual read(2) time, which is best for
performance. Most likely, this premature buffer growth was an accident:
maybeReadVirginBody() correctly called maybeMakeSpaceAvailable() with
doGrow set to false. However, maybeMakeSpaceAvailable() misinterpreted
doGrow as a "do not actually do it" parameter. That bug is now gone.

This recursion bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/
where it was filed as "Chunked Encoding Stack Overflow".
src/http.cc
src/http.h