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 <[email protected]> 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 <[email protected]> Authored: Fri Feb 24 14:38:31 2017 -0800 Committer: Dan Burkert <[email protected]> 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.
