This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 64d033d58a5f8bbcaf0f7d7afa37c53d67872170 Author: Alexey Serbin <[email protected]> AuthorDate: Wed Oct 19 16:20:34 2022 -0700 [rpc] avoid crashes on malicious negotiation attempts This patch updates the code for RPC connection negotiation to change from CHECK() to LOG(DFATAL) and Status::IllegalState() when checking on some security constraint. This narrows the attack vector of potential DOS attacks for Kudu servers. Change-Id: I416e3e952a3ee928ed6c51389227184c73a96b0b Reviewed-on: http://gerrit.cloudera.org:8080/19158 Tested-by: Kudu Jenkins Reviewed-by: Yuqi Du <[email protected]> Reviewed-by: Yingchun Lai <[email protected]> --- src/kudu/rpc/client_negotiation.cc | 7 ++++++- src/kudu/rpc/client_negotiation.h | 2 +- src/kudu/rpc/server_negotiation.cc | 30 ++++++++++++++++++++++++------ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc index 6dcc26ab2..09c5357f6 100644 --- a/src/kudu/rpc/client_negotiation.cc +++ b/src/kudu/rpc/client_negotiation.cc @@ -545,7 +545,12 @@ Status ClientNegotiation::AuthenticateByToken(faststring* recv_buf, unique_ptr<ErrorStatusPB>* rpc_error) { // Sanity check that TLS has been negotiated. Sending the token on an // unencrypted channel is a big no-no. - CHECK(tls_negotiated_); + if (PREDICT_FALSE(!tls_negotiated_)) { + constexpr const char* const kErrMsg = + "received authn token over an unencrypted channel"; + LOG(DFATAL) << kErrMsg; + return Status::IllegalState(kErrMsg); + } // Send the token to the server. NegotiatePB pb; diff --git a/src/kudu/rpc/client_negotiation.h b/src/kudu/rpc/client_negotiation.h index afaf7f068..4481bb59f 100644 --- a/src/kudu/rpc/client_negotiation.h +++ b/src/kudu/rpc/client_negotiation.h @@ -36,6 +36,7 @@ #include "kudu/rpc/sasl_helper.h" #include "kudu/security/security_flags.h" #include "kudu/security/tls_handshake.h" +#include "kudu/security/token.pb.h" #include "kudu/util/monotime.h" #include "kudu/util/net/socket.h" #include "kudu/util/status.h" @@ -46,7 +47,6 @@ class Slice; class faststring; namespace security { -class SignedTokenPB; class TlsContext; } diff --git a/src/kudu/rpc/server_negotiation.cc b/src/kudu/rpc/server_negotiation.cc index 6e5c5f929..332a30534 100644 --- a/src/kudu/rpc/server_negotiation.cc +++ b/src/kudu/rpc/server_negotiation.cc @@ -654,10 +654,15 @@ Status ServerNegotiation::AuthenticateBySasl(faststring* recv_buf) { RETURN_NOT_OK(s); const char* c_username = nullptr; - int rc = sasl_getprop(sasl_conn_.get(), SASL_USERNAME, - reinterpret_cast<const void**>(&c_username)); + const int rc = sasl_getprop(sasl_conn_.get(), SASL_USERNAME, + reinterpret_cast<const void**>(&c_username)); // We expect that SASL_USERNAME will always get set. - CHECK(rc == SASL_OK && c_username != nullptr) << "No username on authenticated connection"; + if (PREDICT_FALSE(rc != SASL_OK || c_username == nullptr)) { + constexpr const char* const kErrMsg = + "no username on authenticated connection"; + LOG(DFATAL) << kErrMsg; + return Status::IllegalState(kErrMsg); + } if (negotiated_mech_ == SaslMechanism::GSSAPI) { // The SASL library doesn't include the user's realm in the username if it's the // same realm as the default realm of the server. So, we pass it back through the @@ -683,7 +688,12 @@ Status ServerNegotiation::AuthenticateBySasl(faststring* recv_buf) { Status ServerNegotiation::AuthenticateByToken(faststring* recv_buf) { // Sanity check that TLS has been negotiated. Receiving the token on an // unencrypted channel is a big no-no. - CHECK(tls_negotiated_); + if (PREDICT_FALSE(!tls_negotiated_)) { + constexpr const char* const kErrMsg = + "received authn token over an unencrypted channel"; + LOG(DFATAL) << kErrMsg; + return Status::IllegalState(kErrMsg); + } // Receive the token from the client. NegotiatePB pb; @@ -765,7 +775,12 @@ Status ServerNegotiation::AuthenticateByToken(faststring* recv_buf) { Status ServerNegotiation::AuthenticateByCertificate() { // Sanity check that TLS has been negotiated. Cert-based authentication is // only possible with TLS. - CHECK(tls_negotiated_); + if (PREDICT_FALSE(!tls_negotiated_)) { + constexpr const char* const kErrMsg = + "received client certificate over an unencrypted channel"; + LOG(DFATAL) << kErrMsg; + return Status::IllegalState(kErrMsg); + } // Grab the subject from the client's cert. security::Cert cert; @@ -1012,7 +1027,10 @@ int ServerNegotiation::PlainAuthCb(sasl_conn_t* /*conn*/, } bool ServerNegotiation::IsTrustedConnection(const Sockaddr& addr) { - if (addr.family() == AF_UNIX) return true; + if (addr.family() == AF_UNIX) { + return true; + } + static std::once_flag once; std::call_once(once, [] { g_trusted_subnets = new vector<Network>();
