From 4658d0fc049738c2e6cd25fc0af10e820cf4c11a Mon Sep 17 00:00:00 2001 From: Markus Koschany Date: Tue, 5 Mar 2024 16:35:19 +0100 Subject: CVE-2024-25111 Origin: http://www.squid-cache.org/Versions/v6/SQUID-2024_1.patch Conflict: NA Reference: https://launchpad.net/ubuntu/+archive/primary/+sourcefiles/squid/4.10-1ubuntu1.10/squid_4.10-1ubuntu1.10.debian.tar.xz --- src/SquidMath.h | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- src/http.cc | 110 +++++++++++++++++++++++++---------- src/http.h | 15 ++--- 3 files changed, 256 insertions(+), 44 deletions(-) --- a/src/SquidMath.h +++ b/src/SquidMath.h @@ -1,13 +1,18 @@ /* - * Copyright (C) 1996-2019 The Squid Software Foundation and contributors + * Copyright (C) 1996-2023 The Squid Software Foundation and contributors * * Squid software is distributed under GPLv2+ license and includes * contributions from numerous individuals and organizations. * Please see the COPYING and CONTRIBUTORS files for details. */ -#ifndef _SQUID_SRC_SQUIDMATH_H -#define _SQUID_SRC_SQUIDMATH_H +#ifndef SQUID_SRC_SQUIDMATH_H +#define SQUID_SRC_SQUIDMATH_H + +#include +#include + +// TODO: Move to src/base/Math.h and drop the Math namespace /* Math functions we define locally for Squid */ namespace Math @@ -21,5 +26,164 @@ double doubleAverage(const double, const } // namespace Math -#endif /* _SQUID_SRC_SQUIDMATH_H */ +// If Sum() performance becomes important, consider using GCC and clang +// built-ins like __builtin_add_overflow() instead of manual overflow checks. + +/// detects a pair of unsigned types +/// reduces code duplication in declarations further below +template +using AllUnsigned = typename std::conditional< + std::is_unsigned::value && std::is_unsigned::value, + std::true_type, + std::false_type + >::type; + +// TODO: Replace with std::cmp_less() after migrating to C++20. +/// whether integer a is less than integer b, with correct overflow handling +template +constexpr bool +Less(const A a, const B b) { + // The casts below make standard C++ integer conversions explicit. They + // quell compiler warnings about signed/unsigned comparison. The first two + // lines exclude different-sign a and b, making the casts/comparison safe. + using AB = typename std::common_type::type; + return + (a >= 0 && b < 0) ? false : + (a < 0 && b >= 0) ? true : + /* (a >= 0) == (b >= 0) */ static_cast(a) < static_cast(b); +} + +/// ensure that T is supported by NaturalSum() and friends +template +constexpr void +AssertNaturalType() +{ + static_assert(std::numeric_limits::is_bounded, "std::numeric_limits::max() is meaningful"); + static_assert(std::numeric_limits::is_exact, "no silent loss of precision"); + static_assert(!std::is_enum::value, "no silent creation of non-enumerated values"); +} + +// TODO: Investigate whether this optimization can be expanded to [signed] types +// A and B when std::numeric_limits::is_modulo is true. +/// This IncreaseSumInternal() overload is optimized for speed. +/// \returns a non-overflowing sum of the two unsigned arguments (or nothing) +/// \prec both argument types are unsigned +template ::value, int> = 0> +std::optional +IncreaseSumInternal(const A a, const B b) { + // paranoid: AllUnsigned precondition established that already + static_assert(std::is_unsigned::value, "AllUnsigned dispatch worked for A"); + static_assert(std::is_unsigned::value, "AllUnsigned dispatch worked for B"); + + AssertNaturalType(); + AssertNaturalType(); + AssertNaturalType(); + + // we should only be called by IncreaseSum(); it forces integer promotion + static_assert(std::is_same::value, "a will not be promoted"); + static_assert(std::is_same::value, "b will not be promoted"); + // and without integer promotions, a sum of unsigned integers is unsigned + static_assert(std::is_unsigned::value, "a+b is unsigned"); + + // with integer promotions ruled out, a or b can only undergo integer + // conversion to the higher rank type (A or B, we do not know which) + using AB = typename std::common_type::type; + static_assert(std::is_same::value || std::is_same::value, "no unexpected conversions"); + static_assert(std::is_same::value, "lossless assignment"); + const AB sum = a + b; + + static_assert(std::numeric_limits::is_modulo, "we can detect overflows"); + // 1. modulo math: overflowed sum is smaller than any of its operands + // 2. the sum may overflow S (i.e. the return base type) + // We do not need Less() here because we compare promoted unsigned types. + return (sum >= a && sum <= std::numeric_limits::max()) ? + std::optional(sum) : std::optional(); +} + +/// This IncreaseSumInternal() overload supports a larger variety of types. +/// \returns a non-overflowing sum of the two arguments (or nothing) +/// \returns nothing if at least one of the arguments is negative +/// \prec at least one of the argument types is signed +template ::value, int> = 0> +std::optional constexpr +IncreaseSumInternal(const A a, const B b) { + AssertNaturalType(); + AssertNaturalType(); + AssertNaturalType(); + + // we should only be called by IncreaseSum() that does integer promotion + static_assert(std::is_same::value, "a will not be promoted"); + static_assert(std::is_same::value, "b will not be promoted"); + + return + // We could support a non-under/overflowing sum of negative numbers, but + // our callers use negative values specially (e.g., for do-not-use or + // do-not-limit settings) and are not supposed to do math with them. + (a < 0 || b < 0) ? std::optional() : + // To avoid undefined behavior of signed overflow, we must not compute + // the raw a+b sum if it may overflow. When A is not B, a or b undergoes + // (safe for non-negatives) integer conversion in these expressions, so + // we do not know the resulting a+b type AB and its maximum. We must + // also detect subsequent casting-to-S overflows. + // Overflow condition: (a + b > maxAB) or (a + b > maxS). + // A is an integer promotion of S, so maxS <= maxA <= maxAB. + // Since maxS <= maxAB, it is sufficient to just check: a + b > maxS, + // which is the same as the overflow-safe condition here: maxS - a < b. + // Finally, (maxS - a) cannot overflow because a is not negative and + // cannot underflow because a is a promotion of s: 0 <= a <= maxS. + Less(std::numeric_limits::max() - a, b) ? std::optional() : + std::optional(a + b); +} + +/// argument pack expansion termination for IncreaseSum() +template +std::optional +IncreaseSum(const S s, const T t) +{ + // Force (always safe) integer promotions now, to give std::enable_if_t<> + // promoted types instead of entering IncreaseSumInternal(s,t) + // but getting a _signed_ promoted value of s or t in s + t. + return IncreaseSumInternal(+s, +t); +} + +/// \returns a non-overflowing sum of the arguments (or nothing) +template +std::optional +IncreaseSum(const S sum, const T t, const Args... args) { + if (const auto head = IncreaseSum(sum, t)) { + return IncreaseSum(head.value(), args...); + } else { + // std::optional() triggers bogus -Wmaybe-uninitialized warnings in GCC v10.3 + return std::nullopt; + } +} + +/// \returns an exact, non-overflowing sum of the arguments (or nothing) +template +std::optional +NaturalSum(const Args... args) { + return IncreaseSum(0, args...); +} + +/// Safely resets the given variable to NaturalSum() of the given arguments. +/// If the sum overflows, resets to variable's maximum possible value. +/// \returns the new variable value (like an assignment operator would) +template +S +SetToNaturalSumOrMax(S &var, const Args... args) +{ + var = NaturalSum(args...).value_or(std::numeric_limits::max()); + return var; +} + +/// converts a given non-negative integer into an integer of a given type +/// without loss of information or undefined behavior +template +Result +NaturalCast(const Source s) +{ + return NaturalSum(s).value(); +} + +#endif /* SQUID_SRC_SQUIDMATH_H */ --- a/src/http.cc +++ b/src/http.cc @@ -57,6 +57,7 @@ #include "StrList.h" #include "tools.h" #include "util.h" +#include "SquidMath.h" #if USE_AUTH #include "auth/UserRequest.h" @@ -1156,18 +1157,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(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) { 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()) @@ -1522,8 +1532,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) @@ -1541,40 +1553,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 +{ + 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 { - // 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 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) --- 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; @@ -107,16 +109,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; + std::optional canBufferMoreReplyBytes() const; + size_t maybeMakeSpaceAvailable(size_t maxReadSize); // consuming request body virtual void handleMoreRequestBodyAvailable();