MSVSphere Packaging Team
3 months ago
parent
ffcfa72f66
commit
18de7973d1
GPG Key ID:
B2B0B9F29E528FE8
@ -0,0 +1,30 @@
|
|||||||
|
commit 8fcff9c09824b18628f010d26a04247f6a6cbcb8
|
||||||
|
Author: Alex Rousskov <rousskov@measurement-factory.com>
|
||||||
|
Date: Sun Nov 12 09:33:20 2023 +0000
|
||||||
|
|
||||||
|
Do not update StoreEntry expiration after errorAppendEntry() (#1580)
|
||||||
|
|
||||||
|
errorAppendEntry() is responsible for setting entry expiration times,
|
||||||
|
which it does by calling StoreEntry::storeErrorResponse() that calls
|
||||||
|
StoreEntry::negativeCache().
|
||||||
|
|
||||||
|
This change was triggered by a vulnerability report by Joshua Rogers at
|
||||||
|
https://megamansec.github.io/Squid-Security-Audit/cache-uaf.html where
|
||||||
|
it was filed as "Use-After-Free in Cache Manager Errors". The reported
|
||||||
|
"use after free" vulnerability was unknowingly addressed by 2022 commit
|
||||||
|
1fa761a that removed excessively long "reentrant" store_client calls
|
||||||
|
responsible for the disappearance of the properly locked StoreEntry in
|
||||||
|
this (and probably other) contexts.
|
||||||
|
|
||||||
|
diff --git a/src/cache_manager.cc b/src/cache_manager.cc
|
||||||
|
index 61c7f65be..65bf22dd0 100644
|
||||||
|
--- a/src/cache_manager.cc
|
||||||
|
+++ b/src/cache_manager.cc
|
||||||
|
@@ -326,7 +326,6 @@ CacheManager::start(const Comm::ConnectionPointer &client, HttpRequest *request,
|
||||||
|
err->url = xstrdup(entry->url());
|
||||||
|
err->detailError(new ExceptionErrorDetail(Here().id()));
|
||||||
|
errorAppendEntry(entry, err);
|
||||||
|
- entry->expires = squid_curtime;
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
@ -0,0 +1,192 @@
|
|||||||
|
diff --git a/src/http.cc b/src/http.cc
|
||||||
|
index 98e3969..8b55bf3 100644
|
||||||
|
--- a/src/http.cc
|
||||||
|
+++ b/src/http.cc
|
||||||
|
@@ -54,6 +54,7 @@
|
||||||
|
#include "rfc1738.h"
|
||||||
|
#include "SquidConfig.h"
|
||||||
|
#include "SquidTime.h"
|
||||||
|
+#include "SquidMath.h"
|
||||||
|
#include "StatCounters.h"
|
||||||
|
#include "Store.h"
|
||||||
|
#include "StrList.h"
|
||||||
|
@@ -1235,18 +1236,26 @@ 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<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())
|
||||||
|
@@ -1617,8 +1626,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)
|
||||||
|
@@ -1636,40 +1647,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<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
|
||||||
|
+{
|
||||||
|
+#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 e70cd7e..f7ed40d 100644
|
||||||
|
--- 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;
|
||||||
|
class String;
|
||||||
|
@@ -112,16 +114,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();
|
||||||
@ -0,0 +1,105 @@
|
|||||||
|
diff --git a/src/SquidString.h b/src/SquidString.h
|
||||||
|
index e36cd27..ea613ad 100644
|
||||||
|
--- a/src/SquidString.h
|
||||||
|
+++ b/src/SquidString.h
|
||||||
|
@@ -140,7 +140,16 @@ private:
|
||||||
|
|
||||||
|
size_type len_ = 0; /* current length */
|
||||||
|
|
||||||
|
- static const size_type SizeMax_ = 65535; ///< 64K limit protects some fixed-size buffers
|
||||||
|
+ /// An earlier 64KB limit was meant to protect some fixed-size buffers, but
|
||||||
|
+ /// (a) we do not know where those buffers are (or whether they still exist)
|
||||||
|
+ /// (b) too many String users unknowingly exceeded that limit and asserted.
|
||||||
|
+ /// We are now using a larger limit to reduce the number of (b) cases,
|
||||||
|
+ /// especially cases where "compact" lists of items grow 50% in size when we
|
||||||
|
+ /// convert them to canonical form. The new limit is selected to withstand
|
||||||
|
+ /// concatenation and ~50% expansion of two HTTP headers limited by default
|
||||||
|
+ /// request_header_max_size and reply_header_max_size settings.
|
||||||
|
+ static const size_type SizeMax_ = 3*64*1024 - 1;
|
||||||
|
+
|
||||||
|
/// returns true after increasing the first argument by extra if the sum does not exceed SizeMax_
|
||||||
|
static bool SafeAdd(size_type &base, size_type extra) { if (extra <= SizeMax_ && base <= SizeMax_ - extra) { base += extra; return true; } return false; }
|
||||||
|
|
||||||
|
diff --git a/src/cache_cf.cc b/src/cache_cf.cc
|
||||||
|
index cb746dc..c4ade96 100644
|
||||||
|
--- a/src/cache_cf.cc
|
||||||
|
+++ b/src/cache_cf.cc
|
||||||
|
@@ -950,6 +950,18 @@ configDoConfigure(void)
|
||||||
|
(uint32_t)Config.maxRequestBufferSize, (uint32_t)Config.maxRequestHeaderSize);
|
||||||
|
}
|
||||||
|
|
||||||
|
+ // Warn about the dangers of exceeding String limits when manipulating HTTP
|
||||||
|
+ // headers. Technically, we do not concatenate _requests_, so we could relax
|
||||||
|
+ // their check, but we keep the two checks the same for simplicity sake.
|
||||||
|
+ const auto safeRawHeaderValueSizeMax = (String::SizeMaxXXX()+1)/3;
|
||||||
|
+ // TODO: static_assert(safeRawHeaderValueSizeMax >= 64*1024); // no WARNINGs for default settings
|
||||||
|
+ if (Config.maxRequestHeaderSize > safeRawHeaderValueSizeMax)
|
||||||
|
+ debugs(3, DBG_CRITICAL, "WARNING: Increasing request_header_max_size beyond " << safeRawHeaderValueSizeMax <<
|
||||||
|
+ " bytes makes Squid more vulnerable to denial-of-service attacks; configured value: " << Config.maxRequestHeaderSize << " bytes");
|
||||||
|
+ if (Config.maxReplyHeaderSize > safeRawHeaderValueSizeMax)
|
||||||
|
+ debugs(3, DBG_CRITICAL, "WARNING: Increasing reply_header_max_size beyond " << safeRawHeaderValueSizeMax <<
|
||||||
|
+ " bytes makes Squid more vulnerable to denial-of-service attacks; configured value: " << Config.maxReplyHeaderSize << " bytes");
|
||||||
|
+
|
||||||
|
/*
|
||||||
|
* Disable client side request pipelining if client_persistent_connections OFF.
|
||||||
|
* Waste of resources queueing any pipelined requests when the first will close the connection.
|
||||||
|
diff --git a/src/cf.data.pre b/src/cf.data.pre
|
||||||
|
index 67a66b0..61a66f1 100644
|
||||||
|
--- a/src/cf.data.pre
|
||||||
|
+++ b/src/cf.data.pre
|
||||||
|
@@ -6489,11 +6489,14 @@ TYPE: b_size_t
|
||||||
|
DEFAULT: 64 KB
|
||||||
|
LOC: Config.maxRequestHeaderSize
|
||||||
|
DOC_START
|
||||||
|
- This specifies the maximum size for HTTP headers in a request.
|
||||||
|
- Request headers are usually relatively small (about 512 bytes).
|
||||||
|
- Placing a limit on the request header size will catch certain
|
||||||
|
- bugs (for example with persistent connections) and possibly
|
||||||
|
- buffer-overflow or denial-of-service attacks.
|
||||||
|
+ This directives limits the header size of a received HTTP request
|
||||||
|
+ (including request-line). Increasing this limit beyond its 64 KB default
|
||||||
|
+ exposes certain old Squid code to various denial-of-service attacks. This
|
||||||
|
+ limit also applies to received FTP commands.
|
||||||
|
+
|
||||||
|
+ This limit has no direct affect on Squid memory consumption.
|
||||||
|
+
|
||||||
|
+ Squid does not check this limit when sending requests.
|
||||||
|
DOC_END
|
||||||
|
|
||||||
|
NAME: reply_header_max_size
|
||||||
|
@@ -6502,11 +6505,14 @@ TYPE: b_size_t
|
||||||
|
DEFAULT: 64 KB
|
||||||
|
LOC: Config.maxReplyHeaderSize
|
||||||
|
DOC_START
|
||||||
|
- This specifies the maximum size for HTTP headers in a reply.
|
||||||
|
- Reply headers are usually relatively small (about 512 bytes).
|
||||||
|
- Placing a limit on the reply header size will catch certain
|
||||||
|
- bugs (for example with persistent connections) and possibly
|
||||||
|
- buffer-overflow or denial-of-service attacks.
|
||||||
|
+ This directives limits the header size of a received HTTP response
|
||||||
|
+ (including status-line). Increasing this limit beyond its 64 KB default
|
||||||
|
+ exposes certain old Squid code to various denial-of-service attacks. This
|
||||||
|
+ limit also applies to FTP command responses.
|
||||||
|
+
|
||||||
|
+ Squid also checks this limit when loading hit responses from disk cache.
|
||||||
|
+
|
||||||
|
+ Squid does not check this limit when sending responses.
|
||||||
|
DOC_END
|
||||||
|
|
||||||
|
NAME: request_body_max_size
|
||||||
|
diff --git a/src/http.cc b/src/http.cc
|
||||||
|
index 7c9ae70..98e3969 100644
|
||||||
|
--- a/src/http.cc
|
||||||
|
+++ b/src/http.cc
|
||||||
|
@@ -1926,8 +1926,9 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
|
||||||
|
|
||||||
|
String strFwd = hdr_in->getList(Http::HdrType::X_FORWARDED_FOR);
|
||||||
|
|
||||||
|
- // if we cannot double strFwd size, then it grew past 50% of the limit
|
||||||
|
- if (!strFwd.canGrowBy(strFwd.size())) {
|
||||||
|
+ // Detect unreasonably long header values. And paranoidly check String
|
||||||
|
+ // limits: a String ought to accommodate two reasonable-length values.
|
||||||
|
+ if (strFwd.size() > 32*1024 || !strFwd.canGrowBy(strFwd.size())) {
|
||||||
|
// There is probably a forwarding loop with Via detection disabled.
|
||||||
|
// If we do nothing, String will assert on overflow soon.
|
||||||
|
// TODO: Terminate all transactions with huge XFF?
|
||||||
@ -0,0 +1,13 @@
|
|||||||
|
diff --git a/lib/libTrie/TrieNode.cc b/lib/libTrie/TrieNode.cc
|
||||||
|
index b379856..5d87279 100644
|
||||||
|
--- a/lib/libTrie/TrieNode.cc
|
||||||
|
+++ b/lib/libTrie/TrieNode.cc
|
||||||
|
@@ -32,7 +32,7 @@ TrieNode::add(char const *aString, size_t theLength, void *privatedata, TrieChar
|
||||||
|
/* We trust that privatedata and existant keys have already been checked */
|
||||||
|
|
||||||
|
if (theLength) {
|
||||||
|
- int index = transform ? (*transform)(*aString): *aString;
|
||||||
|
+ const unsigned char index = transform ? (*transform)(*aString): *aString;
|
||||||
|
|
||||||
|
if (!internal[index])
|
||||||
|
internal[index] = new TrieNode;
|
||||||
@ -0,0 +1,113 @@
|
|||||||
|
From a0a9e6dc69d0c7b9ba237702b4c5020abc7ad1f8 Mon Sep 17 00:00:00 2001
|
||||||
|
From: Alex Rousskov <rousskov@measurement-factory.com>
|
||||||
|
Date: Sat, 4 Nov 2023 00:30:42 +0000
|
||||||
|
Subject: [PATCH] Bug 5154: Do not open IPv6 sockets when IPv6 is disabled
|
||||||
|
(#1567)
|
||||||
|
|
||||||
|
... but allow basic IPv6 manipulations like getSockAddr().
|
||||||
|
|
||||||
|
Address.cc:663 getAddrInfo() assertion failed: false
|
||||||
|
|
||||||
|
Squids receives IPv6 addresses from traffic, configuration, or
|
||||||
|
hard-coded constants even when ./configured with --disable-ipv6 or when
|
||||||
|
IPv6 support was automatically disabled at startup after failing IPv6
|
||||||
|
tests. To handle IPv6 correctly, such Squids must support basic IPv6
|
||||||
|
operations like recognizing an IPv6 address in a request-target or
|
||||||
|
reporting an unsolicited IPv6 DNS record. At least for now, such Squids
|
||||||
|
must also correctly parse configuration-related IPv6 addresses.
|
||||||
|
|
||||||
|
All those activities rely on various low-level operations like filling
|
||||||
|
addrinfo structure with IP address information. Since 2012 commit
|
||||||
|
c5fbbc7, Ip::Address::getAddrInfo() was failing for IPv6 addresses when
|
||||||
|
Ip::EnableIpv6 was falsy. That change correctly recognized[^1] the need
|
||||||
|
for such Squids to handle IPv6, but to support basic operations, we need
|
||||||
|
to reject IPv6 addresses at a higher level and without asserting.
|
||||||
|
|
||||||
|
That high-level rejection work is ongoing, but initial attempts have
|
||||||
|
exposed difficult problems that will take time to address. For now, we
|
||||||
|
just avoid the assertion while protecting IPv6-disabled Squid from
|
||||||
|
listening on or opening connections to IPv6 addresses. Since Squid
|
||||||
|
already expects (and usually correctly handles) socket opening failures,
|
||||||
|
disabling those operations is better than failing in low-level IP
|
||||||
|
manipulation code.
|
||||||
|
|
||||||
|
The overall IPv6 posture of IPv6-disabled Squids that lack http_access
|
||||||
|
or other rules to deny IPv6 requests will change: This fix exposes more
|
||||||
|
of IPv6-disabled Squid code to IPv6 addresses. It is possible that such
|
||||||
|
exposure will make some IPv6 resources inside Squid (e.g., a previously
|
||||||
|
cached HTTP response) accessible to external requests. Squids will not
|
||||||
|
open or accept IPv6 connections but may forward requests with raw IPv6
|
||||||
|
targets to IPv4 cache_peers. Whether these and similar behavior changes
|
||||||
|
are going to be permanent is open for debate, but even if they are
|
||||||
|
temporary, they are arguably better than the corresponding assertions.
|
||||||
|
|
||||||
|
These changes do not effect IPv6-enabled Squids.
|
||||||
|
|
||||||
|
The assertion in IPv6-disabled Squid was reported by Joshua Rogers at
|
||||||
|
https://megamansec.github.io/Squid-Security-Audit/ipv6-assert.html where
|
||||||
|
it was filed as "Assertion on IPv6 Host Requests with --disable-ipv6".
|
||||||
|
|
||||||
|
[^1]: https://bugs.squid-cache.org/show_bug.cgi?id=3593#c1
|
||||||
|
---
|
||||||
|
src/comm.cc | 6 ++++++
|
||||||
|
src/ip/Address.cc | 2 +-
|
||||||
|
src/ip/Intercept.cc | 8 ++++++++
|
||||||
|
3 files changed, 15 insertions(+), 1 deletion(-)
|
||||||
|
|
||||||
|
diff --git a/src/comm.cc b/src/comm.cc
|
||||||
|
index 4659955b011..271ba04d4da 100644
|
||||||
|
--- a/src/comm.cc
|
||||||
|
+++ b/src/comm.cc
|
||||||
|
@@ -344,6 +344,12 @@ comm_openex(int sock_type,
|
||||||
|
/* Create socket for accepting new connections. */
|
||||||
|
++ statCounter.syscalls.sock.sockets;
|
||||||
|
|
||||||
|
+ if (!Ip::EnableIpv6 && addr.isIPv6()) {
|
||||||
|
+ debugs(50, 2, "refusing to open an IPv6 socket when IPv6 support is disabled: " << addr);
|
||||||
|
+ errno = ENOTSUP;
|
||||||
|
+ return -1;
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
/* Setup the socket addrinfo details for use */
|
||||||
|
addr.getAddrInfo(AI);
|
||||||
|
AI->ai_socktype = sock_type;
|
||||||
|
diff --git a/src/ip/Address.cc b/src/ip/Address.cc
|
||||||
|
index b6f810bfc25..ae6db37da5e 100644
|
||||||
|
--- a/src/ip/Address.cc
|
||||||
|
+++ b/src/ip/Address.cc
|
||||||
|
@@ -623,7 +623,7 @@ Ip::Address::getAddrInfo(struct addrinfo *&dst, int force) const
|
||||||
|
&& dst->ai_protocol == 0)
|
||||||
|
dst->ai_protocol = IPPROTO_UDP;
|
||||||
|
|
||||||
|
- if (force == AF_INET6 || (force == AF_UNSPEC && Ip::EnableIpv6 && isIPv6()) ) {
|
||||||
|
+ if (force == AF_INET6 || (force == AF_UNSPEC && isIPv6()) ) {
|
||||||
|
dst->ai_addr = (struct sockaddr*)new sockaddr_in6;
|
||||||
|
|
||||||
|
memset(dst->ai_addr,0,sizeof(struct sockaddr_in6));
|
||||||
|
diff --git a/src/ip/Intercept.cc b/src/ip/Intercept.cc
|
||||||
|
index 1a5e2d15af1..a8522efaac0 100644
|
||||||
|
--- a/src/ip/Intercept.cc
|
||||||
|
+++ b/src/ip/Intercept.cc
|
||||||
|
@@ -15,6 +15,7 @@
|
||||||
|
#include "comm/Connection.h"
|
||||||
|
#include "fde.h"
|
||||||
|
#include "ip/Intercept.h"
|
||||||
|
+#include "ip/tools.h"
|
||||||
|
#include "src/tools.h"
|
||||||
|
|
||||||
|
#include <cerrno>
|
||||||
|
@@ -430,6 +431,13 @@ Ip::Intercept::ProbeForTproxy(Ip::Address &test)
|
||||||
|
|
||||||
|
debugs(3, 3, "Detect TPROXY support on port " << test);
|
||||||
|
|
||||||
|
+ if (!Ip::EnableIpv6 && test.isIPv6() && !test.setIPv4()) {
|
||||||
|
+ debugs(3, DBG_CRITICAL, "Cannot use TPROXY for " << test << " because IPv6 support is disabled");
|
||||||
|
+ if (doneSuid)
|
||||||
|
+ leave_suid();
|
||||||
|
+ return false;
|
||||||
|
+ }
|
||||||
|
+
|
||||||
|
int tos = 1;
|
||||||
|
int tmp_sock = -1;
|
||||||
|
|
||||||
|
|
||||||
Loading…
Reference in new issue