Check all chars in the URL are valid URL chars.

Should probably (also) be done in YaHTTP::URL, though currently the
return value of YaHTTP::URL::parse() is completely ignored, so
there is no easy way to do.

(cherry picked from commit 35eb2fcffa40e7f70b716e99158efe72a0e864d9)

diff --git a/pdns/Makefile.am b/pdns/Makefile.am
index 88229c32a7416c682027be9dbfd8558352037c2d..f391d64a4ce135f05a4c1fe768b8e2a7fcaf66a7 100644 (file)
--- a/pdns/Makefile.am
+++ b/pdns/Makefile.am
@@ -1417,6 +1417,7 @@ testrunner_SOURCES = \
        test-trusted-notification-proxy_cc.cc \
        test-tsig.cc \
        test-ueberbackend_cc.cc \
+       test-webserver_cc.cc \
        test-zonemd_cc.cc \
        test-zoneparser_tng_cc.cc \
        testrunner.cc \
@@ -1425,7 +1426,9 @@ testrunner_SOURCES = \
        tsigverifier.cc tsigverifier.hh \
        ueberbackend.cc ueberbackend.hh \
        unix_utility.cc \
+       uuid-utils.cc \
        validate.hh \
+       webserver.cc \
        zonemd.cc zonemd.hh \
        zoneparser-tng.cc zoneparser-tng.hh
 
@@ -1440,7 +1443,9 @@ testrunner_LDADD = \
        $(RT_LIBS) \
        $(LUA_LIBS) \
        $(LIBDL) \
-       $(IPCRYPT_LIBS)
+       $(IPCRYPT_LIBS) \
+       $(YAHTTP_LIBS) \
+       $(JSON11_LIBS)
 
 if GSS_TSIG
 testrunner_LDADD += $(GSS_LIBS)

diff --git a/pdns/recursordist/ws-recursor.cc b/pdns/recursordist/ws-recursor.cc
index 46efe7a8c4ea7e7443e4e5c1af8cf179a9455f4b..98e420bb27164b53d4cc1bcf118e113210645c93 100644 (file)
--- a/pdns/recursordist/ws-recursor.cc
+++ b/pdns/recursordist/ws-recursor.cc
@@ -1476,6 +1476,9 @@ void AsyncWebServer::serveConnection(const std::shared_ptr<Socket>& socket) cons
            req.d_slog->error(Logr::Warning, e.what(), "Unable to parse request"));
     }
 
+    if (!validURL(req.url)) {
+      throw PDNSException("Received request with invalid URL");
+    }
     logRequest(req, remote);
 
     WebServer::handleRequest(req, resp);
@@ -1492,6 +1495,12 @@ void AsyncWebServer::serveConnection(const std::shared_ptr<Socket>& socket) cons
            req.d_slog->info(Logr::Error, "Failed sending reply to HTTP client"));
     }
     handler->close(); // needed to signal "done" to client
+    if (d_loglevel >= WebServer::LogLevel::Normal) {
+      SLOG(g_log << Logger::Notice << logprefix << remote << " \"" << req.method << " " << req.url.path << " HTTP/" << req.versionStr(req.version) << "\" " << resp.status << " " << reply.size() << endl,
+           req.d_slog->info(Logr::Info, "Request", "remote", Logging::Loggable(remote), "method", Logging::Loggable(req.method),
+                            "urlpath", Logging::Loggable(req.url.path), "HTTPVersion", Logging::Loggable(req.versionStr(req.version)),
+                            "status", Logging::Loggable(resp.status), "respsize", Logging::Loggable(reply.size())));
+    }
   }
   catch (PDNSException& e) {
     SLOG(g_log << Logger::Error << logprefix << "Exception: " << e.reason << endl,
@@ -1507,13 +1516,6 @@ void AsyncWebServer::serveConnection(const std::shared_ptr<Socket>& socket) cons
     SLOG(g_log << Logger::Error << logprefix << "Unknown exception" << endl,
          req.d_slog->error(Logr::Error, "Exception handing request"))
   }
-
-  if (d_loglevel >= WebServer::LogLevel::Normal) {
-    SLOG(g_log << Logger::Notice << logprefix << remote << " \"" << req.method << " " << req.url.path << " HTTP/" << req.versionStr(req.version) << "\" " << resp.status << " " << reply.size() << endl,
-         req.d_slog->info(Logr::Info, "Request", "remote", Logging::Loggable(remote), "method", Logging::Loggable(req.method),
-                          "urlpath", Logging::Loggable(req.url.path), "HTTPVersion", Logging::Loggable(req.versionStr(req.version)),
-                          "status", Logging::Loggable(resp.status), "respsize", Logging::Loggable(reply.size())));
-  }
 }
 
 void AsyncWebServer::go()

diff --git a/pdns/test-webserver_cc.cc b/pdns/test-webserver_cc.cc
new file mode 100644 (file)
index 0000000..785330f
--- /dev/null
+++ b/pdns/test-webserver_cc.cc
@@ -0,0 +1,29 @@
+#define BOOST_TEST_DYN_LINK
+#define BOOST_TEST_NO_MAIN
+
+#include <boost/test/unit_test.hpp>
+#include "webserver.hh"
+
+BOOST_AUTO_TEST_SUITE(test_webserver_cc)
+
+BOOST_AUTO_TEST_CASE(test_validURL)
+{
+  // We cannot test\x00 as embedded NULs are not handled by YaHTTP other than stopping the parsing
+  const std::vector<std::pair<string, bool>> urls = {
+    {"http://www.powerdns.com/?foo=123", true},
+    {"http://ww.powerdns.com/?foo=%ff", true},
+    {"http://\x01ww.powerdns.com/?foo=123", false},
+    {"http://\xffwww.powerdns.com/?foo=123", false},
+    {"http://www.powerdns.com/?foo=123\x01", false},
+    {"http://www.powerdns.com/\x7f?foo=123", false},
+    {"http://www.powerdns.com/\x80?foo=123", false},
+    {"http://www.powerdns.com/?\xff", false},
+  };
+
+  for (const auto& testcase : urls) {
+    cerr << testcase.first << endl;
+    BOOST_CHECK_EQUAL(WebServer::validURL(testcase.first), testcase.second);
+  }
+}
+
+BOOST_AUTO_TEST_SUITE_END();

diff --git a/pdns/webserver.cc b/pdns/webserver.cc
index f417561f3b364590d34968292e239e5cdc8a41a9..62ca90d4e122f5a612d7231a906c21c6daf52d34 100644 (file)
--- a/pdns/webserver.cc
+++ b/pdns/webserver.cc
@@ -36,6 +36,8 @@
 #include "json.hh"
 #include "uuid-utils.hh"
 #include <yahttp/router.hpp>
+#include <algorithm>
+#include <unordered_set>
 
 json11::Json HttpRequest::json()
 {
@@ -461,6 +463,53 @@ void WebServer::logResponse(const HttpResponse& resp, const ComboAddress& /* rem
   }
 }
 
+
+struct ValidChars {
+  ValidChars()
+  {
+    // letter may be signed, but we only pass positive values
+    for (auto letter : "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~:/?#[]@!$&'()*+,;=") {
+      set.set(letter);
+    }
+  }
+  std::bitset<127> set;
+};
+
+static const ValidChars validChars;
+
+static bool validURLChars(const string& str)
+{
+  for (auto iter = str.begin(); iter != str.end(); ++iter) {
+    if (*iter == '%') {
+      ++iter;
+      if (iter == str.end() || isxdigit(static_cast<unsigned char>(*iter)) == 0) {
+        return false;
+      }
+      ++iter;
+      if (iter == str.end() || isxdigit(static_cast<unsigned char>(*iter)) == 0) {
+        return false;
+      }
+    }
+    else if (static_cast<size_t>(*iter) >= validChars.set.size() || !validChars.set[*iter]) {
+      return false;
+    }
+  }
+  return true;
+}
+
+bool WebServer::validURL(const YaHTTP::URL& url)
+{
+  bool isOK = true;
+  isOK = isOK && validURLChars(url.protocol);
+  isOK = isOK && validURLChars(url.host);
+  isOK = isOK && validURLChars(url.username);
+  isOK = isOK && validURLChars(url.password);
+  isOK = isOK && validURLChars(url.path);
+  isOK = isOK && validURLChars(url.parameters);
+  isOK = isOK && validURLChars(url.anchor);
+  return isOK;
+}
+
 void WebServer::serveConnection(const std::shared_ptr<Socket>& client) const {
   const auto unique = getUniqueID();
   const string logprefix = d_logprefix + to_string(unique) + " ";
@@ -504,6 +553,9 @@ void WebServer::serveConnection(const std::shared_ptr<Socket>& client) const {
            d_slog->error(Logr::Warning, e.what(), "Unable to parse request"));
     }
 
+    if (!validURL(req.url)) {
+      throw PDNSException("Received request with invalid URL");
+    }
     // Uses of `remote` below guarded by d_loglevel
     if (d_loglevel > WebServer::LogLevel::None) {
       client->getRemote(remote);

diff --git a/pdns/webserver.hh b/pdns/webserver.hh
index 3859117517dfb9ecfd524b8544edc64fc298f1fd..c75dd99ad09a52ffa6af653536573628101c1322 100644 (file)
--- a/pdns/webserver.hh
+++ b/pdns/webserver.hh
@@ -213,6 +213,8 @@ public:
     d_acl = nmg;
   }
 
+  static bool validURL(const YaHTTP::URL& url);
+
   void bind();
   void go();