From a2ed5e2159ddcabf0746a45a92fc4ace6ece3576 Mon Sep 17 00:00:00 2001 From: xinghe Date: Thu, 7 Mar 2024 08:34:49 +0000 Subject: [PATCH] fix CVE-2024-25111 (cherry picked from commit d029aaf1da0fc9f832fe2cc46f577a86e451eb7c) --- backport-CVE-2024-25111.patch | 248 ++++++++++++++++++++++++++++++++++ squid.spec | 9 +- 2 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 backport-CVE-2024-25111.patch diff --git a/backport-CVE-2024-25111.patch b/backport-CVE-2024-25111.patch new file mode 100644 index 0000000..25de47e --- /dev/null +++ b/backport-CVE-2024-25111.patch @@ -0,0 +1,248 @@ +From 4658d0fc049738c2e6cd25fc0af10e820cf4c11a Mon Sep 17 00:00:00 2001 +From: Alex Rousskov +Date: Tue, 31 Oct 2023 11:35:02 +0000 +Subject: [PATCH] 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". + +Conflict: NA +Reference: https://github.com/squid-cache/squid/commit/4658d0fc049738c2e6cd25fc0af10e820cf4c11a +--- + src/http.cc | 109 +++++++++++++++++++++++++++++++++++++--------------- + src/http.h | 15 +++----- + 2 files changed, 84 insertions(+), 40 deletions(-) + +diff --git a/src/http.cc b/src/http.cc +index 138c845c7b0..0829c25142f 100644 +--- a/src/http.cc ++++ b/src/http.cc +@@ -54,6 +54,7 @@ + #include "RefreshPattern.h" + #include "rfc1738.h" + #include "SquidConfig.h" ++#include "SquidMath.h" + #include "StatCounters.h" + #include "Store.h" + #include "StrList.h" +@@ -1200,16 +1201,24 @@ HttpStateData::readReply(const CommIoCbParams &io) + * Plus, it breaks our lame *HalfClosed() detection + */ + +- Must(maybeMakeSpaceAvailable(true)); +- CommIoCbParams rd(this); // will be expanded with ReadNow results +- rd.conn = io.conn; +- rd.size = entry->bytesWanted(Range(0, inBuf.spaceSize())); ++ const auto moreDataPermission = canBufferMoreReplyBytes(); ++ if (!moreDataPermission) { ++ abortTransaction("ready to read required data, but the read buffer is full and cannot be drained"); ++ return; ++ } ++ ++ const auto readSizeMax = maybeMakeSpaceAvailable(moreDataPermission.value()); ++ // TODO: Move this logic inside maybeMakeSpaceAvailable(): ++ const auto readSizeWanted = readSizeMax ? entry->bytesWanted(Range(0, readSizeMax)) : 0; + +- if (rd.size <= 0) { ++ if (readSizeWanted <= 0) { + delayRead(); + return; + } + ++ CommIoCbParams rd(this); // will be expanded with ReadNow results ++ rd.conn = io.conn; ++ rd.size = readSizeWanted; + switch (Comm::ReadNow(rd, inBuf)) { + case Comm::INPROGRESS: + if (inBuf.isEmpty()) +@@ -1591,8 +1600,10 @@ HttpStateData::maybeReadVirginBody() + if (!Comm::IsConnOpen(serverConnection) || fd_table[serverConnection->fd].closing()) + return; + +- if (!maybeMakeSpaceAvailable(false)) ++ if (!canBufferMoreReplyBytes()) { ++ abortTransaction("more response bytes required, but the read buffer is full and cannot be drained"); + return; ++ } + + // XXX: get rid of the do_next_read flag + // check for the proper reasons preventing read(2) +@@ -1610,40 +1621,78 @@ HttpStateData::maybeReadVirginBody() + Comm::Read(serverConnection, call); + } + +-bool +-HttpStateData::maybeMakeSpaceAvailable(bool doGrow) ++/// Desired inBuf capacity based on various capacity preferences/limits: ++/// * a smaller buffer may not hold enough for look-ahead header/body parsers; ++/// * a smaller buffer may result in inefficient tiny network reads; ++/// * a bigger buffer may waste memory; ++/// * a bigger buffer may exceed SBuf storage capabilities (SBuf::maxSize); ++size_t ++HttpStateData::calcReadBufferCapacityLimit() const + { +- // how much we are allowed to buffer +- const int limitBuffer = (flags.headers_parsed ? Config.readAheadGap : Config.maxReplyHeaderSize); +- +- if (limitBuffer < 0 || inBuf.length() >= (SBuf::size_type)limitBuffer) { +- // when buffer is at or over limit already +- debugs(11, 7, "will not read up to " << limitBuffer << ". buffer has (" << inBuf.length() << "/" << inBuf.spaceSize() << ") from " << serverConnection); +- debugs(11, DBG_DATA, "buffer has {" << inBuf << "}"); +- // Process next response from buffer +- processReply(); +- return false; ++ if (!flags.headers_parsed) ++ return Config.maxReplyHeaderSize; ++ ++ // XXX: Our inBuf is not used to maintain the read-ahead gap, and using ++ // Config.readAheadGap like this creates huge read buffers for large ++ // read_ahead_gap values. TODO: Switch to using tcp_recv_bufsize as the ++ // primary read buffer capacity factor. ++ // ++ // TODO: Cannot reuse throwing NaturalCast() here. Consider removing ++ // .value() dereference in NaturalCast() or add/use NaturalCastOrMax(). ++ const auto configurationPreferences = NaturalSum(Config.readAheadGap).value_or(SBuf::maxSize); ++ ++ // TODO: Honor TeChunkedParser look-ahead and trailer parsing requirements ++ // (when explicit configurationPreferences are set too low). ++ ++ return std::min(configurationPreferences, SBuf::maxSize); ++} ++ ++/// The maximum number of virgin reply bytes we may buffer before we violate ++/// the currently configured response buffering limits. ++/// \retval std::nullopt means that no more virgin response bytes can be read ++/// \retval 0 means that more virgin response bytes may be read later ++/// \retval >0 is the number of bytes that can be read now (subject to other constraints) ++std::optional ++HttpStateData::canBufferMoreReplyBytes() const ++{ ++#if USE_ADAPTATION ++ // If we do not check this now, we may say the final "no" prematurely below ++ // because inBuf.length() will decrease as adaptation drains buffered bytes. ++ if (responseBodyBuffer) { ++ debugs(11, 3, "yes, but waiting for adaptation to drain read buffer"); ++ return 0; // yes, we may be able to buffer more (but later) ++ } ++#endif ++ ++ const auto maxCapacity = calcReadBufferCapacityLimit(); ++ if (inBuf.length() >= maxCapacity) { ++ debugs(11, 3, "no, due to a full buffer: " << inBuf.length() << '/' << inBuf.spaceSize() << "; limit: " << maxCapacity); ++ return std::nullopt; // no, configuration prohibits buffering more + } + ++ const auto maxReadSize = maxCapacity - inBuf.length(); // positive ++ debugs(11, 7, "yes, may read up to " << maxReadSize << " into " << inBuf.length() << '/' << inBuf.spaceSize()); ++ return maxReadSize; // yes, can read up to this many bytes (subject to other constraints) ++} ++ ++/// prepare read buffer for reading ++/// \return the maximum number of bytes the caller should attempt to read ++/// \retval 0 means that the caller should delay reading ++size_t ++HttpStateData::maybeMakeSpaceAvailable(const size_t maxReadSize) ++{ + // how much we want to read +- const size_t read_size = calcBufferSpaceToReserve(inBuf.spaceSize(), (limitBuffer - inBuf.length())); ++ const size_t read_size = calcBufferSpaceToReserve(inBuf.spaceSize(), maxReadSize); + +- if (!read_size) { ++ if (read_size < 2) { + debugs(11, 7, "will not read up to " << read_size << " into buffer (" << inBuf.length() << "/" << inBuf.spaceSize() << ") from " << serverConnection); +- return false; ++ return 0; + } + +- // just report whether we could grow or not, do not actually do it +- if (doGrow) +- return (read_size >= 2); +- + // we may need to grow the buffer + inBuf.reserveSpace(read_size); +- debugs(11, 8, (!flags.do_next_read ? "will not" : "may") << +- " read up to " << read_size << " bytes info buf(" << inBuf.length() << "/" << inBuf.spaceSize() << +- ") from " << serverConnection); +- +- return (inBuf.spaceSize() >= 2); // only read if there is 1+ bytes of space available ++ debugs(11, 7, "may read up to " << read_size << " bytes info buffer (" << inBuf.length() << "/" << inBuf.spaceSize() << ") from " << serverConnection); ++ return read_size; + } + + /// called after writing the very last request byte (body, last-chunk, etc) +diff --git a/src/http.h b/src/http.h +index 7baffe36499..4f59af90ba8 100644 +--- a/src/http.h ++++ b/src/http.h +@@ -15,6 +15,8 @@ + #include "http/StateFlags.h" + #include "sbuf/SBuf.h" + ++#include ++ + class FwdState; + class HttpHeader; + class String; +@@ -114,16 +116,9 @@ class HttpStateData : public Client + + void abortTransaction(const char *reason) { abortAll(reason); } // abnormal termination + +- /** +- * determine if read buffer can have space made available +- * for a read. +- * +- * \param grow whether to actually expand the buffer +- * +- * \return whether the buffer can be grown to provide space +- * regardless of whether the grow actually happened. +- */ +- bool maybeMakeSpaceAvailable(bool grow); ++ size_t calcReadBufferCapacityLimit() const; ++ std::optional canBufferMoreReplyBytes() const; ++ size_t maybeMakeSpaceAvailable(size_t maxReadSize); + + // consuming request body + virtual void handleMoreRequestBodyAvailable(); diff --git a/squid.spec b/squid.spec index f44b0fa..d9b8615 100644 --- a/squid.spec +++ b/squid.spec @@ -2,7 +2,7 @@ Name: squid Version: 6.6 -Release: 1 +Release: 2 Summary: The Squid proxy caching server Epoch: 7 License: GPLv2+ and (LGPLv2+ and MIT and BSD and Public Domain) @@ -22,6 +22,7 @@ Patch1: squid-3.1.0.9-location.patch Patch2: squid-3.0.STABLE1-perlpath.patch Patch3: backport-squid-6.1-symlink-lang-err.patch Patch4: backport-squid-crash-half-closed.patch +Patch5: backport-CVE-2024-25111.patch Requires: bash Requires: httpd-filesystem @@ -244,6 +245,12 @@ fi chgrp squid /var/cache/samba/winbindd_privileged >/dev/null 2>&1 || : %changelog +* Thu Mar 07 2024 xinghe - 7:6.6-2 +- Type:cves +- ID:CVE-2024-25111 +- SUG:NA +- DESC:fix CVE-2024-25111 + * Tue Dec 26 2023 xinghe - 7:6.6-1 - Type:requirements - ID:NA -- Gitee