]> 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)
committerAmos Jeffries <yadij@users.noreply.github.com>
Thu, 6 May 2021 04:56:26 +0000 (16:56 +1200)
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 74ccdcf81f2cff83562156f07f6dfdcba36038bc..793f7d464f3d6887a1b72e0923a5890b7766c4dc 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 360d9e3ccf73aaa754dc34aaa1e71b5f321d8ef9..56400a6cc0f81dc274c2843d8f0681ba097a4708 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 831694245e0811ebb9c07e4d4c7475f86a48f463..a53dee1c72a531f178ad32b56676ffc5aed3c5db 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 bb8f403089afa5f7e296026364b9998949f3d98f..d75580fb7af2e166e4a1e72cc711dbb1c21eb588 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 9a0cd971480dfd62cbec2602813b0be781c1d777..5479f964cdf5666878d7ee7a3171c0f588f5d561 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 f02396176b4ba42760674cb215c734a118f93316..7d6631aaee41383f7b4de6ad87fe540ad90e6d3b 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 59d0edd5549c748d6c67e6ba226f3bc09b39d5dc..4aa22fbf47123b8d87c6d4a278e42f14901cdb14 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