IceSSL cert name verification fixes - Close #512 (#515)

8 files changed, 46 insertions, 37 deletions

diff --git a/cpp/src/IceSSL/OpenSSLEngine.cpp b/cpp/src/IceSSL/OpenSSLEngine.cpp
index 164532b7925..764369d0d45 100644
--- a/cpp/src/IceSSL/OpenSSLEngine.cpp
+++ b/cpp/src/IceSSL/OpenSSLEngine.cpp
@@ -935,19 +935,6 @@ OpenSSL::SSLEngine::destroy()
     }
 }
 
-void
-OpenSSL::SSLEngine::verifyPeer(const string& address, const IceSSL::ConnectionInfoPtr& info, const string& desc)
-{
-#if defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER < 0x10002000L
-    //
-    // Peer hostname verification is new in OpenSSL 1.0.2 for older versions
-    //  we use IceSSL build in hostname verification.
-    //
-    verifyPeerCertName(address, info);
-#endif
-    IceSSL::SSLEngine::verifyPeer(address, info, desc);
-}
-
 IceInternal::TransceiverPtr
 OpenSSL::SSLEngine::createTransceiver(const InstancePtr& instance,
                                       const IceInternal::TransceiverPtr& delegate,
diff --git a/cpp/src/IceSSL/OpenSSLEngine.h b/cpp/src/IceSSL/OpenSSLEngine.h
index 0d77fb64db1..b3c31ba53af 100644
--- a/cpp/src/IceSSL/OpenSSLEngine.h
+++ b/cpp/src/IceSSL/OpenSSLEngine.h
@@ -26,7 +26,6 @@ public:
 
     virtual void initialize();
     virtual void destroy();
-    virtual void verifyPeer(const std::string&, const IceSSL::ConnectionInfoPtr&, const std::string&);
     virtual IceInternal::TransceiverPtr
     createTransceiver(const IceSSL::InstancePtr&, const IceInternal::TransceiverPtr&, const std::string&, bool);
 
diff --git a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
index def54dff731..1acd3b12800 100644
--- a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
+++ b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
@@ -171,7 +171,7 @@ OpenSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::
             // Hostname verification was included in OpenSSL 1.0.2
             //
 #if defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x10002000L
-            if(_engine->getCheckCertName() && !_host.empty() && (sslVerifyMode & SSL_VERIFY_PEER))
+            if(_engine->getCheckCertName() && !_host.empty())
             {
                 X509_VERIFY_PARAM* param = SSL_get0_param(_ssl);
                 if(IceInternal::isIpAddress(_host))
@@ -339,6 +339,24 @@ OpenSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::
     }
 
     _cipher = SSL_get_cipher_name(_ssl); // Nothing needs to be free'd.
+#if defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER < 0x10002000L
+    try
+    {
+        //
+        // Peer hostname verification is new in OpenSSL 1.0.2 for older versions
+        // We use IceSSL built-in hostname verification.
+        //
+        _engine->verifyPeerCertName(address, info);
+    }
+    catch(const SecurityException&)
+    {
+        _verified = false;
+        if(_engine->getVerifyPeer() > 0)
+        {
+            throw;
+        }
+    }
+#endif
     _engine->verifyPeer(_host, ICE_DYNAMIC_CAST(ConnectionInfo, getInfo()), toString());
 
     if(_engine->securityTraceLevel() >= 1)
diff --git a/cpp/src/IceSSL/SChannelEngine.cpp b/cpp/src/IceSSL/SChannelEngine.cpp
index 770988c3e90..420a83966a7 100644
--- a/cpp/src/IceSSL/SChannelEngine.cpp
+++ b/cpp/src/IceSSL/SChannelEngine.cpp
@@ -1273,13 +1273,6 @@ SChannel::SSLEngine::destroy()
     }
 }
 
-void
-SChannel::SSLEngine::verifyPeer(const string& address, const IceSSL::ConnectionInfoPtr& info, const string& desc)
-{
-    verifyPeerCertName(address, info);
-    IceSSL::SSLEngine::verifyPeer(address, info, desc);
-}
-
 IceInternal::TransceiverPtr
 SChannel::SSLEngine::createTransceiver(const InstancePtr& instance,
                                        const IceInternal::TransceiverPtr& delegate,
diff --git a/cpp/src/IceSSL/SChannelEngine.h b/cpp/src/IceSSL/SChannelEngine.h
index 6d6b96d8481..c40af674e61 100644
--- a/cpp/src/IceSSL/SChannelEngine.h
+++ b/cpp/src/IceSSL/SChannelEngine.h
@@ -91,8 +91,6 @@ public:
     //
     virtual void destroy();
 
-    virtual void verifyPeer(const std::string&, const ConnectionInfoPtr&, const std::string&);
-
     std::string getCipherName(ALG_ID) const;
 
     CredHandle newCredentialsHandle(bool);
diff --git a/cpp/src/IceSSL/SChannelTransceiverI.cpp b/cpp/src/IceSSL/SChannelTransceiverI.cpp
index ec9562d4153..3d497f3816a 100644
--- a/cpp/src/IceSSL/SChannelTransceiverI.cpp
+++ b/cpp/src/IceSSL/SChannelTransceiverI.cpp
@@ -746,7 +746,20 @@ SChannel::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal:
         throw SecurityException(__FILE__, __LINE__, "IceSSL: error reading cipher info:\n" + secStatusToString(err));
     }
 
-    _engine->verifyPeer(_host, ICE_DYNAMIC_CAST(ConnectionInfo, getInfo()), toString());
+    ConnectionInfoPtr info = ICE_DYNAMIC_CAST(ConnectionInfo, getInfo());
+    try
+    {
+        _engine->verifyPeerCertName(_host, info);
+    }
+    catch(const Ice::SecurityException&)
+    {
+        _verified = false;
+        if(_engine->getVerifyPeer() > 0)
+        {
+            throw;
+        }
+    }
+    _engine->verifyPeer(_host, info, toString());
     _state = StateHandshakeComplete;
 
     if(_instance->engine()->securityTraceLevel() >= 1)
@@ -754,12 +767,11 @@ SChannel::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal:
         string sslCipherName;
         string sslKeyExchangeAlgorithm;
         string sslProtocolName;
-        SecPkgContext_ConnectionInfo info;
-        if(QueryContextAttributes(&_ssl, SECPKG_ATTR_CONNECTION_INFO, &info) == SEC_E_OK)
+        if(QueryContextAttributes(&_ssl, SECPKG_ATTR_CONNECTION_INFO, &connInfo) == SEC_E_OK)
         {
-            sslCipherName = _engine->getCipherName(info.aiCipher);
-            sslKeyExchangeAlgorithm = _engine->getCipherName(info.aiExch);
-            sslProtocolName = protocolName(info.dwProtocol);
+            sslCipherName = _engine->getCipherName(connInfo.aiCipher);
+            sslKeyExchangeAlgorithm = _engine->getCipherName(connInfo.aiExch);
+            sslProtocolName = protocolName(connInfo.dwProtocol);
         }
 
         Trace out(_instance->logger(), _instance->traceCategory());
diff --git a/cpp/src/IceSSL/SSLEngine.cpp b/cpp/src/IceSSL/SSLEngine.cpp
index 1e2b4f15ec3..d989a784c90 100644
--- a/cpp/src/IceSSL/SSLEngine.cpp
+++ b/cpp/src/IceSSL/SSLEngine.cpp
@@ -207,19 +207,19 @@ IceSSL::SSLEngine::verifyPeerCertName(const string& address, const ConnectionInf
         if(!certNameOK)
         {
             ostringstream ostr;
-            ostr << "IceSSL: certificate validation failure: "
-                 << (isIpAddress ? "IP address mismatch" : "Hostname mismatch");
+            ostr << "IceSSL: ";
+            if(_verifyPeer > 0)
+            {
+                ostr << "ignoring ";
+            }
+            ostr << "certificate verification failure " << (isIpAddress ? "IP address mismatch" : "Hostname mismatch");
             string msg = ostr.str();
             if(_securityTraceLevel >= 1)
             {
                 Trace out(_logger, _securityTraceCategory);
                 out << msg;
             }
-
-            if(_verifyPeer > 0)
-            {
-                throw SecurityException(__FILE__, __LINE__, msg);
-            }
+            throw SecurityException(__FILE__, __LINE__, msg);
         }
     }
 }
diff --git a/cpp/test/IceSSL/configuration/AllTests.cpp b/cpp/test/IceSSL/configuration/AllTests.cpp
index caae5f854a2..9a7b6dbd88a 100644
--- a/cpp/test/IceSSL/configuration/AllTests.cpp
+++ b/cpp/test/IceSSL/configuration/AllTests.cpp
@@ -1467,6 +1467,8 @@ allTests(Test::TestHelper* helper, const string& /*testDir*/, bool p12)
             try
             {
                 server->ice_ping();
+                info = ICE_DYNAMIC_CAST(IceSSL::ConnectionInfo, server->ice_getCachedConnection()->getInfo());
+                test(!info->verified);
             }
             catch(const Ice::LocalException& ex)
             {