]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Bug 5106: Broken cache manager URL parsing (#788)
authorAmos Jeffries <yadij@users.noreply.github.com>
Fri, 30 Apr 2021 05:15:44 +0000 (05:15 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Fri, 30 Apr 2021 12:52:41 +0000 (12:52 +0000)
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.

src/CacheManager.h
src/cache_manager.cc
src/mgr/QueryParams.cc
src/mgr/QueryParams.h
src/tests/stub_libmgr.cc
src/tests/testCacheManager.cc
src/tests/testCacheManager.h

index bb1ef02b815e5ee06e95c1f7a0dd70fc6201dceb..3d61a07a8266cb4c449af4812b947ffde5077eac 100644 (file)
@@ -9,6 +9,7 @@
 #ifndef SQUID_CACHEMANAGER_H
 #define SQUID_CACHEMANAGER_H
 
+#include "anyp/forward.h"
 #include "comm/forward.h"
 #include "log/forward.h"
 #include "mgr/Action.h"
@@ -51,7 +52,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 *);
index 158bb25756e7fa97336948ae9f66ce87ee6e4486..19b79caa580b7dff17f05dda220f51450c4ba3f1 100644 (file)
@@ -15,6 +15,7 @@
 #include "comm/Connection.h"
 #include "Debug.h"
 #include "errorpage.h"
+#include "error/ExceptionErrorDetail.h"
 #include "fde.h"
 #include "HttpReply.h"
 #include "HttpRequest.h"
@@ -27,7 +28,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"
@@ -148,82 +151,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;
 }
 
@@ -306,12 +314,17 @@ CacheManager::CheckPassword(const Mgr::Command &cmd)
 void
 CacheManager::start(const Comm::ConnectionPointer &client, HttpRequest *request, StoreEntry *entry, const AccessLogEntry::Pointer &ale)
 {
-    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) {
+    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, ale);
         err->url = xstrdup(entry->url());
+        err->detailError(new ExceptionErrorDetail(Here().id()));
         errorAppendEntry(entry, err);
         entry->expires = squid_curtime;
         return;
@@ -474,4 +487,3 @@ CacheManager::GetInstance()
     }
     return instance;
 }
-
index 43c739eed8fdfdf4399310d4f5d8fa04028c5280..ad1503c7fd525be552025e247e9b9f9a14bae3d6 100644 (file)
 #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;
 }
-
index 28816b7572b940fb890ffecab9668139bf59f185..e8c9a4b30febc487833890df62e01e7ddff70423 100644 (file)
 
 #include "ipc/forward.h"
 #include "mgr/QueryParam.h"
+#include "parser/forward.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;
index b2a1335b78509a416d739c28ba41e52fa2992c95..0823c8c108c3f71d17089402fddd78224d11bc4f 100644 (file)
@@ -176,11 +176,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);
index 3e412de9ff9f029447bd5454227da06ae2792673..3188249d31b93d42e221a797f53e49c22c1ea3af 100644 (file)
@@ -7,6 +7,7 @@
  */
 
 #include "squid.h"
+#include "anyp/Uri.h"
 #include "CacheManager.h"
 #include "mgr/Action.h"
 #include "Store.h"
 
 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");
+                    }
+                }
+            }
+        }
+    }
+}
index 8be302fb2bdf6bf0fd86c78c995df91906602a35..2f5d7b96e47feaf8cd3c5d1a971627a98d6e4532 100644 (file)
@@ -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