system-proxy: Secure system credentials
Currently System-proxy sends the policy set credentials with every
connect request to a remote proxy. Since less secure authentication
schemes send the credentials in clear to the proxy, an attacker can
easily obtain the policy set credentials.
To protect against a downgrade attack, this CL restricts the auth
schemes for which the policy set credentials can be applied.
BUG=chromium:1132247
TEST=HttpServerProxyConnectJobTest.PolicyAuth*
Change-Id: I17e2d3e38b1560f0fadf347657bd3f4b6e1bae09
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2483831
Tested-by: Andreea-Elena Costinas <acostinas@google.com>
Commit-Queue: Andreea-Elena Costinas <acostinas@google.com>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
diff --git a/system-proxy/connect_headers_parser_fuzzer.cc b/system-proxy/connect_headers_parser_fuzzer.cc
index 838710c..e8bf56e 100644
--- a/system-proxy/connect_headers_parser_fuzzer.cc
+++ b/system-proxy/connect_headers_parser_fuzzer.cc
@@ -8,6 +8,8 @@
#include <sys/socket.h>
#include <sys/types.h>
+#include <curl/curl.h>
+
#include <base/bind.h>
#include <base/files/file_util.h>
#include <base/logging.h>
@@ -72,6 +74,7 @@
auto connect_job = std::make_unique<system_proxy::ProxyConnectJob>(
std::make_unique<patchpanel::Socket>(base::ScopedFD(fds[0])), "",
+ CURLAUTH_ANY,
base::BindOnce(&ResolveProxyCallback, run_loop.QuitClosure()),
base::BindRepeating(&NullAuthenticationRequiredCallback),
base::BindOnce(&OnConnectionSetupFinished, run_loop.QuitClosure()));
diff --git a/system-proxy/http_util.cc b/system-proxy/http_util.cc
index af923a5..780c3fd 100644
--- a/system-proxy/http_util.cc
+++ b/system-proxy/http_util.cc
@@ -7,9 +7,12 @@
#include <array>
#include <string_view>
+#include <curl/curl.h>
+
#include <base/strings/stringprintf.h>
#include <base/strings/string_split.h>
#include <base/strings/string_tokenizer.h>
+#include <base/strings/string_util.h>
namespace {
// The elements in this array are used to identify the end of a HTTP header
@@ -108,5 +111,4 @@
return scheme_realm_pairs;
}
-
} // namespace system_proxy
diff --git a/system-proxy/http_util.h b/system-proxy/http_util.h
index e6d4587..125e1c1 100644
--- a/system-proxy/http_util.h
+++ b/system-proxy/http_util.h
@@ -45,7 +45,6 @@
// Parses the HTTP server reply and extracts the supported authentication scheme
// and realm.
SchemeRealmPairList ParseAuthChallenge(const base::StringPiece& http_request);
-
} // namespace system_proxy
#endif // SYSTEM_PROXY_HTTP_UTIL_H_
diff --git a/system-proxy/proto/worker_common.proto b/system-proxy/proto/worker_common.proto
index e0f904c..f2f5993 100644
--- a/system-proxy/proto/worker_common.proto
+++ b/system-proxy/proto/worker_common.proto
@@ -20,6 +20,9 @@
optional string username = 1;
optional string password = 2;
optional ProtectionSpace protection_space = 3;
+ // Authentication schemes for which policy set credentials can be
+ // automatically applied. Valid values are 'basic', 'digest' and 'ntlm'.
+ repeated string policy_credentials_auth_schemes = 4;
}
message SocketAddress {
diff --git a/system-proxy/proxy_connect_job.cc b/system-proxy/proxy_connect_job.cc
index c2ba1bc..ee228b8 100644
--- a/system-proxy/proxy_connect_job.cc
+++ b/system-proxy/proxy_connect_job.cc
@@ -109,13 +109,30 @@
return size * nmemb;
}
+// This callback receives debug information from curl, as specified in the
+// `type` argument (e.g. incoming or outgoing HTTP headers, SSL data).
+static size_t WriteDebugInfoCallback(CURL* handle,
+ curl_infotype type,
+ char* contents,
+ size_t size,
+ void* userdata) {
+ // We're only interested in outgoing headers for testing.
+ if (type != CURLINFO_HEADER_OUT)
+ return 0;
+ std::string* headers = (std::string*)userdata;
+ *headers = std::string(contents, size);
+ return 0;
+}
+
ProxyConnectJob::ProxyConnectJob(
std::unique_ptr<patchpanel::Socket> socket,
const std::string& credentials,
+ int64_t curl_auth_schemes,
ResolveProxyCallback resolve_proxy_callback,
AuthenticationRequiredCallback auth_required_callback,
OnConnectionSetupFinishedCallback setup_finished_callback)
: credentials_(credentials),
+ curl_auth_schemes_(curl_auth_schemes),
resolve_proxy_callback_(std::move(resolve_proxy_callback)),
auth_required_callback_(std::move(auth_required_callback)),
setup_finished_callback_(std::move(setup_finished_callback)),
@@ -147,6 +164,13 @@
return true;
}
+void ProxyConnectJob::StoreRequestHeadersForTesting() {
+ store_headers_for_testing_ = true;
+}
+std::string ProxyConnectJob::GetRequestHeadersForTesting() {
+ return request_headers_for_testing_;
+}
+
void ProxyConnectJob::OnClientReadReady() {
// The first message should be a HTTP CONNECT request.
std::vector<char> buf(kMaxHttpRequestHeadersSize);
@@ -242,6 +266,9 @@
return;
}
credentials_ = credentials;
+ // Covers the case for which `curl_auth_schemes_` was initialized with policy
+ // set schemes which are not supported by the remote remote server.
+ curl_auth_schemes_ = CURLAUTH_ANY;
VLOG(1) << "Connecting to the remote server with provided credentials";
DoCurlServerConnection();
}
@@ -287,7 +314,7 @@
curl_easy_setopt(easyhandle, CURLOPT_CONNECT_ONLY, 1);
// Allow libcurl to pick authentication method. Curl will use the most
// secure one the remote site claims to support.
- curl_easy_setopt(easyhandle, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+ curl_easy_setopt(easyhandle, CURLOPT_PROXYAUTH, curl_auth_schemes_);
curl_easy_setopt(easyhandle, CURLOPT_PROXYUSERPWD, credentials_.c_str());
}
curl_easy_setopt(easyhandle, CURLOPT_CONNECTTIMEOUT_MS,
@@ -296,7 +323,13 @@
curl_easy_setopt(easyhandle, CURLOPT_HEADERDATA, &http_response_headers);
curl_easy_setopt(easyhandle, CURLOPT_WRITEFUNCTION, WriteCallback);
curl_easy_setopt(easyhandle, CURLOPT_WRITEDATA, &http_response_body);
-
+ if (store_headers_for_testing_) {
+ curl_easy_setopt(easyhandle, CURLOPT_DEBUGFUNCTION, WriteDebugInfoCallback);
+ curl_easy_setopt(easyhandle, CURLOPT_DEBUGDATA,
+ &request_headers_for_testing_);
+ // The DEBUGFUNCTION has no effect until we enable VERBOSE.
+ curl_easy_setopt(easyhandle, CURLOPT_VERBOSE, 1L);
+ }
res = curl_easy_perform(easyhandle);
curl_easy_getinfo(easyhandle, CURLINFO_HTTP_CONNECTCODE,
&http_response_code_);
diff --git a/system-proxy/proxy_connect_job.h b/system-proxy/proxy_connect_job.h
index b872c1c..004093c 100644
--- a/system-proxy/proxy_connect_job.h
+++ b/system-proxy/proxy_connect_job.h
@@ -71,6 +71,7 @@
ProxyConnectJob(std::unique_ptr<patchpanel::Socket> socket,
const std::string& credentials,
+ int64_t curl_auth_schemes,
ResolveProxyCallback resolve_proxy_callback,
AuthenticationRequiredCallback auth_required_callback,
OnConnectionSetupFinishedCallback setup_finished_callback);
@@ -83,6 +84,12 @@
virtual bool Start();
void OnProxyResolution(const std::list<std::string>& proxy_servers);
+ // Enables storing curl debug information for test.
+ void StoreRequestHeadersForTesting();
+ // Returns the HTTP headers sent by the curl client. Tests must call
+ // `StoreRequestHeadersForTesting` before calling this method.
+ std::string GetRequestHeadersForTesting();
+
friend std::ostream& operator<<(std::ostream& stream,
const ProxyConnectJob& job);
@@ -104,6 +111,8 @@
FRIEND_TEST(HttpServerProxyConnectJobTest, MultipleReadConnectRequest);
FRIEND_TEST(HttpServerProxyConnectJobTest, BufferedClientData);
FRIEND_TEST(HttpServerProxyConnectJobTest, BufferedClientDataAltEnding);
+ FRIEND_TEST(HttpServerProxyConnectJobTest, PolicyAuthSchemeOk);
+ FRIEND_TEST(HttpServerProxyConnectJobTest, PolicyAuthBadScheme);
// Called when the client socket is ready for reading.
void OnClientReadReady();
@@ -150,6 +159,9 @@
bool authentication_timer_started_ = false;
std::string credentials_;
+ // Bitmask to tell curl which authentication methods are allowed to be used
+ // with `credentials_`. This value is set with the `CURLOPT_PROXYAUTH` option.
+ int64_t curl_auth_schemes_;
// CONNECT request and playload data read from the client socket. The client
// may send the HTTP CONNECT request across multiple socket writes.
// |connect_data_| will cache the partial messages until we receive the end of
@@ -169,6 +181,9 @@
// required).
base::CancelableClosure credentials_request_timeout_callback_;
+ bool store_headers_for_testing_ = false;
+ std::string request_headers_for_testing_;
+
std::unique_ptr<patchpanel::Socket> client_socket_;
std::unique_ptr<base::FileDescriptorWatcher::Controller> read_watcher_;
base::WeakPtrFactory<ProxyConnectJob> weak_ptr_factory_{this};
diff --git a/system-proxy/proxy_connect_job_test.cc b/system-proxy/proxy_connect_job_test.cc
index 2fd4184..0c352c2 100644
--- a/system-proxy/proxy_connect_job_test.cc
+++ b/system-proxy/proxy_connect_job_test.cc
@@ -12,14 +12,18 @@
#include <gtest/gtest.h>
#include <utility>
+#include <curl/curl.h>
+
#include <base/bind.h>
#include <base/bind_helpers.h>
#include <base/callback_helpers.h>
#include <base/files/file_util.h>
#include <base/files/scoped_file.h>
+#include <base/strings/stringprintf.h>
#include <base/task/single_thread_task_executor.h>
#include <base/test/test_mock_time_task_runner.h>
#include <brillo/message_loops/base_message_loop.h>
+#include <chromeos/patchpanel/net_util.h>
#include <chromeos/patchpanel/socket.h>
#include <chromeos/patchpanel/socket_forwarder.h>
@@ -32,6 +36,7 @@
constexpr char kCredentials[] = "username:pwd";
constexpr char kValidConnectRequest[] =
"CONNECT www.example.server.com:443 HTTP/1.1\r\n\r\n";
+constexpr char kProxyAuthorizationHeaderToken[] = "Proxy-Authorization:";
} // namespace
namespace system_proxy {
@@ -70,6 +75,7 @@
connect_job_ = std::make_unique<ProxyConnectJob>(
std::make_unique<patchpanel::Socket>(base::ScopedFD(fds[0])), "",
+ CURLAUTH_ANY,
base::BindOnce(&ProxyConnectJobTest::ResolveProxy,
base::Unretained(this)),
base::BindRepeating(&ProxyConnectJobTest::OnAuthCredentialsRequired,
@@ -200,6 +206,7 @@
auto connect_job = std::make_unique<ProxyConnectJob>(
std::make_unique<patchpanel::Socket>(base::ScopedFD(fds[0])), "",
+ CURLAUTH_ANY,
base::BindOnce(&ProxyConnectJobTest::ResolveProxy,
base::Unretained(this)),
base::BindRepeating(&ProxyConnectJobTest::OnAuthCredentialsRequired,
@@ -515,4 +522,46 @@
const std::string actual(connect_job_->connect_data_.data(),
connect_job_->connect_data_.size());
}
+
+// Test that the policy auth scheme is respected by curl.
+TEST_F(HttpServerProxyConnectJobTest, PolicyAuthSchemeOk) {
+ AddServerReply(HttpTestServer::HttpConnectReply::kAuthRequiredBasic);
+ http_test_server_.Start();
+ connect_job_->credentials_ = "a:b";
+ connect_job_->curl_auth_schemes_ = CURLAUTH_BASIC;
+ connect_job_->StoreRequestHeadersForTesting();
+ connect_job_->Start();
+
+ cros_client_socket_->SendTo(kValidConnectRequest,
+ std::strlen(kValidConnectRequest));
+ brillo_loop_->RunOnce(false);
+
+ std::size_t pos = connect_job_->GetRequestHeadersForTesting().find(
+ kProxyAuthorizationHeaderToken);
+ // Expect to find the proxy auth headers since CURLAUTH_BASIC is an allowed
+ // auth scheme.
+ EXPECT_NE(pos, std::string::npos);
+}
+
+// Test that proxy auth headers with credentials are not sent by curl if the
+// auth scheme used by the server is not allowed.
+TEST_F(HttpServerProxyConnectJobTest, PolicyAuthBadScheme) {
+ AddServerReply(HttpTestServer::HttpConnectReply::kAuthRequiredBasic);
+ http_test_server_.Start();
+ connect_job_->credentials_ = "a:b";
+ connect_job_->curl_auth_schemes_ = CURLAUTH_DIGEST;
+ connect_job_->StoreRequestHeadersForTesting();
+ connect_job_->Start();
+
+ cros_client_socket_->SendTo(kValidConnectRequest,
+ std::strlen(kValidConnectRequest));
+ brillo_loop_->RunOnce(false);
+
+ std::size_t pos = connect_job_->GetRequestHeadersForTesting().find(
+ kProxyAuthorizationHeaderToken);
+ // Expect not to find the proxy auth headers since CURLAUTH_BASIC is not an
+ // allowed auth scheme.
+ EXPECT_EQ(pos, std::string::npos);
+}
+
} // namespace system_proxy
diff --git a/system-proxy/server_proxy.cc b/system-proxy/server_proxy.cc
index 13ea7cb..3cef840 100644
--- a/system-proxy/server_proxy.cc
+++ b/system-proxy/server_proxy.cc
@@ -9,11 +9,14 @@
#include <utility>
#include <vector>
+#include <curl/curl.h>
+
#include <base/bind.h>
#include <base/bind_helpers.h>
#include <base/callback_helpers.h>
#include <base/posix/eintr_wrapper.h>
#include <base/files/file_util.h>
+#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
#include <base/threading/thread.h>
#include <base/threading/thread_task_runner_handle.h>
@@ -23,6 +26,7 @@
#include <chromeos/patchpanel/socket_forwarder.h>
#include "bindings/worker_common.pb.h"
+#include "system-proxy/http_util.h"
#include "system-proxy/protobuf_util.h"
#include "system-proxy/proxy_connect_job.h"
@@ -48,6 +52,27 @@
/* encodeSpaceAsPlus= */ false);
}
+// Converts the list of proxy authentication schemes in string format to the
+// curl bit-mask format.
+int64_t GetAuthSchemes(
+ const google::protobuf::RepeatedPtrField<std::string>& auth_schemes) {
+ if (auth_schemes.empty())
+ return CURLAUTH_ANY;
+ // Convert auth schemes to curl format.
+ int64_t curl_scheme = CURLAUTH_NEGOTIATE;
+ // Auth scheme is case insensitive, see
+ // https://tools.ietf.org/html/rfc7235#section-2.1
+ for (auto const& scheme : auth_schemes) {
+ const std::string lower_scheme = base::ToLowerASCII(scheme);
+ if (lower_scheme == "basic")
+ curl_scheme |= CURLAUTH_BASIC;
+ if (lower_scheme == "digest")
+ curl_scheme |= CURLAUTH_DIGEST;
+ if (lower_scheme == "ntlm")
+ curl_scheme |= CURLAUTH_NTLM;
+ }
+ return curl_scheme;
+}
} // namespace
ServerProxy::ServerProxy(base::OnceClosure quit_closure)
@@ -167,6 +192,8 @@
AuthCredentialsProvided(auth_key, std::string());
}
} else {
+ system_credentials_auth_schemes_ = GetAuthSchemes(
+ config.credentials().policy_credentials_auth_schemes());
system_credentials_ = credentials;
}
}
@@ -256,6 +283,7 @@
listening_fd_->Accept((struct sockaddr*)&client_src, &sockaddr_len)) {
auto connect_job = std::make_unique<ProxyConnectJob>(
std::move(client_conn), system_credentials_,
+ system_credentials_auth_schemes_,
base::BindOnce(&ServerProxy::ResolveProxy, base::Unretained(this)),
base::BindRepeating(&ServerProxy::AuthenticationRequired,
base::Unretained(this)),
diff --git a/system-proxy/server_proxy.h b/system-proxy/server_proxy.h
index 2f3de43..094e664 100644
--- a/system-proxy/server_proxy.h
+++ b/system-proxy/server_proxy.h
@@ -126,6 +126,10 @@
// proxy it's connecting to or the challenge response.
std::string system_credentials_;
+ // Curl compatible bit-mask list of proxy authenticated schemes that can be
+ // used with the policy set credentials.
+ int64_t system_credentials_auth_schemes_ = 0;
+
std::unique_ptr<patchpanel::Socket> listening_fd_;
// List of SocketForwarders that corresponds to the TCP tunnel between the
diff --git a/system-proxy/server_proxy_test.cc b/system-proxy/server_proxy_test.cc
index 3106b59..f84607b 100644
--- a/system-proxy/server_proxy_test.cc
+++ b/system-proxy/server_proxy_test.cc
@@ -8,6 +8,8 @@
#include <sys/socket.h>
#include <sys/types.h>
+#include <curl/curl.h>
+
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <utility>
@@ -62,6 +64,7 @@
OnConnectionSetupFinishedCallback setup_finished_callback)
: ProxyConnectJob(std::move(socket),
credentials,
+ CURLAUTH_ANY,
std::move(resolve_proxy_callback),
std::move(auth_required_callback),
std::move(setup_finished_callback)) {}
@@ -117,6 +120,9 @@
worker::Credentials credentials;
credentials.set_username(kUsername);
credentials.set_password(kPassword);
+ credentials.add_policy_credentials_auth_schemes("basic");
+ credentials.add_policy_credentials_auth_schemes("digest");
+
worker::WorkerConfigs configs;
*configs.mutable_credentials() = credentials;
RedirectStdPipes();
@@ -128,6 +134,8 @@
std::string expected_credentials =
base::JoinString({kUsernameEncoded, kPasswordEncoded}, ":");
EXPECT_EQ(server_proxy_->system_credentials_, expected_credentials);
+ EXPECT_EQ(server_proxy_->system_credentials_auth_schemes_,
+ CURLAUTH_BASIC | CURLAUTH_DIGEST | CURLAUTH_NEGOTIATE);
}
TEST_F(ServerProxyTest, FetchListeningAddress) {
diff --git a/system-proxy/system_proxy_adaptor.cc b/system-proxy/system_proxy_adaptor.cc
index e6bf877..9d57214 100644
--- a/system-proxy/system_proxy_adaptor.cc
+++ b/system-proxy/system_proxy_adaptor.cc
@@ -120,10 +120,12 @@
}
if (auth_details.has_credentials()) {
- if (auth_details.credentials().has_username() &&
- auth_details.credentials().has_password()) {
- credentials.set_username(auth_details.credentials().username());
- credentials.set_password(auth_details.credentials().password());
+ system_proxy::Credentials dbus_cred = auth_details.credentials();
+ if (dbus_cred.has_username() && dbus_cred.has_password()) {
+ credentials.set_username(dbus_cred.username());
+ credentials.set_password(dbus_cred.password());
+ credentials.mutable_policy_credentials_auth_schemes()->Swap(
+ dbus_cred.mutable_policy_credentials_auth_schemes());
}
}
diff --git a/system-proxy/system_proxy_adaptor_test.cc b/system-proxy/system_proxy_adaptor_test.cc
index 5d33bbc..b8c1ac6 100644
--- a/system-proxy/system_proxy_adaptor_test.cc
+++ b/system-proxy/system_proxy_adaptor_test.cc
@@ -221,6 +221,9 @@
Credentials credentials;
credentials.set_username(kUser);
credentials.set_password(kPassword);
+ credentials.add_policy_credentials_auth_schemes("basic");
+ credentials.add_policy_credentials_auth_schemes("digest");
+
*request.mutable_credentials() = credentials;
request.set_traffic_type(TrafficOrigin::ALL);
@@ -264,6 +267,9 @@
EXPECT_TRUE(config_user.has_credentials());
EXPECT_EQ(config_user.credentials().username(), kUser);
EXPECT_EQ(config_user.credentials().password(), kPassword);
+ EXPECT_GT(credentials.policy_credentials_auth_schemes().size(), 0);
+ EXPECT_EQ(credentials.policy_credentials_auth_schemes().Get(0), "basic");
+ EXPECT_EQ(credentials.policy_credentials_auth_schemes().Get(1), "digest");
}
// Verifies if System-proxy only starts the worker which tunnels system traffic.
diff --git a/system-proxy/test_http_server.cc b/system-proxy/test_http_server.cc
index efcba00..6fb9dbc 100644
--- a/system-proxy/test_http_server.cc
+++ b/system-proxy/test_http_server.cc
@@ -32,7 +32,7 @@
"\r\n";
const std::string_view kHttpBadGateway =
- "HTTP/1.1 502 Bad Gateway\r\n\r\nBag gateway message from the server";
+ "HTTP/1.1 502 Bad Gateway\r\n\r\nBad gateway message from the server";
} // namespace
namespace system_proxy {
@@ -62,7 +62,6 @@
std::string_view server_reply =
GetConnectReplyString(expected_responses_.front());
expected_responses_.pop();
- LOG(ERROR) << server_reply;
client_conn->SendTo(server_reply.data(), server_reply.size());
}
}