Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 00261200D34 for ; Fri, 20 Oct 2017 02:19:38 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id F2EF61609EE; Fri, 20 Oct 2017 00:19:37 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 21468160BEC for ; Fri, 20 Oct 2017 02:19:36 +0200 (CEST) Received: (qmail 13141 invoked by uid 500); 20 Oct 2017 00:19:36 -0000 Mailing-List: contact commits-help@drill.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: commits@drill.apache.org Delivered-To: mailing list commits@drill.apache.org Received: (qmail 13132 invoked by uid 99); 20 Oct 2017 00:19:36 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 20 Oct 2017 00:19:36 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 6F604DFB0E; Fri, 20 Oct 2017 00:19:35 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: parthc@apache.org To: commits@drill.apache.org Date: Fri, 20 Oct 2017 00:19:36 -0000 Message-Id: <86bde5ed3a1046288d3cab5fa253e2b0@git.apache.org> In-Reply-To: <8e43b940d3884067bcc1dd6aeb6110b3@git.apache.org> References: <8e43b940d3884067bcc1dd6aeb6110b3@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [2/2] drill git commit: DRILL-5582: C++ Client: [Threat Modeling] Drillbit may be spoofed by an attacker and this may lead to data being written to the attacker's target instead of Drillbit archived-at: Fri, 20 Oct 2017 00:19:38 -0000 DRILL-5582: C++ Client: [Threat Modeling] Drillbit may be spoofed by an attacker and this may lead to data being written to the attacker's target instead of Drillbit This closes #997 Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/f1d1945b Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/f1d1945b Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/f1d1945b Branch: refs/heads/master Commit: f1d1945b3772bb782039fd6811e34a7de66441c8 Parents: 40d0991 Author: karthik Authored: Tue Oct 17 16:18:45 2017 -0700 Committer: Parth Chandra Committed: Thu Oct 19 17:13:05 2017 -0700 ---------------------------------------------------------------------- .../client/src/clientlib/drillClientImpl.cpp | 22 ++++++++++++++++++-- .../client/src/clientlib/drillClientImpl.hpp | 3 +++ contrib/native/client/src/clientlib/errmsgs.cpp | 6 ++++++ contrib/native/client/src/clientlib/errmsgs.hpp | 4 +++- .../src/clientlib/saslAuthenticatorImpl.cpp | 2 ++ 5 files changed, 34 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/f1d1945b/contrib/native/client/src/clientlib/drillClientImpl.cpp ---------------------------------------------------------------------- diff --git a/contrib/native/client/src/clientlib/drillClientImpl.cpp b/contrib/native/client/src/clientlib/drillClientImpl.cpp index f0bb636..9fdd725 100644 --- a/contrib/native/client/src/clientlib/drillClientImpl.cpp +++ b/contrib/native/client/src/clientlib/drillClientImpl.cpp @@ -518,6 +518,21 @@ bool DrillClientImpl::clientNeedsEncryption(const DrillUserProperties* userPrope return needsEncryption; } +/* + * Checks if the client has explicitly expressed interest in authenticated connections only. + * If the USERPROP_PASSWORD or USERPROP_AUTH_MECHANISM connection string properties are + * non-empty, then it is implied that the client wants authentication. + */ +bool DrillClientImpl::clientNeedsAuthentication(const DrillUserProperties* userProperties) { + bool needsAuthentication = false; + if(!userProperties) { return false; } + std::string passwd = ""; + userProperties->getProp(USERPROP_PASSWORD, passwd); + std::string authMech = ""; + userProperties->getProp(USERPROP_AUTH_MECHANISM, authMech); + return !passwd.empty() || !authMech.empty(); +} + connectionStatus_t DrillClientImpl::validateHandshake(DrillUserProperties* properties){ DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "validateHandShake\n";) @@ -595,6 +610,10 @@ connectionStatus_t DrillClientImpl::validateHandshake(DrillUserProperties* prope switch(this->m_handshakeStatus) { case exec::user::SUCCESS: + // Check if client needs auth/encryption and server is not requiring it + if(clientNeedsAuthentication(properties) || clientNeedsEncryption(properties)) { + return handleConnError(CONN_AUTH_FAILED, getMessage(ERR_CONN_NOSERVERAUTH)); + } // reset io_service after handshake is validated before running queries m_io_service.reset(); return CONN_SUCCESS; @@ -630,8 +649,7 @@ connectionStatus_t DrillClientImpl::handleAuthentication(const DrillUserProperti // Check if client needs encryption and server is configured for encryption or not before starting handshake if(clientNeedsEncryption(userProperties) && !m_encryptionCtxt.isEncryptionReqd()) { - return handleConnError(CONN_AUTH_FAILED, "Client needs encryption but on server side encryption is disabled." - " Please check connection parameters or contact administrator?"); + return handleConnError(CONN_AUTH_FAILED, getMessage(ERR_CONN_NOSERVERENC)); } try { http://git-wip-us.apache.org/repos/asf/drill/blob/f1d1945b/contrib/native/client/src/clientlib/drillClientImpl.hpp ---------------------------------------------------------------------- diff --git a/contrib/native/client/src/clientlib/drillClientImpl.hpp b/contrib/native/client/src/clientlib/drillClientImpl.hpp index dc4a67e..2e7623e 100644 --- a/contrib/native/client/src/clientlib/drillClientImpl.hpp +++ b/contrib/native/client/src/clientlib/drillClientImpl.hpp @@ -474,6 +474,9 @@ class DrillClientImpl : public DrillClientImplBase{ void freeMetadata(meta::DrillMetadata* metadata); + static bool clientNeedsAuthentication(const DrillUserProperties* userProperties); + + private: friend class meta::DrillMetadata; friend class DrillClientQueryHandle; http://git-wip-us.apache.org/repos/asf/drill/blob/f1d1945b/contrib/native/client/src/clientlib/errmsgs.cpp ---------------------------------------------------------------------- diff --git a/contrib/native/client/src/clientlib/errmsgs.cpp b/contrib/native/client/src/clientlib/errmsgs.cpp index d2d8c18..a5d4a44 100644 --- a/contrib/native/client/src/clientlib/errmsgs.cpp +++ b/contrib/native/client/src/clientlib/errmsgs.cpp @@ -52,6 +52,12 @@ static Drill::ErrorMessages errorMessages[]={ {ERR_CONN_NOCONNSTR, ERR_CATEGORY_CONN, 0, "Cannot connect if either host name or port number are empty."}, {ERR_CONN_SSLCERTFAIL, ERR_CATEGORY_CONN, 0, "SSL certificate file %s could not be loaded (exception message: %s)."}, {ERR_CONN_NOSOCKET, ERR_CATEGORY_CONN, 0, "Failed to open socket connection."}, + {ERR_CONN_NOSERVERAUTH, ERR_CATEGORY_CONN, 0, "Client needs a secure connection but server does not" + " support any security mechanisms. Please contact an administrator. [Warn: This" + " could be due to a bad configuration or a security attack is in progress.]"}, + {ERR_CONN_NOSERVERENC, ERR_CATEGORY_CONN, 0, "Client needs encryption but encryption is disabled on the server." + " Please check connection parameters or contact administrator. [Warn: This" + " could be due to a bad configuration or a security attack is in progress.]"}, {ERR_QRY_OUTOFMEM, ERR_CATEGORY_QRY, 0, "Out of memory."}, {ERR_QRY_COMMERR, ERR_CATEGORY_QRY, 0, "Communication error. %s"}, {ERR_QRY_INVREADLEN, ERR_CATEGORY_QRY, 0, "Internal Error: Received a message with an invalid read length."}, http://git-wip-us.apache.org/repos/asf/drill/blob/f1d1945b/contrib/native/client/src/clientlib/errmsgs.hpp ---------------------------------------------------------------------- diff --git a/contrib/native/client/src/clientlib/errmsgs.hpp b/contrib/native/client/src/clientlib/errmsgs.hpp index 246e4bb..c4a0080 100644 --- a/contrib/native/client/src/clientlib/errmsgs.hpp +++ b/contrib/native/client/src/clientlib/errmsgs.hpp @@ -54,7 +54,9 @@ namespace Drill{ #define ERR_CONN_NOCONNSTR DRILL_ERR_START+21 #define ERR_CONN_SSLCERTFAIL DRILL_ERR_START+22 #define ERR_CONN_NOSOCKET DRILL_ERR_START+23 -#define ERR_CONN_MAX DRILL_ERR_START+23 +#define ERR_CONN_NOSERVERAUTH DRILL_ERR_START+24 +#define ERR_CONN_NOSERVERENC DRILL_ERR_START+25 +#define ERR_CONN_MAX DRILL_ERR_START+25 #define ERR_QRY_OUTOFMEM ERR_CONN_MAX+1 #define ERR_QRY_COMMERR ERR_CONN_MAX+2 http://git-wip-us.apache.org/repos/asf/drill/blob/f1d1945b/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp ---------------------------------------------------------------------- diff --git a/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp b/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp index 78f99b6..9057a37 100644 --- a/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp +++ b/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp @@ -145,6 +145,8 @@ int SaslAuthenticatorImpl::init(const std::vector& mechanisms, exec authMechanismToUse = value; } } + // clientNeedsAuthentication() cannot be false if the code above picks an authMechanism + assert (authMechanismToUse.empty() || DrillClientImpl::clientNeedsAuthentication(m_pUserProperties)); if (authMechanismToUse.empty()) return SASL_NOMECH; // check if requested mechanism is supported by server