File SQUID-2024_1.patch of Package squid.33029

Overview Repositories Revisions Requests Users Attributes Meta

File SQUID-2024_1.patch of Package squid.33029

commit 4658d0fc049738c2e6cd25fc0af10e820cf4c11a
Author: Alex Rousskov <rousskov@measurement-factory.com>
Date:   Tue Oct 31 11:35:02 2023 +0000

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".

Index: squid-5.7/src/http.cc
===================================================================
--- squid-5.7.orig/src/http.cc
+++ squid-5.7/src/http.cc
@@ -1235,18 +1235,27 @@ HttpStateData::readReply(const CommIoCbP
      * 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<size_t>(0, inBuf.spaceSize()));
+    bool canRead;
+    const auto moreDataPermission = canBufferMoreReplyBytes(&canRead);
+    if (!canRead) {
+        abortTransaction("ready to read required data, but the read buffer is full and cannot be drained");
+        return;
+    }
 
-    if (rd.size <= 0) {
+    const auto readSizeMax = maybeMakeSpaceAvailable(moreDataPermission);
+    // TODO: Move this logic inside maybeMakeSpaceAvailable():
+    const auto readSizeWanted = readSizeMax ? entry->bytesWanted(Range<size_t>(0, readSizeMax)) : 0;
+
+    if (readSizeWanted <= 0) {
         assert(entry->mem_obj);
         AsyncCall::Pointer nilCall;
         entry->mem_obj->delayRead(DeferredRead(readDelayed, this, CommRead(io.conn, NULL, 0, nilCall)));
         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())
@@ -1617,8 +1626,12 @@ HttpStateData::maybeReadVirginBody()
     if (!Comm::IsConnOpen(serverConnection) || fd_table[serverConnection->fd].closing())
         return;
 
-    if (!maybeMakeSpaceAvailable(false))
+    bool canRead;
+    canBufferMoreReplyBytes(&canRead);
+    if (!canRead) {
+        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)
@@ -1636,40 +1649,81 @@ 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 (!flags.headers_parsed)
+        return 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;
+    // 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().
+    // note: def value is 16KB
+    // const auto configurationPreferences = NaturalSum<size_t>(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<size_t>(Config.readAheadGap, SBuf::maxSize);
+}
+
+/// The maximum number of virgin reply bytes we may buffer before we violate
+/// the currently configured response buffering limits.
+/// \retval *canRead==false 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)
+size_t
+HttpStateData::canBufferMoreReplyBytes(bool *canRead) const
+{
+    *canRead = true;
+#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);
+	*canRead = false;
+        return 0; // 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)
Index: squid-5.7/src/http.h
===================================================================
--- squid-5.7.orig/src/http.h
+++ squid-5.7/src/http.h
@@ -112,16 +112,9 @@ private:
 
     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;
+    size_t canBufferMoreReplyBytes(bool *canRead) const;
+    size_t maybeMakeSpaceAvailable(size_t maxReadSize);
 
     // consuming request body
     virtual void handleMoreRequestBodyAvailable();

Places

File SQUID-2024_1.patch of Package squid.33029

Places