From 86d33a0a3dca40d452e5a5868c399df2238fa934 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 3 Nov 2022 09:23:54 -0700 Subject: [PATCH] IMPALA-11856: Use POST requests to set log level Set and reset loglevel handlers now require a POST. Implements Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using the Double Submit Cookie pattern - where POST requests must include a csrf_token field in their post with the random value from the cookie - or a custom header. CSRF attacks rely on the browser always sending a cookie or 'Authorization: Basic' header. - With cookies, attacks don't have access to default form values or the original cookie, so we can include the cookie's random value in the form as a cross-check. As the cookie is cryptographically signed, they also can't be replaced with one that would match an attack's forms. - When not using cookies, a custom header (X-Requested-By) is required as CSRFs are unable to add custom headers. This approach is also used by Jersey; see http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf In a broader implementation this would require a separate cookie so it can be used to protect logins as well, but login is handled external to Impala so we re-use the cookie the page already has. Cookies are now generated for the HTPASSWD authentication method. Authenticating via JWT still omits cookies because the JWT is already provided via custom header (preventing CSRF) and disabling authentication (NONE) means anyone could directly send a request so CSRF protection is meaningless. We also start an additional Webserver instance with authentication NONE when metrics_webserver_port > 0, and the Webserver metric "impala.webserver.total-cookie-auth-success" can only be registered once. Additional changes would be necessary to make metric names unique in Webserver (based on port); for the moment we avoid that by ensuring all metrics counters are only instantiated for Webservers that use authentication. Cookie generation and authentication were updated to provide access to the random value. Adds flag to enable SameSite=Strict for defense in depth as mentioned in https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis. This can be enabled if another CSRF attack method is found. Verified that this prevents CSRF attacks by disabling SameSite=Strict and visiting (via https://security.love/CSRF-PoC-Genorator): ```
glog
``` Adds tests for the webserver with basic authentication, LDAP, and SPNEGO that authorization fails on POST unless - using a cookie and csrf_token is correctly set in the POST body - the X-Requested-By header is set Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693 Reviewed-on: http://gerrit.cloudera.org:8080/19199 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- be/src/rpc/authentication-util.cc | 35 +- be/src/rpc/authentication-util.h | 14 +- be/src/util/logging-support.cc | 24 +- be/src/util/webserver-test.cc | 313 +++++++++++++----- be/src/util/webserver.cc | 111 ++++++- be/src/util/webserver.h | 14 +- .../impala/customcluster/JwtHttpTest.java | 15 +- .../customcluster/JwtWebserverTest.java | 11 +- .../impala/customcluster/LdapHS2Test.java | 23 +- .../customcluster/LdapImpalaShellTest.java | 12 +- .../customcluster/LdapImpylaHttpTest.java | 26 +- .../impala/customcluster/LdapJdbcTest.java | 14 +- .../customcluster/LdapWebserverTest.java | 238 +++++++++++-- .../org/apache/impala/service/JdbcTest.java | 16 +- .../Metrics.java => testutil/WebClient.java} | 112 +++++-- tests/webserver/test_web_pages.py | 84 +++-- www/form-hidden-inputs.tmpl | 3 + www/log_level.tmpl | 8 +- 18 files changed, 802 insertions(+), 271 deletions(-) rename fe/src/test/java/org/apache/impala/{util/Metrics.java => testutil/WebClient.java} (50%) diff --git a/be/src/rpc/authentication-util.cc b/be/src/rpc/authentication-util.cc index b02a97ed8..36c67a6fa 100644 --- a/be/src/rpc/authentication-util.cc +++ b/be/src/rpc/authentication-util.cc @@ -35,6 +35,8 @@ DEFINE_bool(cookie_require_secure, true, DEFINE_int64(max_cookie_lifetime_s, 24 * 60 * 60, "Maximum amount of time in seconds that an authentication cookie will remain valid. " "Setting to 0 disables use of cookies. Defaults to 1 day."); +DEFINE_bool(samesite_strict, false, + "(Advanced) If true, authentication cookies will include SameSite=Strict."); using namespace strings; @@ -59,13 +61,14 @@ static const int SHA256_BASE64_LEN = CalculateBase64EscapedLen(AuthenticationHash::HashLen(), /* do_padding */ true); // Since we only return cookies with a single name, well behaved clients should only ever -// return one cookie to us. To accomodate non-malicious but poorly behaved clients, we +// return one cookie to us. To accommodate non-malicious but poorly behaved clients, we // allow for checking a limited number of cookies, up to MAX_COOKIES_TO_CHECK or until we // find the first one with COOKIE_NAME. static const int MAX_COOKIES_TO_CHECK = 5; Status AuthenticateCookie( - const AuthenticationHash& hash, const string& cookie_header, string* username) { + const AuthenticationHash& hash, const string& cookie_header, + string* username, string* rand) { // The 'Cookie' header allows sending multiple name/value pairs separated by ';'. vector cookies = strings::Split(cookie_header, ";"); if (cookies.size() > MAX_COOKIES_TO_CHECK) { @@ -125,6 +128,11 @@ Status AuthenticateCookie( if (!TryStripPrefixString(cookie_value_split[0], USERNAME_KEY, username)) { return Status("The cookie username value has an invalid format."); } + if (rand != nullptr) { + if (!TryStripPrefixString(cookie_value_split[2], RAND_KEY, rand)) { + return Status("The cookie rand value has an invalid format."); + } + } // We've successfully authenticated. return Status::OK(); } else { @@ -135,12 +143,18 @@ Status AuthenticateCookie( return Status(Substitute("Did not find expected cookie name: $0", COOKIE_NAME)); } -string GenerateCookie(const string& username, const AuthenticationHash& hash) { +string GenerateCookie(const string& username, const AuthenticationHash& hash, + std::string* srand) { // Its okay to use rand() here even though its a weak RNG because being able to guess // the random numbers generated won't help an attacker. The important thing is that // we're using a strong RNG to create the key and a strong HMAC function. + int cookie_rand = rand(); + string cookie_rand_s = std::to_string(cookie_rand); + if (srand != nullptr) { + *srand = cookie_rand_s; + } string cookie_value = StrCat(USERNAME_KEY, username, COOKIE_SEPARATOR, TIMESTAMP_KEY, - MonotonicMillis(), COOKIE_SEPARATOR, RAND_KEY, rand()); + MonotonicMillis(), COOKIE_SEPARATOR, RAND_KEY, cookie_rand_s); uint8_t signature[AuthenticationHash::HashLen()]; Status compute_status = hash.Compute(reinterpret_cast(cookie_value.data()), @@ -155,12 +169,13 @@ string GenerateCookie(const string& username, const AuthenticationHash& hash) { SHA256_BASE64_LEN, /* do_padding */ true); base64_signature[SHA256_BASE64_LEN] = '\0'; - const char* secure_flag = ";Secure"; - if (!FLAGS_cookie_require_secure) { - secure_flag = ""; - } - return Substitute("$0=$1$2$3;HttpOnly;Max-Age=$4$5", COOKIE_NAME, base64_signature, - COOKIE_SEPARATOR, cookie_value, FLAGS_max_cookie_lifetime_s, secure_flag); + const char* secure_flag = FLAGS_cookie_require_secure ? ";Secure" : ""; + const char* samesite_flag = FLAGS_samesite_strict ? ";SameSite=Strict" : ""; + // Add SameSite=Strict to notify the browser it should avoid sending the cookie with + // requests from other domains. + return Substitute("$0=$1$2$3;HttpOnly;Max-Age=$4$5$6", COOKIE_NAME, base64_signature, + COOKIE_SEPARATOR, cookie_value, FLAGS_max_cookie_lifetime_s, secure_flag, + samesite_flag); } string GetDeleteCookie() { diff --git a/be/src/rpc/authentication-util.h b/be/src/rpc/authentication-util.h index c07b7cc50..c901beea8 100644 --- a/be/src/rpc/authentication-util.h +++ b/be/src/rpc/authentication-util.h @@ -25,13 +25,15 @@ class AuthenticationHash; // Takes a single 'key=value' pair from a 'Cookie' header and attempts to verify its // signature with 'hash'. If verification is successful and the cookie is still valid, -// sets 'username' to the corresponding username and returns OK. -Status AuthenticateCookie(const AuthenticationHash& hash, - const std::string& cookie_header, std::string* username); +// sets 'username' and 'rand' (if specified) to the corresponding values and returns OK. +Status AuthenticateCookie( + const AuthenticationHash& hash, const std::string& cookie_header, + std::string* username, std::string* rand = nullptr); // Generates and returns a cookie containing the username set on 'connection_context' and -// a signature generated with 'hash'. -std::string GenerateCookie(const std::string& username, const AuthenticationHash& hash); +// a signature generated with 'hash'. If specified, sets 'rand' to the 'r=' cookie value. +std::string GenerateCookie(const std::string& username, const AuthenticationHash& hash, + std::string* rand = nullptr); // Returns a empty cookie. Returned in a 'Set-Cookie' when cookie auth fails to indicate // to the client that the cookie should be deleted. @@ -51,4 +53,6 @@ bool IsTrustedDomain(const std::string& origin, const std::string& trusted_domai Status BasicAuthExtractCredentials( const string& token, string& username, string& password); +constexpr int RAND_MAX_LENGTH = 10; + } // namespace impala diff --git a/be/src/util/logging-support.cc b/be/src/util/logging-support.cc index 459f913d2..9e2e103ad 100644 --- a/be/src/util/logging-support.cc +++ b/be/src/util/logging-support.cc @@ -149,7 +149,12 @@ void GetJavaLogLevels(Document* document) { // Callback handler for /set_java_loglevel. void SetJavaLogLevelCallback(const Webserver::WebRequest& req, Document* document) { - const auto& args = req.parsed_args; + if (req.request_method != "POST") { + AddDocumentMember("Use form input to update java log levels", "error", document); + return; + } + + Webserver::ArgumentMap args = Webserver::GetVars(req.post_data); Webserver::ArgumentMap::const_iterator classname = args.find("class"); Webserver::ArgumentMap::const_iterator level = args.find("level"); if (classname == args.end() || classname->second.empty() || @@ -179,6 +184,11 @@ void SetJavaLogLevelCallback(const Webserver::WebRequest& req, Document* documen // Callback handler for /reset_java_loglevel. void ResetJavaLogLevelCallback(const Webserver::WebRequest& req, Document* document) { + if (req.request_method != "POST") { + AddDocumentMember("Use form input to reset java log levels", "error", document); + return; + } + Status status = ResetJavaLogLevels(); if (!status.ok()) { AddDocumentMember(status.GetDetail(), "error", document); @@ -202,7 +212,12 @@ void GetGlogLevel(Document* document) { // Callback handler for /set_glog_level void SetGlogLevelCallback(const Webserver::WebRequest& req, Document* document) { - const auto& args = req.parsed_args; + if (req.request_method != "POST") { + AddDocumentMember("Use form input to update glog level", "error", document); + return; + } + + Webserver::ArgumentMap args = Webserver::GetVars(req.post_data); Webserver::ArgumentMap::const_iterator glog_level = args.find("glog"); if (glog_level == args.end() || glog_level->second.empty()) { AddDocumentMember("Bad glog level input. Valid inputs are integers in the " @@ -221,6 +236,11 @@ void SetGlogLevelCallback(const Webserver::WebRequest& req, Document* document) // Callback handler for /reset_glog_level void ResetGlogLevelCallback(const Webserver::WebRequest& req, Document* document) { + if (req.request_method != "POST") { + AddDocumentMember("Use form input to reset glog level", "error", document); + return; + } + string new_log_level = google::SetCommandLineOption("v", to_string(FLAGS_v_original_value).data()); VLOG(1) << "New glog level set: " << new_log_level; diff --git a/be/src/util/webserver-test.cc b/be/src/util/webserver-test.cc index 65e18f640..26ee1ee28 100644 --- a/be/src/util/webserver-test.cc +++ b/be/src/util/webserver-test.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -65,49 +66,74 @@ const string TO_ESCAPE_KEY = "ToEscape"; const string TO_ESCAPE_VALUE = "