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

5 files changed, 34 insertions, 3 deletions

diff --git a/contrib/native/client/src/clientlib/drillClientImpl.cpp b/contrib/native/client/src/clientlib/drillClientImpl.cpp
index f0bb636b3..9fdd72547 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 {
diff --git a/contrib/native/client/src/clientlib/drillClientImpl.hpp b/contrib/native/client/src/clientlib/drillClientImpl.hpp
index dc4a67eaa..2e7623e97 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;
diff --git a/contrib/native/client/src/clientlib/errmsgs.cpp b/contrib/native/client/src/clientlib/errmsgs.cpp
index d2d8c1806..a5d4a4428 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."},
diff --git a/contrib/native/client/src/clientlib/errmsgs.hpp b/contrib/native/client/src/clientlib/errmsgs.hpp
index 246e4bbf2..c4a008026 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
diff --git a/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp b/contrib/native/client/src/clientlib/saslAuthenticatorImpl.cpp
index 78f99b619..9057a372f 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<std::string>& 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