[kudu] 02/02: [rpc] avoid crashes on malicious negotiation attempts

[al...@apache.org]

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 <al...@apache.org>
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 <sh...@gmail.com>
    Reviewed-by: Yingchun Lai <ac...@gmail.com>
---
 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>();