From 64c4f83c31c5f87684fde86612efd6b19ae2bc6b Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 9 Apr 2021 12:29:09 +0200 Subject: [PATCH] Make the hashing of plaintext credentials optional --- docs/settings.rst | 11 +++++++++++ pdns/common_startup.cc | 1 + pdns/credentials.cc | 15 ++++++++++----- pdns/credentials.hh | 9 ++++++++- pdns/dnsdist-lua.cc | 13 +++++++++---- pdns/dnsdistdist/docs/reference/config.rst | 2 ++ pdns/pdns_recursor.cc | 1 + pdns/recursordist/docs/settings.rst | 11 +++++++++++ pdns/test-credentials_cc.cc | 12 ++++++++++-- pdns/webserver.hh | 8 ++++---- pdns/ws-auth.cc | 6 +++--- pdns/ws-recursor.cc | 4 ++-- 12 files changed, 72 insertions(+), 21 deletions(-) diff --git a/docs/settings.rst b/docs/settings.rst index e6b568022f..b8a7c0beda 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -1774,6 +1774,17 @@ IP Address for webserver/API to listen on. Webserver/API access is only allowed from these subnets. +.. _setting-webserver-hash-plaintext-credentials: + +``webserver-hash-plaintext-credentials`` +---------------------------------------- +..versionadded:: 4.6.0 + +- Boolean +- Default: no + +Whether passwords and API keys supplied as plaintext should be hashed during startup, to prevent the plaintext versions from staying in memory. Doing so increases significantly the cost of verifying credentials. + .. _setting-webserver-loglevel: ``webserver-loglevel`` diff --git a/pdns/common_startup.cc b/pdns/common_startup.cc index afc9b0b685..042647f3c6 100644 --- a/pdns/common_startup.cc +++ b/pdns/common_startup.cc @@ -169,6 +169,7 @@ void declareArguments() ::arg().set("webserver-allow-from","Webserver/API access is only allowed from these subnets")="127.0.0.1,::1"; ::arg().set("webserver-loglevel", "Amount of logging in the webserver (none, normal, detailed)") = "normal"; ::arg().set("webserver-max-bodysize","Webserver/API maximum request/response body size in megabytes")="2"; + ::arg().setSwitch("webserver-hash-plaintext-credentials","Whether to hash passwords and api keys supplied in plaintext, to prevent keeping the plaintext version in memory at runtime")="no"; ::arg().setSwitch("query-logging","Hint backends that queries should be logged")="no"; diff --git a/pdns/credentials.cc b/pdns/credentials.cc index e17111058d..139e6337ee 100644 --- a/pdns/credentials.cc +++ b/pdns/credentials.cc @@ -92,21 +92,26 @@ bool isPasswordHashed(const std::string& password) /* if the password is in cleartext and hashing is available, the hashed form will be kept in memory */ -CredentialsHolder::CredentialsHolder(std::string&& password) +CredentialsHolder::CredentialsHolder(std::string&& password, bool hashPlaintext) { bool locked = false; if (isHashingAvailable()) { if (!isPasswordHashed(password)) { - d_credentials = hashPassword(password); - locked = true; + if (hashPlaintext) { + d_credentials = hashPassword(password); + locked = true; + d_isHashed = true; + } } else { d_wasHashed = true; + d_isHashed = true; d_credentials = std::move(password); } } - else { + + if (!d_isHashed) { d_fallbackHashPerturb = random(); d_fallbackHash = burtle(reinterpret_cast(password.data()), password.size(), d_fallbackHashPerturb); d_credentials = std::move(password); @@ -130,7 +135,7 @@ CredentialsHolder::~CredentialsHolder() bool CredentialsHolder::matches(const std::string& password) const { - if (isHashingAvailable()) { + if (d_isHashed) { return verifyPassword(d_credentials, password); } else { diff --git a/pdns/credentials.hh b/pdns/credentials.hh index 970aa3fbcd..9d42669d6b 100644 --- a/pdns/credentials.hh +++ b/pdns/credentials.hh @@ -32,7 +32,7 @@ class CredentialsHolder public: /* if the password is in cleartext and hashing is available, the hashed form will be kept in memory */ - CredentialsHolder(std::string&& password); + CredentialsHolder(std::string&& password, bool hashPlaintext); ~CredentialsHolder(); CredentialsHolder(const CredentialsHolder&) = delete; @@ -44,6 +44,11 @@ public: { return d_wasHashed; } + /* whether it is hashed in memory */ + bool isHashed() const + { + return d_isHashed; + } static bool isHashingAvailable(); @@ -53,4 +58,6 @@ private: uint32_t d_fallbackHash{0}; /* whether it was constructed from a hashed and salted string */ bool d_wasHashed{false}; + /* whether it is hashed in memory */ + bool d_isHashed{false}; }; diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc index 79d019f1da..b09f4ba076 100644 --- a/pdns/dnsdist-lua.cc +++ b/pdns/dnsdist-lua.cc @@ -944,7 +944,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) SListen(sock, 5); auto launch=[sock, local, password, apiKey, customHeaders, acl]() { if (password) { - auto holder = make_unique(std::string(*password)); + auto holder = make_unique(std::string(*password), false); if (!holder->wasHashed() && holder->isHashingAvailable()) { warnlog("Passing a plain-text password to 'webserver()' is deprecated, please use 'setWebserverConfig()' with a hashed password instead."); } @@ -953,7 +953,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) } if (apiKey) { - auto holder = make_unique(std::string(*apiKey)); + auto holder = make_unique(std::string(*apiKey), false); setWebserverAPIKey(std::move(holder)); } @@ -990,9 +990,14 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) return ; } + bool hashPlaintextCredentials = false; + if (vars->count("hashPlaintextCredentials")) { + hashPlaintextCredentials = boost::get(vars->at("hashPlaintextCredentials")); + } + if (vars->count("password")) { std::string password = boost::get(vars->at("password")); - auto holder = make_unique(std::move(password)); + auto holder = make_unique(std::move(password), hashPlaintextCredentials); if (!holder->wasHashed() && holder->isHashingAvailable()) { warnlog("Passing a plain-text password via the 'password' parameter to 'setWebserverConfig()' is deprecated, please generate a hashed one using 'hashPassword()' instead."); } @@ -1002,7 +1007,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck) if (vars->count("apiKey")) { std::string apiKey = boost::get(vars->at("apiKey")); - auto holder = make_unique(std::move(apiKey)); + auto holder = make_unique(std::move(apiKey), hashPlaintextCredentials); if (!holder->wasHashed() && holder->isHashingAvailable()) { warnlog("Passing a plain-text API key via the 'apiKey' parameter to 'setWebserverConfig()' is deprecated, please generate a hashed one using 'hashPassword()' instead."); } diff --git a/pdns/dnsdistdist/docs/reference/config.rst b/pdns/dnsdistdist/docs/reference/config.rst index 815d93d2ed..296c239777 100644 --- a/pdns/dnsdistdist/docs/reference/config.rst +++ b/pdns/dnsdistdist/docs/reference/config.rst @@ -335,6 +335,7 @@ Webserver configuration .. versionchanged:: 1.7.0 The optional ``password`` and ``apiKey`` parameters now accept hashed passwords. + The optional ``hashPlaintextCredentials`` parameter has been added. Setup webserver configuration. See :func:`webserver`. @@ -348,6 +349,7 @@ Webserver configuration * ``acl=newACL``: string - List of IP addresses, as a string, that are allowed to open a connection to the web server. Defaults to "127.0.0.1, ::1". * ``statsRequireAuthentication``: bool - Whether access to the statistics (/metrics and /jsonstat endpoints) require a valid password or API key. Defaults to true. * ``maxConcurrentConnections``: int - The maximum number of concurrent web connections, or 0 which means an unlimited number. Defaults to 100. + * ``hashPlaintextCredentials``: bool - Whether passwords and API keys provided in plaintext should be hashed during startup, to prevent the plaintext versions from staying in memory. Doing so increases significantly the cost of verifying credentials. Defaults to false. .. function:: registerWebHandler(path, handler) diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index a8228e74cc..66dd782918 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -5746,6 +5746,7 @@ int main(int argc, char **argv) ::arg().set("webserver-password", "Password required for accessing the webserver") = ""; ::arg().set("webserver-allow-from","Webserver access is only allowed from these subnets")="127.0.0.1,::1"; ::arg().set("webserver-loglevel", "Amount of logging in the webserver (none, normal, detailed)") = "normal"; + ::arg().setSwitch("webserver-hash-plaintext-credentials","Whether to hash passwords and api keys supplied in plaintext, to prevent keeping the plaintext version in memory at runtime")="no"; ::arg().set("carbon-ourname", "If set, overrides our reported hostname for carbon stats")=""; ::arg().set("carbon-server", "If set, send metrics in carbon (graphite) format to this server IP address")=""; ::arg().set("carbon-interval", "Number of seconds between carbon (graphite) updates")="30"; diff --git a/pdns/recursordist/docs/settings.rst b/pdns/recursordist/docs/settings.rst index 8fb965af73..6e16c30d12 100644 --- a/pdns/recursordist/docs/settings.rst +++ b/pdns/recursordist/docs/settings.rst @@ -2110,6 +2110,17 @@ These IPs and subnets are allowed to access the webserver. Note that specifying an IP address without a netmask uses an implicit netmask of /32 or /128. +.. _setting-webserver-hash-plaintext-credentials: + +``webserver-hash-plaintext-credentials`` +---------------------------------------- +..versionadded:: 4.6.0 + +- Boolean +- Default: no + +Whether passwords and API keys supplied as plaintext should be hashed during startup, to prevent the plaintext versions from staying in memory. Doing so increases significantly the cost of verifying credentials. + .. _setting-webserver-loglevel: ``webserver-loglevel`` diff --git a/pdns/test-credentials_cc.cc b/pdns/test-credentials_cc.cc index a1853a82ab..145ab311b5 100644 --- a/pdns/test-credentials_cc.cc +++ b/pdns/test-credentials_cc.cc @@ -35,20 +35,28 @@ BOOST_AUTO_TEST_CASE(test_CredentialsHolder) { const std::string plaintext("test"); - auto holder = CredentialsHolder(std::string(plaintext)); + auto holder = CredentialsHolder(std::string(plaintext), false); BOOST_CHECK(holder.matches(plaintext)); BOOST_CHECK(!holder.matches("not test")); BOOST_CHECK(!holder.wasHashed()); + BOOST_CHECK(!holder.isHashed()); #ifdef HAVE_CRYPTO_PWHASH_STR BOOST_CHECK(CredentialsHolder::isHashingAvailable()); const std::string sampleHash("$argon2id$v=19$m=65536,t=2,p=1$ndQKu3+ZsWedqRrlNFUaNw$tnb0MJVe5C2hlqkDt0Ln3R6VKCYkfMYdxDy+puXes3s"); - auto fromHashedHolder = CredentialsHolder(std::string(sampleHash)); + auto fromHashedHolder = CredentialsHolder(std::string(sampleHash), true); BOOST_CHECK(fromHashedHolder.wasHashed()); + BOOST_CHECK(fromHashedHolder.isHashed()); BOOST_CHECK(fromHashedHolder.matches(plaintext)); BOOST_CHECK(!fromHashedHolder.matches("not test")); + + auto fromPlaintextHolder = CredentialsHolder(std::string(plaintext), true); + BOOST_CHECK(!fromPlaintextHolder.wasHashed()); + BOOST_CHECK(fromPlaintextHolder.isHashed()); + BOOST_CHECK(fromPlaintextHolder.matches(plaintext)); + BOOST_CHECK(!fromPlaintextHolder.matches("not test")); #else BOOST_CHECK(!CredentialsHolder::isHashingAvailable()); #endif diff --git a/pdns/webserver.hh b/pdns/webserver.hh index 2fff9c5009..d8707e0914 100644 --- a/pdns/webserver.hh +++ b/pdns/webserver.hh @@ -163,18 +163,18 @@ public: WebServer(string listenaddress, int port); virtual ~WebServer() { }; - void setApiKey(const string &apikey) { + void setApiKey(const string &apikey, bool hashPlaintext) { if (!apikey.empty()) { - d_apikey = make_unique(std::string(apikey)); + d_apikey = make_unique(std::string(apikey), hashPlaintext); } else { d_apikey.reset(); } } - void setPassword(const string &password) { + void setPassword(const string &password, bool hashPlaintext) { if (!password.empty()) { - d_webserverPassword = make_unique(std::string(password)); + d_webserverPassword = make_unique(std::string(password), hashPlaintext); } else { d_webserverPassword.reset(); diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc index b1a39a86de..2aae0c300c 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -65,10 +65,10 @@ AuthWebServer::AuthWebServer() : d_min5(0), d_min1(0) { - if(arg().mustDo("webserver") || arg().mustDo("api")) { + if (arg().mustDo("webserver") || arg().mustDo("api")) { d_ws = unique_ptr(new WebServer(arg()["webserver-address"], arg().asNum("webserver-port"))); - d_ws->setApiKey(arg()["api-key"]); - d_ws->setPassword(arg()["webserver-password"]); + d_ws->setApiKey(arg()["api-key"], arg().mustDo("webserver-hash-plaintext-credentials")); + d_ws->setPassword(arg()["webserver-password"], arg().mustDo("webserver-hash-plaintext-credentials")); d_ws->setLogLevel(arg()["webserver-loglevel"]); NetmaskGroup acl; diff --git a/pdns/ws-recursor.cc b/pdns/ws-recursor.cc index 58d310e469..2b0a87068d 100644 --- a/pdns/ws-recursor.cc +++ b/pdns/ws-recursor.cc @@ -1122,8 +1122,8 @@ RecursorWebServer::RecursorWebServer(FDMultiplexer* fdm) d_ws = make_unique(fdm, arg()["webserver-address"], arg().asNum("webserver-port")); - d_ws->setApiKey(arg()["api-key"]); - d_ws->setPassword(arg()["webserver-password"]); + d_ws->setApiKey(arg()["api-key"], arg().mustDo("webserver-hash-plaintext-credentials")); + d_ws->setPassword(arg()["webserver-password"], arg().mustDo("webserver-hash-plaintext-credentials")); d_ws->setLogLevel(arg()["webserver-loglevel"]); NetmaskGroup acl; -- 2.47.2