kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: [security] protect against master SASL negotiation short-circuit
Date Wed, 01 Mar 2017 07:17:19 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 9591957b8 -> 8f8dfe174


[security] protect against master SASL negotiation short-circuit

In both clients we weren't checking that the local SASL client
considered the negotiation to be complete when the server sent us a
SASL_SUCCESS message. This would allow a malicious server to trick the
client into thinking it had authenticated the server, when in reality it
had not. This ended up being easier in the Java client because the JDK
SASL API includes an 'isComplete', and there is not equivalent in cyrus
SASL, so the state has to be tracked explicitly.

Change-Id: I8f3b3d4f47e887b48c1c704c900e9260c22cec3a
Reviewed-on: http://gerrit.cloudera.org:8080/6148
Reviewed-by: Todd Lipcon <todd@apache.org>
Tested-by: Kudu Jenkins


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/8f8dfe17
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/8f8dfe17
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/8f8dfe17

Branch: refs/heads/master
Commit: 8f8dfe174a943bbe833f6be1ebdbfc4313f5c1ec
Parents: 9591957
Author: Dan Burkert <danburkert@apache.org>
Authored: Fri Feb 24 14:38:31 2017 -0800
Committer: Dan Burkert <danburkert@apache.org>
Committed: Wed Mar 1 07:17:01 2017 +0000

----------------------------------------------------------------------
 .../java/org/apache/kudu/client/Negotiator.java | 12 ++++-
 src/kudu/rpc/client_negotiation.cc              | 51 ++++++++++++--------
 src/kudu/rpc/client_negotiation.h               |  8 +++
 3 files changed, 49 insertions(+), 22 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/8f8dfe17/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java b/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
index 1457fcf..25d0ac2 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
@@ -544,7 +544,7 @@ public class Negotiator extends SimpleChannelUpstreamHandler {
         "expected TOKEN_EXCHANGE, got step: {}", response.getStep());
 
     // The token response doesn't have any actual data in it, so we can just move on.
-    handleSuccessResponse(chan, response);
+    finish(chan);
   }
 
   private void sendSaslInitiate(Channel chan) throws SaslException {
@@ -599,10 +599,20 @@ public class Negotiator extends SimpleChannelUpstreamHandler {
   }
 
   private void handleSuccessResponse(Channel chan, NegotiatePB response) {
+    Preconditions.checkState(saslClient.isComplete(),
+                             "server sent SASL_SUCCESS step, but SASL negotiation is not
complete");
     if (peerCert != null && "GSSAPI".equals(chosenMech)) {
       verifyChannelBindings(response);
     }
 
+    finish(chan);
+  }
+
+  /**
+   * Marks the negotiation as finished, and sends the connection context to the server.
+   * @param chan the connection channel
+   */
+  private void finish(Channel chan) {
     state = State.FINISHED;
     chan.getPipeline().remove(this);
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/8f8dfe17/src/kudu/rpc/client_negotiation.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc
index ec27855..cfcb68f 100644
--- a/src/kudu/rpc/client_negotiation.cc
+++ b/src/kudu/rpc/client_negotiation.cc
@@ -473,24 +473,23 @@ Status ClientNegotiation::HandleTlsHandshake(const NegotiatePB&
response) {
 
 Status ClientNegotiation::AuthenticateBySasl(faststring* recv_buf) {
   RETURN_NOT_OK(InitSaslClient());
-  RETURN_NOT_OK(SendSaslInitiate());
-  NegotiatePB response;
-  while (true) {
-    RETURN_NOT_OK(RecvNegotiatePB(&response, recv_buf));
-    Status s;
-    switch (response.step()) {
-      // SASL_CHALLENGE: Server sent us a follow-up to a SASL_INITIATE or SASL_RESPONSE request.
-      case NegotiatePB::SASL_CHALLENGE:
-        RETURN_NOT_OK(HandleSaslChallenge(response));
-        break;
-      // SASL_SUCCESS: Server has accepted our authentication request. Negotiation successful.
-      case NegotiatePB::SASL_SUCCESS:
-        return HandleSaslSuccess(response);
-      default:
-        return Status::NotAuthorized("expected SASL_CHALLENGE or SASL_SUCCESS step",
-                                     NegotiatePB::NegotiateStep_Name(response.step()));
-    }
+  Status s = SendSaslInitiate();
+
+  // HandleSasl[Initiate, Challenge] return incomplete if an additional
+  // challenge step is required, or OK if a SASL_SUCCESS message is expected.
+  while (s.IsIncomplete()) {
+    NegotiatePB challenge;
+    RETURN_NOT_OK(RecvNegotiatePB(&challenge, recv_buf));
+    s = HandleSaslChallenge(challenge);
   }
+
+  // Propagate failure from SendSaslInitiate or HandleSaslChallenge.
+  RETURN_NOT_OK(s);
+
+  // Server challenges are over; we now expect the success message.
+  NegotiatePB success;
+  RETURN_NOT_OK(RecvNegotiatePB(&success, recv_buf));
+  return HandleSaslSuccess(success);
 }
 
 Status ClientNegotiation::AuthenticateByToken(faststring* recv_buf) {
@@ -542,7 +541,7 @@ Status ClientNegotiation::SendSaslInitiate() {
    *  SASL_INTERACT -- user interaction needed to fill in prompt_need list
    */
   TRACE("Calling sasl_client_start()");
-  Status s = WrapSaslCall(sasl_conn_.get(), [&]() {
+  const Status s = WrapSaslCall(sasl_conn_.get(), [&]() {
       return sasl_client_start(
           sasl_conn_.get(),                         // The SASL connection context created
by init()
           SaslMechanism::name_of(negotiated_mech_), // The list of mechanisms to negotiate.
@@ -570,7 +569,8 @@ Status ClientNegotiation::SendSaslInitiate() {
   msg.set_step(NegotiatePB::SASL_INITIATE);
   msg.mutable_token()->assign(init_msg, init_msg_len);
   msg.add_sasl_mechanisms()->set_mechanism(negotiated_mech);
-  return SendNegotiatePB(msg);
+  RETURN_NOT_OK(SendNegotiatePB(msg));
+  return s;
 }
 
 Status ClientNegotiation::SendSaslResponse(const char* resp_msg, unsigned resp_msg_len) {
@@ -581,6 +581,10 @@ Status ClientNegotiation::SendSaslResponse(const char* resp_msg, unsigned
resp_m
 }
 
 Status ClientNegotiation::HandleSaslChallenge(const NegotiatePB& response) {
+  if (PREDICT_FALSE(response.step() != NegotiatePB::SASL_CHALLENGE)) {
+    return Status::NotAuthorized("expected SASL_CHALLENGE step",
+                                 NegotiatePB::NegotiateStep_Name(response.step()));
+  }
   TRACE("Received SASL_CHALLENGE response from server");
   if (PREDICT_FALSE(!response.has_token())) {
     return Status::NotAuthorized("no token in SASL_CHALLENGE response from server");
@@ -588,15 +592,20 @@ Status ClientNegotiation::HandleSaslChallenge(const NegotiatePB&
response) {
 
   const char* out = nullptr;
   unsigned out_len = 0;
-  Status s = DoSaslStep(response.token(), &out, &out_len);
+  const Status s = DoSaslStep(response.token(), &out, &out_len);
   if (PREDICT_FALSE(!s.IsIncomplete() && !s.ok())) {
     return s;
   }
 
-  return SendSaslResponse(out, out_len);
+  RETURN_NOT_OK(SendSaslResponse(out, out_len));
+  return s;
 }
 
 Status ClientNegotiation::HandleSaslSuccess(const NegotiatePB& response) {
+  if (PREDICT_FALSE(response.step() != NegotiatePB::SASL_SUCCESS)) {
+    return Status::NotAuthorized("expected SASL_SUCCESS step",
+                                 NegotiatePB::NegotiateStep_Name(response.step()));
+  }
   TRACE("Received SASL_SUCCESS response from server");
 
   if (tls_negotiated_ &&

http://git-wip-us.apache.org/repos/asf/kudu/blob/8f8dfe17/src/kudu/rpc/client_negotiation.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/client_negotiation.h b/src/kudu/rpc/client_negotiation.h
index dc92204..467b6c8 100644
--- a/src/kudu/rpc/client_negotiation.h
+++ b/src/kudu/rpc/client_negotiation.h
@@ -172,12 +172,20 @@ class ClientNegotiation {
   Status AuthenticateByToken(faststring* recv_buf) WARN_UNUSED_RESULT;
 
   // Send an SASL_INITIATE message to the server.
+  // Returns:
+  //  Status::OK if the SASL_SUCCESS message is expected next.
+  //  Status::Incomplete if the SASL_CHALLENGE message is expected next.
+  //  Any other status indicates an error.
   Status SendSaslInitiate() WARN_UNUSED_RESULT;
 
   // Send a SASL_RESPONSE message to the server.
   Status SendSaslResponse(const char* resp_msg, unsigned resp_msg_len) WARN_UNUSED_RESULT;
 
   // Handle case when server sends SASL_CHALLENGE response.
+  // Returns:
+  //  Status::OK if a SASL_SUCCESS message is expected next.
+  //  Status::Incomplete if another SASL_CHALLENGE message is expected.
+  //  Any other status indicates an error.
   Status HandleSaslChallenge(const NegotiatePB& response) WARN_UNUSED_RESULT;
 
   // Handle case when server sends SASL_SUCCESS response.


Mime
View raw message