packages/squid
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
squid/backport-CVE-2024-25111.patch
xh 953b2e48d4 fix CVE-2024-25111 
(cherry picked from commit 8fcee556bde3cd09cbd5575106d03db6a28eaf77)
2024-09-23 11:17:28 +08:00

395 lines
16 KiB
Diff
Raw Blame History

From 4658d0fc049738c2e6cd25fc0af10e820cf4c11a Mon Sep 17 00:00:00 2001
From: Markus Koschany <apo@debian.org>
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 <limits>
+#include <optional>
+
+// 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 <typename T, typename U>
+using AllUnsigned = typename std::conditional<
+ std::is_unsigned<T>::value && std::is_unsigned<U>::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 <typename A, typename B>
+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<A, B>::type;
+ return
+ (a >= 0 && b < 0) ? false :
+ (a < 0 && b >= 0) ? true :
+ /* (a >= 0) == (b >= 0) */ static_cast<AB>(a) < static_cast<AB>(b);
+}
+
+/// ensure that T is supported by NaturalSum() and friends
+template<typename T>
+constexpr void
+AssertNaturalType()
+{
+ static_assert(std::numeric_limits<T>::is_bounded, "std::numeric_limits<T>::max() is meaningful");
+ static_assert(std::numeric_limits<T>::is_exact, "no silent loss of precision");
+ static_assert(!std::is_enum<T>::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<decltype(A(0)+B(0))>::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 <typename S, typename A, typename B, std::enable_if_t<AllUnsigned<A,B>::value, int> = 0>
+std::optional<S>
+IncreaseSumInternal(const A a, const B b) {
+ // paranoid: AllUnsigned<A,B> precondition established that already
+ static_assert(std::is_unsigned<A>::value, "AllUnsigned dispatch worked for A");
+ static_assert(std::is_unsigned<B>::value, "AllUnsigned dispatch worked for B");
+
+ AssertNaturalType<S>();
+ AssertNaturalType<A>();
+ AssertNaturalType<B>();
+
+ // we should only be called by IncreaseSum(); it forces integer promotion
+ static_assert(std::is_same<A, decltype(+a)>::value, "a will not be promoted");
+ static_assert(std::is_same<B, decltype(+b)>::value, "b will not be promoted");
+ // and without integer promotions, a sum of unsigned integers is unsigned
+ static_assert(std::is_unsigned<decltype(a+b)>::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<A, B>::type;
+ static_assert(std::is_same<AB, A>::value || std::is_same<AB, B>::value, "no unexpected conversions");
+ static_assert(std::is_same<AB, decltype(a+b)>::value, "lossless assignment");
+ const AB sum = a + b;
+
+ static_assert(std::numeric_limits<AB>::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<S>::max()) ?
+ std::optional<S>(sum) : std::optional<S>();
+}
+
+/// 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 <typename S, typename A, typename B, std::enable_if_t<!AllUnsigned<A,B>::value, int> = 0>
+std::optional<S> constexpr
+IncreaseSumInternal(const A a, const B b) {
+ AssertNaturalType<S>();
+ AssertNaturalType<A>();
+ AssertNaturalType<B>();
+
+ // we should only be called by IncreaseSum() that does integer promotion
+ static_assert(std::is_same<A, decltype(+a)>::value, "a will not be promoted");
+ static_assert(std::is_same<B, decltype(+b)>::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<S>() :
+ // 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<S>::max() - a, b) ? std::optional<S>() :
+ std::optional<S>(a + b);
+}
+
+/// argument pack expansion termination for IncreaseSum<S, T, Args...>()
+template <typename S, typename T>
+std::optional<S>
+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<AllUnsigned>(s,t)
+ // but getting a _signed_ promoted value of s or t in s + t.
+ return IncreaseSumInternal<S>(+s, +t);
+}
+
+/// \returns a non-overflowing sum of the arguments (or nothing)
+template <typename S, typename T, typename... Args>
+std::optional<S>
+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<S>() triggers bogus -Wmaybe-uninitialized warnings in GCC v10.3
+ return std::nullopt;
+ }
+}
+
+/// \returns an exact, non-overflowing sum of the arguments (or nothing)
+template <typename SummationType, typename... Args>
+std::optional<SummationType>
+NaturalSum(const Args... args) {
+ return IncreaseSum<SummationType>(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 <typename S, typename... Args>
+S
+SetToNaturalSumOrMax(S &var, const Args... args)
+{
+ var = NaturalSum<S>(args...).value_or(std::numeric_limits<S>::max());
+ return var;
+}
+
+/// converts a given non-negative integer into an integer of a given type
+/// without loss of information or undefined behavior
+template <typename Result, typename Source>
+Result
+NaturalCast(const Source s)
+{
+ return NaturalSum<Result>(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<size_t>(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<size_t>(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<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>(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<size_t>
+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 <optional>
+
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<size_t> canBufferMoreReplyBytes() const;
+ size_t maybeMakeSpaceAvailable(size_t maxReadSize);
// consuming request body
virtual void handleMoreRequestBodyAvailable();
Reference in New Issue View Git Blame Copy Permalink