diff options
author | Jose <pepone@users.noreply.github.com> | 2019-09-10 10:29:11 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-09-10 10:29:11 +0200 |
commit | bad1d435dfba9b103bfe76555506757beda5c4df (patch) | |
tree | f4a8dd87308ee80781f9b39ffabd95cee9e1ef69 | |
parent | Whitespace fixes (diff) | |
download | ice-bad1d435dfba9b103bfe76555506757beda5c4df.tar.bz2 ice-bad1d435dfba9b103bfe76555506757beda5c4df.tar.xz ice-bad1d435dfba9b103bfe76555506757beda5c4df.zip |
IceSSL cert name verification fixes - Close #512 (#515)
-rw-r--r-- | CHANGELOG-3.7.md | 10 | ||||
-rw-r--r-- | cpp/src/IceSSL/OpenSSLEngine.cpp | 13 | ||||
-rw-r--r-- | cpp/src/IceSSL/OpenSSLEngine.h | 1 | ||||
-rw-r--r-- | cpp/src/IceSSL/OpenSSLTransceiverI.cpp | 20 | ||||
-rw-r--r-- | cpp/src/IceSSL/SChannelEngine.cpp | 7 | ||||
-rw-r--r-- | cpp/src/IceSSL/SChannelEngine.h | 2 | ||||
-rw-r--r-- | cpp/src/IceSSL/SChannelTransceiverI.cpp | 24 | ||||
-rw-r--r-- | cpp/src/IceSSL/SSLEngine.cpp | 14 | ||||
-rw-r--r-- | cpp/test/IceSSL/configuration/AllTests.cpp | 2 | ||||
-rw-r--r-- | csharp/src/IceSSL/SSLEngine.cs | 1 | ||||
-rw-r--r-- | csharp/src/IceSSL/TransceiverI.cs | 24 | ||||
-rw-r--r-- | csharp/test/IceSSL/configuration/AllTests.cs | 29 | ||||
-rw-r--r-- | java-compat/test/src/main/java/test/IceSSL/configuration/AllTests.java | 2 | ||||
-rw-r--r-- | java/test/src/main/java/test/IceSSL/configuration/AllTests.java | 3 |
14 files changed, 109 insertions, 43 deletions
diff --git a/CHANGELOG-3.7.md b/CHANGELOG-3.7.md index 914a369a58f..551c3b998c0 100644 --- a/CHANGELOG-3.7.md +++ b/CHANGELOG-3.7.md @@ -112,6 +112,13 @@ These are the changes since Ice 3.7.2. - Add support for AIX 7.2 with the IBM XL C/C++ 16.1 compiler (C++98 only). +- Fixed a bug in IceSSL that could result in `IceSSL::ConnectionInfo` not having + the `verified` data member set to false when the certficiate hostname verification + failed. This affected IceSSL based on OpenSSL < 1.0.2 and SChannel. + +- Fixed IceSSL to ignore hostname verification errors when `IceSSL.VerifyPeer` is + set to 0. This affected IceSSL based on OpenSSL >= 1.0.2. + ## C# Changes - Added back support for caching the output stream used to marshal the response @@ -123,6 +130,9 @@ These are the changes since Ice 3.7.2. - Fixed loading of Bzip2 native libraries in Linux to fallback to libbz2.so.1 if libbz2.so.1.0 doesn't exists. +- Fixed IceSSL to ignore hostname verification errors when `IceSSL.VerifyPeer` is + set to 0. + ## Java Changes - Added back support for caching the output stream used to marshal the response 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) { diff --git a/csharp/src/IceSSL/SSLEngine.cs b/csharp/src/IceSSL/SSLEngine.cs index e008da6a245..d97addc0603 100644 --- a/csharp/src/IceSSL/SSLEngine.cs +++ b/csharp/src/IceSSL/SSLEngine.cs @@ -488,7 +488,6 @@ namespace IceSSL internal void verifyPeer(string address, IceSSL.ConnectionInfo info, string desc) { - if(_verifyDepthMax > 0 && info.certs != null && info.certs.Length > _verifyDepthMax) { string msg = (info.incoming ? "incoming" : "outgoing") + " connection rejected:\n" + diff --git a/csharp/src/IceSSL/TransceiverI.cs b/csharp/src/IceSSL/TransceiverI.cs index 90698ee5a6f..daa2bb201a2 100644 --- a/csharp/src/IceSSL/TransceiverI.cs +++ b/csharp/src/IceSSL/TransceiverI.cs @@ -589,20 +589,34 @@ namespace IceSSL } } - if((errors & (int)SslPolicyErrors.RemoteCertificateNameMismatch) > 0) + bool certificateNameMismatch = (errors & (int)SslPolicyErrors.RemoteCertificateNameMismatch) > 0; + if(certificateNameMismatch) { if(_instance.engine().getCheckCertName() && !string.IsNullOrEmpty(_host)) { if(_instance.securityTraceLevel() >= 1) { - _instance.logger().trace(_instance.securityTraceCategory(), - "SSL certificate validation failed - Hostname mismatch"); + string msg = "SSL certificate validation failed - Hostname mismatch"; + if(_verifyPeer == 0) + { + msg += " (ignored)"; + } + _instance.logger().trace(_instance.securityTraceCategory(), msg); + } + + if(_verifyPeer > 0) + { + return false; + } + else + { + errors ^= (int)SslPolicyErrors.RemoteCertificateNameMismatch; } - return false; } else { errors ^= (int)SslPolicyErrors.RemoteCertificateNameMismatch; + certificateNameMismatch = false; } } @@ -633,7 +647,7 @@ namespace IceSSL } else { - _verified = true; + _verified = !certificateNameMismatch; } } else if(status.Status == X509ChainStatusFlags.Revoked) diff --git a/csharp/test/IceSSL/configuration/AllTests.cs b/csharp/test/IceSSL/configuration/AllTests.cs index f8d66796217..8f5710df081 100644 --- a/csharp/test/IceSSL/configuration/AllTests.cs +++ b/csharp/test/IceSSL/configuration/AllTests.cs @@ -783,6 +783,35 @@ public class AllTests // // Target host does not match the certificate DNS altName, connection should succeed + // because IceSSL.VerifyPeer is set to 0. + // + { + initData = createClientProps(defaultProperties, "c_rsa_ca1", "cacert1"); + initData.properties.setProperty("IceSSL.CheckCertName", "1"); + initData.properties.setProperty("IceSSL.VerifyPeer", "0"); + comm = Ice.Util.initialize(ref args, initData); + + fact = Test.ServerFactoryPrxHelper.checkedCast(comm.stringToProxy(factoryRef)); + test(fact != null); + d = createServerProps(props, "s_rsa_ca1_cn2", "cacert1"); + server = fact.createServer(d); + try + { + server.ice_ping(); + IceSSL.ConnectionInfo info = (IceSSL.ConnectionInfo)server.ice_getConnection().getInfo(); + test(!info.verified); + } + catch(Ice.LocalException ex) + { + Console.WriteLine(ex.ToString()); + test(false); + } + fact.destroyServer(server); + comm.destroy(); + } + + // + // Target host does not match the certificate DNS altName, connection should succeed // because IceSSL.CheckCertName is set to 0. // { diff --git a/java-compat/test/src/main/java/test/IceSSL/configuration/AllTests.java b/java-compat/test/src/main/java/test/IceSSL/configuration/AllTests.java index 8de74f60b51..cfd5a301015 100644 --- a/java-compat/test/src/main/java/test/IceSSL/configuration/AllTests.java +++ b/java-compat/test/src/main/java/test/IceSSL/configuration/AllTests.java @@ -686,6 +686,8 @@ public class AllTests try { server.ice_ping(); + IceSSL.ConnectionInfo info = (IceSSL.ConnectionInfo)server.ice_getConnection().getInfo(); + test(info.verified); } catch(Ice.LocalException ex) { diff --git a/java/test/src/main/java/test/IceSSL/configuration/AllTests.java b/java/test/src/main/java/test/IceSSL/configuration/AllTests.java index bfa01f4e882..3bd6f10615a 100644 --- a/java/test/src/main/java/test/IceSSL/configuration/AllTests.java +++ b/java/test/src/main/java/test/IceSSL/configuration/AllTests.java @@ -684,6 +684,9 @@ public class AllTests try { server.ice_ping(); + com.zeroc.IceSSL.ConnectionInfo info = + (com.zeroc.IceSSL.ConnectionInfo)server.ice_getConnection().getInfo(); + test(info.verified); } catch(com.zeroc.Ice.LocalException ex) { |