Fix (6462) - Consider changing some IceSSL checks to use native APIs

6 files changed, 88 insertions, 71 deletions

diff --git a/cpp/src/Ice/Network.cpp b/cpp/src/Ice/Network.cpp
index c64d9fa7cb2..05abc9528f9 100755
--- a/cpp/src/Ice/Network.cpp
+++ b/cpp/src/Ice/Network.cpp
@@ -3056,3 +3056,19 @@ IceInternal::doFinishConnectAsync(SOCKET fd, AsyncInfo& info)
     }
 }
 #endif
+
+
+bool
+IceInternal::isIpAddress(const string& name)
+{
+#ifdef ICE_OS_UWP
+     HostName^ hostname = ref new HostName(ref new String(stringToWstring(name,
+                                                          getProcessStringConverter()).c_str()));
+     return hostname->Type == HostNameType::Ipv4 || hostname->Type == HostNameType::Ipv6;
+#else
+    in_addr addr;
+    in6_addr addr6;
+    
+    return inet_pton(AF_INET, name.c_str(), &addr) > 0 || inet_pton(AF_INET6, name.c_str(), &addr6) > 0;
+#endif
+}
\ No newline at end of file
diff --git a/cpp/src/Ice/Network.h b/cpp/src/Ice/Network.h
index 9a5099a9eb3..df07941f73d 100755
--- a/cpp/src/Ice/Network.h
+++ b/cpp/src/Ice/Network.h
@@ -368,6 +368,9 @@ ICE_API void runSync(Windows::Foundation::IAsyncAction^ action);
 ICE_API void doConnectAsync(SOCKET, const Address&, const Address&, AsyncInfo&);
 ICE_API void doFinishConnectAsync(SOCKET, AsyncInfo&);
 #endif
+
+ICE_API bool isIpAddress(const std::string&);
+
 }
 
 #endif
diff --git a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
index e2a48cf7eea..0f8359f3638 100644
--- a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
+++ b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
@@ -20,6 +20,7 @@
 #include <Ice/LoggerUtil.h>
 #include <Ice/Buffer.h>
 #include <Ice/LocalException.h>
+#include <Ice/Network.h>
 
 #ifdef ICE_USE_OPENSSL
 
@@ -97,9 +98,6 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
 
     if(!_ssl)
     {
-        //
-        // This static_cast is necessary due to 64bit windows. There SOCKET is a non-int type.
-        //
         SOCKET fd = _delegate->getNativeInfo()->fd();
         if(fd == INVALID_SOCKET)
         {
@@ -154,6 +152,25 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
                     assert(false);
                 }
             }
+
+            //
+            // 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))
+            {
+                X509_VERIFY_PARAM* param = SSL_get0_param(_ssl);
+                if(IceInternal::isIpAddress(_host))
+                {
+                    X509_VERIFY_PARAM_set1_ip_asc(param, _host.c_str());
+                }
+                else
+                {
+                    X509_VERIFY_PARAM_set1_host(param, _host.c_str(), 0);
+                }
+            }
+#endif
+            
             SSL_set_verify(_ssl, sslVerifyMode, IceSSL_opensslVerifyCallback);
         }
     }
diff --git a/cpp/src/IceSSL/SSLEngine.cpp b/cpp/src/IceSSL/SSLEngine.cpp
index 2bc0b574627..c8342073d35 100644
--- a/cpp/src/IceSSL/SSLEngine.cpp
+++ b/cpp/src/IceSSL/SSLEngine.cpp
@@ -132,12 +132,14 @@ IceSSL::SSLEngine::verifyPeer(const string& address, const NativeConnectionInfoP
 {
     const CertificateVerifierPtr verifier = getCertificateVerifier();
 
-#if !defined(ICE_USE_SECURE_TRANSPORT_IOS)
+#if defined(ICE_USE_SCHANNEL) || \
+    (defined(ICE_USE_OPENSSL) && defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER < 0x10002000L)
+
     //
     // For an outgoing connection, we compare the proxy address (if any) against
     // fields in the server's certificate (if any).
     //
-    if(!info->nativeCerts.empty() && !address.empty())
+    if(_checkCertName && !info->nativeCerts.empty() && !address.empty())
     {
         const CertificatePtr cert = info->nativeCerts[0];
         //
@@ -159,82 +161,56 @@ IceSSL::SSLEngine::verifyPeer(const string& address, const NativeConnectionInfoP
             }
         }
 
-        //
-        // Compare the peer's address against the common name.
-        //
         bool certNameOK = false;
         string dn;
+        bool isIpAddress = IceInternal::isIpAddress(address);
         string addrLower = IceUtilInternal::toLower(address);
-        {
-            DistinguishedName d = cert->getSubjectDN();
-            dn = IceUtilInternal::toLower(string(d));
-            string cn = "cn=" + addrLower;
-            string::size_type pos = dn.find(cn);
-            if(pos != string::npos)
-            {
-                //
-                // Ensure we match the entire common name.
-                //
-                certNameOK = (pos + cn.size() == dn.size()) || (dn[pos + cn.size()] == ',');
-            }
-        }
-
         //
-        // Compare the peer's address against the dnsName and ipAddress
-        // values in the subject alternative name.
+        // If address is and IP address compare it to the subject alt name IP adddress
         //
-        if(!certNameOK)
+        if(isIpAddress)
         {
             certNameOK = find(ipAddresses.begin(), ipAddresses.end(), addrLower) != ipAddresses.end();
         }
-        if(!certNameOK)
-        {
-            certNameOK = find(dnsNames.begin(), dnsNames.end(), addrLower) != dnsNames.end();
-        }
-
-        //
-        // Log a message if the name comparison fails. If CheckCertName is defined,
-        // we also raise an exception to abort the connection. Don't log a message if
-        // CheckCertName is not defined and a verifier is present.
-        //
-        if(!certNameOK && (_checkCertName || (_securityTraceLevel >= 1 && !verifier)))
+        else
         {
-            ostringstream ostr;
-            ostr << "IceSSL: ";
-            if(!_checkCertName)
+            //
+            // If subjectAlt is empty compare it ot the subject CN, othewise
+            // compare it to the to the subject alt name dnsNames
+            //
+            if(dnsNames.empty())
             {
-                ostr << "ignoring ";
+                DistinguishedName d = cert->getSubjectDN();
+                dn = IceUtilInternal::toLower(string(d));
+                string cn = "cn=" + addrLower;
+                string::size_type pos = dn.find(cn);
+                if(pos != string::npos)
+                {
+                    //
+                    // Ensure we match the entire common name.
+                    //
+                    certNameOK = (pos + cn.size() == dn.size()) || (dn[pos + cn.size()] == ',');
+                }
             }
-            ostr << "certificate validation failure:\npeer certificate does not have `" << address
-                    << "' as its commonName or in its subjectAltName extension";
-            if(!dn.empty())
+            else
             {
-                ostr << "\nSubject DN: " << dn;
+                certNameOK = find(dnsNames.begin(), dnsNames.end(), addrLower) != dnsNames.end();                
             }
-            if(!dnsNames.empty())
+        }
+
+        if(!certNameOK)
+        {
+            ostringstream ostr;
+            ostr << "IceSSL: certificate validation failure: ";
+            if(isIpAddress)
             {
-                ostr << "\nDNS names found in certificate: ";
-                for(vector<string>::const_iterator p = dnsNames.begin(); p != dnsNames.end(); ++p)
-                {
-                    if(p != dnsNames.begin())
-                    {
-                        ostr << ", ";
-                    }
-                    ostr << *p;
-                }
+                ostr << "IP address mismatch";
             }
-            if(!ipAddresses.empty())
+            else
             {
-                ostr << "\nIP addresses found in certificate: ";
-                for(vector<string>::const_iterator p = ipAddresses.begin(); p != ipAddresses.end(); ++p)
-                {
-                    if(p != ipAddresses.begin())
-                    {
-                        ostr << ", ";
-                    }
-                    ostr << *p;
-                }
+                ostr << "Hostname mismatch";
             }
+
             string msg = ostr.str();
             if(_securityTraceLevel >= 1)
             {
diff --git a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
index 5de85f884c2..123d5a09a22 100644
--- a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
+++ b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
@@ -117,12 +117,11 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
             throw SecurityException(__FILE__, __LINE__, "IceSSL: handshake failure:\n" + errorToString(err));
         }
 
-#if defined(ICE_USE_SECURE_TRANSPORT_IOS)
+        //
+        // Add SSL trust policy if we need to check the certificate name.
+        //
         if(engine->getCheckCertName() && !host.empty())
         {
-            //
-            // Add SSL trust policy if we need to check the certificate name.
-            //
             UniqueRef<SecPolicyRef> policy(SecPolicyCreateSSL(false, toCFString(host)));
             UniqueRef<CFArrayRef> policies;
             if((err = SecTrustCopyPolicies(trust, &policies.get())))
@@ -136,7 +135,6 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
                 throw SecurityException(__FILE__, __LINE__, "IceSSL: handshake failure:\n" + errorToString(err));
             }
         }
-#endif
 
         //
         // Evaluate the trust
diff --git a/cpp/src/IceSSL/UWPTransceiverI.cpp b/cpp/src/IceSSL/UWPTransceiverI.cpp
index b58a688733f..91945bd7fce 100644
--- a/cpp/src/IceSSL/UWPTransceiverI.cpp
+++ b/cpp/src/IceSSL/UWPTransceiverI.cpp
@@ -167,7 +167,7 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
                 // Ignore InvalidName errors here SSLEngine::verifyPeer already checks that
                 // using IceSSL.CheckCertName settings.
                 //
-                if(result != ChainValidationResult::InvalidName && result != ChainValidationResult::Success)
+                if(result != ChainValidationResult::Success)
                 {
                     if(_engine->getVerifyPeer() == 0)
                     {
@@ -259,7 +259,10 @@ IceSSL::TransceiverI::startWrite(IceInternal::Buffer& buf)
         //
         stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::Expired);
         stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::IncompleteChain);
-        stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::InvalidName);
+        if(!_engine->getCheckCertName())
+        {
+            stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::InvalidName);
+        }
         stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::RevocationFailure);
         stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::RevocationInformationMissing);
         stream->Control->IgnorableServerCertificateErrors->Append(ChainValidationResult::Untrusted);
@@ -292,6 +295,10 @@ IceSSL::TransceiverI::finishWrite(IceInternal::Buffer& buf)
         IceInternal::AsyncInfo* asyncInfo = getNativeInfo()->getAsyncInfo(IceInternal::SocketOperationWrite);
         if(asyncInfo->count == SOCKET_ERROR)
         {
+            if(CERT_E_CN_NO_MATCH == asyncInfo->error)
+            {
+                throw SecurityException(__FILE__, __LINE__, "Hostname mismatch");
+            }
             IceInternal::checkErrorCode(__FILE__, __LINE__, asyncInfo->error);
         }
         return;