packages/squid
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
squid/backport-0002-CVE-2021-28652.patch

632 lines
21 KiB
Diff
Raw Blame History

commit 0003e3518dc95e4b5ab46b5140af79b22253048e
Author: Amos Jeffries <yadij@users.noreply.github.com>
Date: 2021-04-30 05:15:44 +0000
Bug 5106: Broken cache manager URL parsing (#788)
Use already parsed request-target URL in cache manager and
update CacheManager to Tokanizer based URL parse
Removing use of sscan() and regex string processing which have
proven to be problematic on many levels. Most particularly with
regards to tolerance of normally harmless garbage syntax in URLs
received.
Support for generic URI schemes is added possibly resolving some
issues reported with ftp:// URL and manager access via ftp_port
sockets.
Truly generic support for /squid-internal-mgr/ path prefix is
added, fixing some user confusion about its use on cache_object:
scheme URLs.
TODO: support for single-name parameters and URL #fragments
are left to future updates. As is refactoring the QueryParams
data storage to avoid SBuf data copying.
diff --git a/src/CacheManager.h b/src/CacheManager.h
index 78a69f799..74705c58a 100644
--- a/src/CacheManager.h
+++ b/src/CacheManager.h
@@ -9,6 +9,7 @@
#ifndef SQUID_CACHEMANAGER_H
#define SQUID_CACHEMANAGER_H
+#include "anyp/forward.h"
#include "comm/forward.h"
#include "mgr/Action.h"
#include "mgr/ActionProfile.h"
@@ -50,7 +51,7 @@ public:
protected:
CacheManager() {} ///< use Instance() instead
- Mgr::CommandPointer ParseUrl(const char *url);
+ Mgr::CommandPointer ParseUrl(const AnyP::Uri &);
void ParseHeaders(const HttpRequest * request, Mgr::ActionParams &params);
int CheckPassword(const Mgr::Command &cmd);
char *PasswdGet(Mgr::ActionPasswordList *, const char *);
diff --git a/src/cache_manager.cc b/src/cache_manager.cc
index 9fe9bbb89..8055ece6b 100644
--- a/src/cache_manager.cc
+++ b/src/cache_manager.cc
@@ -26,7 +26,9 @@
#include "mgr/Forwarder.h"
#include "mgr/FunAction.h"
#include "mgr/QueryParams.h"
+#include "parser/Tokenizer.h"
#include "protos.h"
+#include "sbuf/Stream.h"
#include "sbuf/StringConvert.h"
#include "SquidConfig.h"
#include "SquidTime.h"
@@ -147,82 +149,87 @@ CacheManager::createRequestedAction(const Mgr::ActionParams &params)
return cmd->profile->creator->create(cmd);
}
+static const CharacterSet &
+MgrFieldChars(const AnyP::ProtocolType &protocol)
+{
+ // Deprecated cache_object:// scheme used '@' to delimit passwords
+ if (protocol == AnyP::PROTO_CACHE_OBJECT) {
+ static const CharacterSet fieldChars = CharacterSet("cache-object-field", "@?#").complement();
+ return fieldChars;
+ }
+
+ static const CharacterSet actionChars = CharacterSet("mgr-field", "?#").complement();
+ return actionChars;
+}
+
/**
- \ingroup CacheManagerInternal
* define whether the URL is a cache-manager URL and parse the action
* requested by the user. Checks via CacheManager::ActionProtection() that the
* item is accessible by the user.
- \retval CacheManager::cachemgrStateData state object for the following handling
- \retval NULL if the action can't be found or can't be accessed by the user
+ *
+ * Syntax:
+ *
+ * scheme "://" authority [ '/squid-internal-mgr' ] path-absolute [ '@' unreserved ] '?' query-string
+ *
+ * see RFC 3986 for definitions of scheme, authority, path-absolute, query-string
+ *
+ * \returns Mgr::Command object with action to perform and parameters it might use
*/
Mgr::Command::Pointer
-CacheManager::ParseUrl(const char *url)
+CacheManager::ParseUrl(const AnyP::Uri &uri)
{
- int t;
- LOCAL_ARRAY(char, host, MAX_URL);
- LOCAL_ARRAY(char, request, MAX_URL);
- LOCAL_ARRAY(char, password, MAX_URL);
- LOCAL_ARRAY(char, params, MAX_URL);
- host[0] = 0;
- request[0] = 0;
- password[0] = 0;
- params[0] = 0;
- int pos = -1;
- int len = strlen(url);
- Must(len > 0);
- t = sscanf(url, "cache_object://%[^/]/%[^@?]%n@%[^?]?%s", host, request, &pos, password, params);
- if (t < 3) {
- t = sscanf(url, "cache_object://%[^/]/%[^?]%n?%s", host, request, &pos, params);
- }
- if (t < 1) {
- t = sscanf(url, "http://%[^/]/squid-internal-mgr/%[^?]%n?%s", host, request, &pos, params);
- }
- if (t < 1) {
- t = sscanf(url, "https://%[^/]/squid-internal-mgr/%[^?]%n?%s", host, request, &pos, params);
- }
- if (t < 2) {
- if (strncmp("cache_object://",url,15)==0)
- xstrncpy(request, "menu", MAX_URL);
- else
- xstrncpy(request, "index", MAX_URL);
- }
+ Parser::Tokenizer tok(uri.path());
-#if _SQUID_OS2_
- if (t == 2 && request[0] == '\0') {
- /*
- * emx's sscanf insists of returning 2 because it sets request
- * to null
- */
- if (strncmp("cache_object://",url,15)==0)
- xstrncpy(request, "menu", MAX_URL);
- else
- xstrncpy(request, "index", MAX_URL);
- }
-#endif
+ static const SBuf internalMagicPrefix("/squid-internal-mgr/");
+ if (!tok.skip(internalMagicPrefix) && !tok.skip('/'))
+ throw TextException("invalid URL path", Here());
- debugs(16, 3, HERE << "MGR request: t=" << t << ", host='" << host << "', request='" << request << "', pos=" << pos <<
- ", password='" << password << "', params='" << params << "'");
+ Mgr::Command::Pointer cmd = new Mgr::Command();
+ cmd->params.httpUri = SBufToString(uri.absolute());
- Mgr::ActionProfile::Pointer profile = findAction(request);
- if (!profile) {
- debugs(16, DBG_IMPORTANT, "CacheManager::ParseUrl: action '" << request << "' not found");
- return NULL;
+ const auto &fieldChars = MgrFieldChars(uri.getScheme());
+
+ SBuf action;
+ if (!tok.prefix(action, fieldChars)) {
+ if (uri.getScheme() == AnyP::PROTO_CACHE_OBJECT) {
+ static const SBuf menuReport("menu");
+ action = menuReport;
+ } else {
+ static const SBuf indexReport("index");
+ action = indexReport;
+ }
}
+ cmd->params.actionName = SBufToString(action);
+
+ const auto profile = findAction(action.c_str());
+ if (!profile)
+ throw TextException(ToSBuf("action '", action, "' not found"), Here());
const char *prot = ActionProtection(profile);
- if (!strcmp(prot, "disabled") || !strcmp(prot, "hidden")) {
- debugs(16, DBG_IMPORTANT, "CacheManager::ParseUrl: action '" << request << "' is " << prot);
- return NULL;
+ if (!strcmp(prot, "disabled") || !strcmp(prot, "hidden"))
+ throw TextException(ToSBuf("action '", action, "' is ", prot), Here());
+ cmd->profile = profile;
+
+ SBuf passwd;
+ if (uri.getScheme() == AnyP::PROTO_CACHE_OBJECT && tok.skip('@')) {
+ (void)tok.prefix(passwd, fieldChars);
+ cmd->params.password = SBufToString(passwd);
}
- Mgr::Command::Pointer cmd = new Mgr::Command;
- if (!Mgr::QueryParams::Parse(params, cmd->params.queryParams))
- return NULL;
- cmd->profile = profile;
- cmd->params.httpUri = url;
- cmd->params.userName = String();
- cmd->params.password = password;
- cmd->params.actionName = request;
+ // TODO: fix when AnyP::Uri::parse() separates path?query#fragment
+ SBuf params;
+ if (tok.skip('?')) {
+ params = tok.remaining();
+ Mgr::QueryParams::Parse(tok, cmd->params.queryParams);
+ }
+
+ if (!tok.skip('#') && !tok.atEnd())
+ throw TextException("invalid characters in URL", Here());
+ // else ignore #fragment (if any)
+
+ debugs(16, 3, "MGR request: host=" << uri.host() << ", action=" << action <<
+ ", password=" << passwd << ", params=" << params);
+
return cmd;
}
@@ -305,11 +312,15 @@ CacheManager::CheckPassword(const Mgr::Command &cmd)
void
CacheManager::Start(const Comm::ConnectionPointer &client, HttpRequest * request, StoreEntry * entry)
{
- debugs(16, 3, "CacheManager::Start: '" << entry->url() << "'" );
+ debugs(16, 3, "request-url= '" << request->url << "', entry-url='" << entry->url() << "'");
- Mgr::Command::Pointer cmd = ParseUrl(entry->url());
- if (!cmd) {
- ErrorState *err = new ErrorState(ERR_INVALID_URL, Http::scNotFound, request);
+ Mgr::Command::Pointer cmd;
+ try {
+ cmd = ParseUrl(request->url);
+
+ } catch (...) {
+ debugs(16, 2, "request URL error: " << CurrentException);
+ const auto err = new ErrorState(ERR_INVALID_URL, Http::scNotFound, request);
err->url = xstrdup(entry->url());
errorAppendEntry(entry, err);
entry->expires = squid_curtime;
@@ -473,4 +484,3 @@ CacheManager::GetInstance()
}
return instance;
}
-
diff --git a/src/mgr/QueryParams.cc b/src/mgr/QueryParams.cc
index 831694245..a53dee1c7 100644
--- a/src/mgr/QueryParams.cc
+++ b/src/mgr/QueryParams.cc
@@ -14,6 +14,10 @@
#include "mgr/IntParam.h"
#include "mgr/QueryParams.h"
#include "mgr/StringParam.h"
+#include "parser/Tokenizer.h"
+#include "sbuf/StringConvert.h"
+
+#include <limits>
Mgr::QueryParam::Pointer
Mgr::QueryParams::get(const String& name) const
@@ -65,61 +69,76 @@ Mgr::QueryParams::find(const String& name) const
return iter;
}
-bool
-Mgr::QueryParams::ParseParam(const String& paramStr, Param& param)
+/**
+ * Parses the value part of a "param=value" URL section.
+ * Value can be a comma-separated list of integers or an opaque string.
+ *
+ * value = *pchar | ( 1*DIGIT *( ',' 1*DIGIT ) )
+ *
+ * \note opaque string may be a list with a non-integer (e.g., "1,2,3,z")
+ */
+Mgr::QueryParam::Pointer
+ParseParamValue(const SBuf &rawValue)
{
- bool parsed = false;
- regmatch_t pmatch[3];
- regex_t intExpr;
- regcomp(&intExpr, "^([a-z][a-z0-9_]*)=([0-9]+((,[0-9]+))*)$", REG_EXTENDED | REG_ICASE);
- regex_t stringExpr;
- regcomp(&stringExpr, "^([a-z][a-z0-9_]*)=([^&= ]+)$", REG_EXTENDED | REG_ICASE);
- if (regexec(&intExpr, paramStr.termedBuf(), 3, pmatch, 0) == 0) {
- param.first = paramStr.substr(pmatch[1].rm_so, pmatch[1].rm_eo);
- std::vector<int> array;
- int n = pmatch[2].rm_so;
- for (int i = n; i < pmatch[2].rm_eo; ++i) {
- if (paramStr[i] == ',') {
- array.push_back(atoi(paramStr.substr(n, i).termedBuf()));
- n = i + 1;
- }
- }
- if (n < pmatch[2].rm_eo)
- array.push_back(atoi(paramStr.substr(n, pmatch[2].rm_eo).termedBuf()));
- param.second = new IntParam(array);
- parsed = true;
- } else if (regexec(&stringExpr, paramStr.termedBuf(), 3, pmatch, 0) == 0) {
- param.first = paramStr.substr(pmatch[1].rm_so, pmatch[1].rm_eo);
- param.second = new StringParam(paramStr.substr(pmatch[2].rm_so, pmatch[2].rm_eo));
- parsed = true;
+ static const CharacterSet comma("comma", ",");
+
+ Parser::Tokenizer tok(rawValue);
+ std::vector<int> array;
+ int64_t intVal = 0;
+ while (tok.int64(intVal, 10, false)) {
+ Must(intVal >= std::numeric_limits<int>::min());
+ Must(intVal <= std::numeric_limits<int>::max());
+ array.emplace_back(intVal);
+ // integer list has comma between values.
+ // Require at least one potential DIGIT after the skipped ','
+ if (tok.remaining().length() > 1)
+ (void)tok.skipOne(comma);
}
- regfree(&stringExpr);
- regfree(&intExpr);
- return parsed;
+
+ if (tok.atEnd())
+ return new Mgr::IntParam(array);
+ else
+ return new Mgr::StringParam(SBufToString(rawValue));
}
-bool
-Mgr::QueryParams::Parse(const String& aParamsStr, QueryParams& aParams)
+/**
+ * Syntax:
+ * query = [ param *( '&' param ) ]
+ * param = name '=' value
+ * name = [a-zA-Z0-9]+
+ * value = *pchar | ( 1*DIGIT *( ',' 1*DIGIT ) )
+ */
+void
+Mgr::QueryParams::Parse(Parser::Tokenizer &tok, QueryParams &aParams)
{
- if (aParamsStr.size() != 0) {
- Param param;
- size_t n = 0;
- size_t len = aParamsStr.size();
- for (size_t i = n; i < len; ++i) {
- if (aParamsStr[i] == '&') {
- if (!ParseParam(aParamsStr.substr(n, i), param))
- return false;
- aParams.params.push_back(param);
- n = i + 1;
- }
- }
- if (n < len) {
- if (!ParseParam(aParamsStr.substr(n, len), param))
- return false;
- aParams.params.push_back(param);
- }
+ static const CharacterSet nameChars = CharacterSet("param-name", "_") + CharacterSet::ALPHA + CharacterSet::DIGIT;
+ static const CharacterSet valueChars = CharacterSet("param-value", "&= #").complement();
+ static const CharacterSet delimChars("param-delim", "&");
+
+ while (!tok.atEnd()) {
+
+ // TODO: remove '#' processing when AnyP::Uri splits 'query#fragment' properly
+ // #fragment handled by caller. Do not throw.
+ if (tok.remaining()[0] == '#')
+ return;
+
+ if (tok.skipAll(delimChars))
+ continue;
+
+ SBuf nameStr;
+ if (!tok.prefix(nameStr, nameChars))
+ throw TextException("invalid query parameter name", Here());
+ if (!tok.skip('='))
+ throw TextException("missing parameter value", Here());
+
+ SBuf valueStr;
+ if (!tok.prefix(valueStr, valueChars))
+ throw TextException("missing or malformed parameter value", Here());
+
+ const auto name = SBufToString(nameStr);
+ const auto value = ParseParamValue(valueStr);
+ aParams.params.emplace_back(name, value);
}
- return true;
}
Mgr::QueryParam::Pointer
@@ -138,4 +157,3 @@ Mgr::QueryParams::CreateParam(QueryParam::Type aType)
}
return NULL;
}
-
diff --git a/src/mgr/QueryParams.h b/src/mgr/QueryParams.h
index bb8f40308..450c20f86 100644
--- a/src/mgr/QueryParams.h
+++ b/src/mgr/QueryParams.h
@@ -13,9 +13,11 @@
#include "ipc/forward.h"
#include "mgr/QueryParam.h"
+#include "parser/Tokenizer.h"
#include "SquidString.h"
-#include <vector>
+
#include <utility>
+#include <vector>
namespace Mgr
{
@@ -32,15 +34,13 @@ public:
void pack(Ipc::TypedMsgHdr& msg) const; ///< store params into msg
void unpack(const Ipc::TypedMsgHdr& msg); ///< load params from msg
/// parses the query string parameters
- static bool Parse(const String& aParamsStr, QueryParams& aParams);
+ static void Parse(Parser::Tokenizer &, QueryParams &);
private:
/// find query parameter by name
Params::const_iterator find(const String& name) const;
/// creates a parameter of the specified type
static QueryParam::Pointer CreateParam(QueryParam::Type aType);
- /// parses string like "param=value"; returns true if success
- static bool ParseParam(const String& paramStr, Param& param);
private:
Params params;
diff --git a/src/tests/stub_libmgr.cc b/src/tests/stub_libmgr.cc
index f8be88a58..cd3ffc2de 100644
--- a/src/tests/stub_libmgr.cc
+++ b/src/tests/stub_libmgr.cc
@@ -174,11 +174,10 @@ void Mgr::IoAction::dump(StoreEntry* entry) STUB
Mgr::QueryParam::Pointer Mgr::QueryParams::get(const String& name) const STUB_RETVAL(Mgr::QueryParam::Pointer(NULL))
void Mgr::QueryParams::pack(Ipc::TypedMsgHdr& msg) const STUB
void Mgr::QueryParams::unpack(const Ipc::TypedMsgHdr& msg) STUB
-bool Mgr::QueryParams::Parse(const String& aParamsStr, QueryParams& aParams) STUB_RETVAL(false)
+void Mgr::QueryParams::Parse(Parser::Tokenizer &, QueryParams &) STUB
//private:
//Params::const_iterator Mgr::QueryParams::find(const String& name) const STUB_RETVAL(new Mgr::Params::const_iterator(*this))
Mgr::QueryParam::Pointer Mgr::QueryParams::CreateParam(QueryParam::Type aType) STUB_RETVAL(Mgr::QueryParam::Pointer(NULL))
-bool Mgr::QueryParams::ParseParam(const String& paramStr, Param& param) STUB_RETVAL(false)
#include "mgr/Registration.h"
//void Mgr::RegisterAction(char const * action, char const * desc, OBJH * handler, int pw_req_flag, int atomic);
diff --git a/src/tests/testCacheManager.cc b/src/tests/testCacheManager.cc
index f02396176..7d6631aae 100644
--- a/src/tests/testCacheManager.cc
+++ b/src/tests/testCacheManager.cc
@@ -7,6 +7,7 @@
*/
#include "squid.h"
+#include "anyp/Uri.h"
#include "CacheManager.h"
#include "mgr/Action.h"
#include "Store.h"
@@ -17,11 +18,19 @@
CPPUNIT_TEST_SUITE_REGISTRATION( testCacheManager );
+/// Provides test code access to CacheManager internal symbols
+class CacheManagerInternals : public CacheManager
+{
+public:
+ void ParseUrl(const AnyP::Uri &u) { CacheManager::ParseUrl(u); }
+};
+
/* init memory pools */
void testCacheManager::setUp()
{
Mem::Init();
+ AnyP::UriScheme::Init();
}
/*
@@ -66,3 +75,146 @@ testCacheManager::testRegister()
CPPUNIT_ASSERT_EQUAL(1,(int)sentry->flags);
}
+void
+testCacheManager::testParseUrl()
+{
+ auto *mgr = static_cast<CacheManagerInternals *>(CacheManager::GetInstance());
+ CPPUNIT_ASSERT(mgr != nullptr);
+
+ std::vector<AnyP::ProtocolType> validSchemes = {
+ AnyP::PROTO_CACHE_OBJECT,
+ AnyP::PROTO_HTTP,
+ AnyP::PROTO_HTTPS,
+ AnyP::PROTO_FTP
+ };
+
+ AnyP::Uri mgrUrl;
+ mgrUrl.host("localhost");
+ mgrUrl.port(3128);
+
+ const std::vector<const char *> magicPrefixes = {
+ "/",
+ "/squid-internal-mgr/"
+ };
+
+ const std::vector<const char *> validActions = {
+ "",
+ "menu"
+ };
+
+ const std::vector<const char *> invalidActions = {
+ "INVALID" // any unregistered name
+ };
+
+ const std::vector<const char *> validParams = {
+ "",
+ "?",
+ "?&",
+ "?&&&&&&&&&&&&",
+ "?foo=bar",
+ "?0123456789=bar",
+ "?foo=bar&",
+ "?foo=bar&&&&",
+ "?&foo=bar",
+ "?&&&&foo=bar",
+ "?&foo=bar&",
+ "?&&&&foo=bar&&&&",
+ "?foo=?_weird?~`:[]stuff&bar=okay&&&&&&",
+ "?intlist=1",
+ "?intlist=1,2,3,4,5",
+ "?string=1a",
+ "?string=1,2,3,4,z",
+ "?string=1,2,3,4,[0]",
+ "?intlist=1,2,3,4,5&string=1,2,3,4,y"
+ };
+
+ const std::vector<const char *> invalidParams = {
+ "?/",
+ "?foo",
+ "?/foo",
+ "?foo/",
+ "?foo=",
+ "?foo=&",
+ "?=foo",
+ "? foo=bar",
+ "? &",
+ "?& ",
+ "?=&",
+ "?&=",
+ "? &&&",
+ "?& &&",
+ "?&& &",
+ "?=&&&",
+ "?&=&&",
+ "?&&=&"
+ };
+
+ const std::vector<const char *> validFragments = {
+ "",
+ "#",
+ "##",
+ "#?a=b",
+ "#fragment"
+ };
+
+ for (const auto &scheme : validSchemes) {
+ mgrUrl.setScheme(scheme);
+
+ for (const auto *magic : magicPrefixes) {
+
+ // all schemes except cache_object require magic path prefix bytes
+ if (scheme != AnyP::PROTO_CACHE_OBJECT && strlen(magic) <= 2)
+ continue;
+
+ /* Check the parser accepts all the valid cases */
+
+ for (const auto *action : validActions) {
+ for (const auto *param : validParams) {
+ for (const auto *frag : validFragments) {
+ try {
+ SBuf bits;
+ bits.append(magic);
+ bits.append(action);
+ bits.append(param);
+ bits.append(frag);
+ mgrUrl.path(bits);
+
+ (void)mgr->ParseUrl(mgrUrl);
+ } catch (...) {
+ std::cerr << std::endl
+ << "FAIL: " << mgrUrl
+ << Debug::Extra << "error: " << CurrentException << std::endl;
+ CPPUNIT_FAIL("rejected a valid URL");
+ }
+ }
+ }
+ }
+
+ /* Check that invalid parameters are rejected */
+
+ for (const auto *action : validActions) {
+ for (const auto *param : invalidParams) {
+ for (const auto *frag : validFragments) {
+ try {
+ SBuf bits;
+ bits.append(magic);
+ bits.append(action);
+ bits.append(param);
+ bits.append(frag);
+ mgrUrl.path(bits);
+
+ (void)mgr->ParseUrl(mgrUrl);
+
+ std::cerr << std::endl
+ << "FAIL: " << mgrUrl
+ << Debug::Extra << "error: should be rejected due to '" << param << "'" << std::endl;
+ } catch (const TextException &e) {
+ continue; // success. caught bad input
+ }
+ CPPUNIT_FAIL("failed to reject an invalid URL");
+ }
+ }
+ }
+ }
+ }
+}
diff --git a/src/tests/testCacheManager.h b/src/tests/testCacheManager.h
index 6d32d69e5..fee15846a 100644
--- a/src/tests/testCacheManager.h
+++ b/src/tests/testCacheManager.h
@@ -20,6 +20,7 @@ class testCacheManager : public CPPUNIT_NS::TestFixture
CPPUNIT_TEST_SUITE( testCacheManager );
CPPUNIT_TEST( testCreate );
CPPUNIT_TEST( testRegister );
+ CPPUNIT_TEST( testParseUrl );
CPPUNIT_TEST_SUITE_END();
public:
@@ -28,6 +29,7 @@ public:
protected:
void testCreate();
void testRegister();
+ void testParseUrl();
};
#endif
Reference in New Issue View Git Blame Copy Permalink