summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJose <pepone@users.noreply.github.com>2019-09-10 10:29:11 +0200
committerGitHub <noreply@github.com>2019-09-10 10:29:11 +0200
commitbad1d435dfba9b103bfe76555506757beda5c4df (patch)
treef4a8dd87308ee80781f9b39ffabd95cee9e1ef69
parentWhitespace fixes (diff)
downloadice-bad1d435dfba9b103bfe76555506757beda5c4df.tar.bz2
ice-bad1d435dfba9b103bfe76555506757beda5c4df.tar.xz
ice-bad1d435dfba9b103bfe76555506757beda5c4df.zip
IceSSL cert name verification fixes - Close #512 (#515)
-rw-r--r--CHANGELOG-3.7.md10
-rw-r--r--cpp/src/IceSSL/OpenSSLEngine.cpp13
-rw-r--r--cpp/src/IceSSL/OpenSSLEngine.h1
-rw-r--r--cpp/src/IceSSL/OpenSSLTransceiverI.cpp20
-rw-r--r--cpp/src/IceSSL/SChannelEngine.cpp7
-rw-r--r--cpp/src/IceSSL/SChannelEngine.h2
-rw-r--r--cpp/src/IceSSL/SChannelTransceiverI.cpp24
-rw-r--r--cpp/src/IceSSL/SSLEngine.cpp14
-rw-r--r--cpp/test/IceSSL/configuration/AllTests.cpp2
-rw-r--r--csharp/src/IceSSL/SSLEngine.cs1
-rw-r--r--csharp/src/IceSSL/TransceiverI.cs24
-rw-r--r--csharp/test/IceSSL/configuration/AllTests.cs29
-rw-r--r--java-compat/test/src/main/java/test/IceSSL/configuration/AllTests.java2
-rw-r--r--java/test/src/main/java/test/IceSSL/configuration/AllTests.java3
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)
{