From: Otto Moerbeek Date: Fri, 21 Jul 2023 12:23:02 +0000 (+0200) Subject: Check all chars in the URL are valid URL chars. X-Git-Tag: rec-4.9.1^2~3 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ddedba8d1630cf26762334b83630a3a8c9ec54e5;p=thirdparty%2Fpdns.git 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 88229c32a7..f391d64a4c 100644 --- 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 46efe7a8c4..98e420bb27 100644 --- a/pdns/recursordist/ws-recursor.cc +++ b/pdns/recursordist/ws-recursor.cc @@ -1476,6 +1476,9 @@ void AsyncWebServer::serveConnection(const std::shared_ptr& 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) 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) 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 index 0000000000..785330f932 --- /dev/null +++ b/pdns/test-webserver_cc.cc @@ -0,0 +1,29 @@ +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_NO_MAIN + +#include +#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> 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 f417561f3b..62ca90d4e1 100644 --- a/pdns/webserver.cc +++ b/pdns/webserver.cc @@ -36,6 +36,8 @@ #include "json.hh" #include "uuid-utils.hh" #include +#include +#include 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(*iter)) == 0) { + return false; + } + ++iter; + if (iter == str.end() || isxdigit(static_cast(*iter)) == 0) { + return false; + } + } + else if (static_cast(*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& 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& 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 3859117517..c75dd99ad0 100644 --- 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();