Fixed ICE-6438, ICE-6439 and various inconsistencies between IceSSL implementations

7 files changed, 119 insertions, 98 deletions

diff --git a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
index fa60bdaf5f7..bed46b3b8b4 100644
--- a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
+++ b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
@@ -258,8 +258,7 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
             if(_engine->securityTraceLevel() >= 1)
             {
                 ostringstream ostr;
-                ostr << "IceSSL: ignoring certificate verification failure:\n"
-                     << X509_verify_cert_error_string(result);
+                ostr << "IceSSL: ignoring certificate verification failure:\n" << X509_verify_cert_error_string(result);
                 _instance->logger()->trace(_instance->traceCategory(), ostr.str());
             }
         }
@@ -277,6 +276,10 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
             throw ex;
         }
     }
+    else if(_info)
+    {
+        _info->verified = true;
+    }
     _engine->verifyPeer(_stream->fd(), _host, getNativeConnectionInfo());
 
     if(_engine->securityTraceLevel() >= 1)
@@ -667,12 +670,18 @@ IceSSL::TransceiverI::initNativeConnectionInfo(X509_STORE_CTX* ctx) const
     }
     info->adapterName = _adapterName;
     info->incoming = _incoming;
+    info->verified = false;
 
     STACK_OF(X509)* chain = 0; 
     if(ctx)
     {
+        //
+        // This is called from the verify callback where OpenSSL provides the verified 
+        // certificate chain.
+        //
         chain = X509_STORE_CTX_get1_chain(ctx);
     }
+
     if(chain == 0 && _ssl != 0)
     {
         //
@@ -685,37 +694,37 @@ IceSSL::TransceiverI::initNativeConnectionInfo(X509_STORE_CTX* ctx) const
         //
         X509* cert = SSL_get_peer_certificate(_ssl);
         chain = SSL_get_peer_cert_chain(_ssl);
-	if(cert != 0 && (chain == 0 || sk_X509_num(chain) == 0 || cert != sk_X509_value(chain, 0)))
-	{
-	    CertificatePtr certificate = new Certificate(cert);
+        if(cert != 0 && (chain == 0 || sk_X509_num(chain) == 0 || cert != sk_X509_value(chain, 0)))
+        {
+            CertificatePtr certificate = new Certificate(cert);
             info->nativeCerts.push_back(certificate);
             info->certs.push_back(certificate->encode());
-	}
-	else
-	{
-	    X509_free(cert);
-	}
+        }
+        else
+        {
+            X509_free(cert);
+        }
     }
 
     if(chain != 0)
     {
-	for(int i = 0; i < sk_X509_num(chain); ++i)
-	{
-	    //
-	    // Duplicate the certificate since the stack comes straight from the SSL connection.
-	    //
-	    CertificatePtr certificate = new Certificate(X509_dup(sk_X509_value(chain, i)));
-	    info->nativeCerts.push_back(certificate);
-	    info->certs.push_back(certificate->encode());
-	}
-	if(ctx)
-	{
-	    sk_X509_pop_free(chain, X509_free);
-	}
+        for(int i = 0; i < sk_X509_num(chain); ++i)
+        {
+            //
+            // Duplicate the certificate since the stack comes straight from the SSL connection.
+            //
+            CertificatePtr certificate = new Certificate(X509_dup(sk_X509_value(chain, i)));
+            info->nativeCerts.push_back(certificate);
+            info->certs.push_back(certificate->encode());
+        }
+        if(ctx)
+        {
+            sk_X509_pop_free(chain, X509_free);
+        }
     }
     if(_ssl != 0)
     {
-	info->cipher = SSL_get_cipher_name(_ssl); // Nothing needs to be free'd.
+        info->cipher = SSL_get_cipher_name(_ssl); // Nothing needs to be free'd.
     }
     info->adapterName = _adapterName;
     info->incoming = _incoming;
diff --git a/cpp/src/IceSSL/SChannelEngine.cpp b/cpp/src/IceSSL/SChannelEngine.cpp
index 009714706f1..efb3b099ce3 100644
--- a/cpp/src/IceSSL/SChannelEngine.cpp
+++ b/cpp/src/IceSSL/SChannelEngine.cpp
@@ -705,10 +705,6 @@ SChannelEngine::newCredentialsHandle(bool incoming)
         // the root certificate either way.
         //
         cred.dwFlags = SCH_CRED_NO_SYSTEM_MAPPER;
-        if(_rootStore)
-        {
-            cred.hRootStore = _rootStore;
-        }
     }
     else
     {
diff --git a/cpp/src/IceSSL/SChannelTransceiverI.cpp b/cpp/src/IceSSL/SChannelTransceiverI.cpp
index 8f79d63df1d..74d9d840ed2 100644
--- a/cpp/src/IceSSL/SChannelTransceiverI.cpp
+++ b/cpp/src/IceSSL/SChannelTransceiverI.cpp
@@ -636,81 +636,85 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
         return op;
     }
 
-    if(!_incoming || _engine->getVerifyPeer() > 0)
+    //
+    // Build the peer certificate chain and verify it.
+    //
+    PCCERT_CONTEXT cert = 0;
+    SECURITY_STATUS err = QueryContextAttributes(&_ssl, SECPKG_ATTR_REMOTE_CERT_CONTEXT, &cert);
+    if(err && err != SEC_E_NO_CREDENTIALS)
+    {
+        throw SecurityException(__FILE__, __LINE__, "IceSSL: certificate verification failure:" +
+                                IceUtilInternal::lastErrorToString());
+    }
+
+    if(!cert && ((!_incoming && _engine->getVerifyPeer() > 0) || (_incoming && _engine->getVerifyPeer() == 2)))
     {
         //
-        // Build the peer certificate chain and verify it.
+        // Clients require server certificate if VerifyPeer > 0 and servers require client
+        // certificate if VerifyPeer == 2
         //
-        PCCERT_CONTEXT cert = 0;
-        SECURITY_STATUS err = QueryContextAttributes(&_ssl, SECPKG_ATTR_REMOTE_CERT_CONTEXT, &cert);
-        if(err && err != SEC_E_NO_CREDENTIALS)
+        throw SecurityException(__FILE__, __LINE__, "IceSSL: certificate required:" +
+                                IceUtilInternal::lastErrorToString());
+    }
+    else if(cert) // Verify the remote certificate
+    {
+        try
         {
-            throw SecurityException(__FILE__, __LINE__, "IceSSL: certificate verification failure:" +
-                                    IceUtilInternal::lastErrorToString());
-        }
+            CERT_CHAIN_PARA chainP;
+            memset(&chainP, 0, sizeof(chainP));
+            chainP.cbSize = sizeof(chainP);
 
-        if(!cert && (!_incoming || _engine->getVerifyPeer() == 2))
-        {
-            // Clients require server certificate if VerifyPeer > 0
-            // and servers require client certificate if VerifyPeer == 2
-            throw SecurityException(__FILE__, __LINE__, "IceSSL: certificate required:" +
-                                    IceUtilInternal::lastErrorToString());
-        }
-        else if(cert) // Verify the remote certificate
-        {
-            try
+            PCCERT_CHAIN_CONTEXT certChain;
+            if(!CertGetCertificateChain(_engine->chainEngine(), cert, 0, 0, &chainP,
+                                        CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, 0, &certChain))
             {
-                CERT_CHAIN_PARA chainP;
-                memset(&chainP, 0, sizeof(chainP));
-                chainP.cbSize = sizeof(chainP);
-
-                PCCERT_CHAIN_CONTEXT certChain;
-                if(!CertGetCertificateChain(_engine->chainEngine(), cert, 0, 0, &chainP,
-                                            CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, 0, &certChain))
-                {
-                    CertFreeCertificateContext(cert);
-                    throw IceUtilInternal::lastErrorToString();
-                }
+                CertFreeCertificateContext(cert);
+                throw IceUtilInternal::lastErrorToString();
+            }
 
-                CERT_SIMPLE_CHAIN* simpleChain = certChain->rgpChain[0];
+            CERT_SIMPLE_CHAIN* simpleChain = certChain->rgpChain[0];
 
-                string trustError;
-                if(simpleChain->TrustStatus.dwErrorStatus != CERT_TRUST_NO_ERROR)
-                {
-                    trustError = trustStatusToString(certChain->TrustStatus.dwErrorStatus);
-                }
+            string trustError;
+            if(simpleChain->TrustStatus.dwErrorStatus != CERT_TRUST_NO_ERROR)
+            {
+                trustError = trustStatusToString(certChain->TrustStatus.dwErrorStatus);
+            }
+            else
+            {
+                _verified = true;
+            }
 
-                CertFreeCertificateChain(certChain);
-                CertFreeCertificateContext(cert);
-                if(!trustError.empty())
-                {
-                    throw trustError;
-                }
+            CertFreeCertificateChain(certChain);
+            CertFreeCertificateContext(cert);
+            if(!trustError.empty())
+            {
+                throw trustError;
             }
-            catch(const string& reason)
+        }
+        catch(const string& reason)
+        {
+            if(_engine->getVerifyPeer() == 0)
             {
-                if(_engine->getVerifyPeer() == 0)
+                if(_instance->traceLevel() >= 1)
                 {
-                    if(_instance->traceLevel() >= 1)
-                    {
-                        _instance->logger()->trace(_instance->traceCategory(),
-                                                   "IceSSL: ignoring certificate verification failure\n" + reason);
-                    }
+                    _instance->logger()->trace(_instance->traceCategory(),
+                                               "IceSSL: ignoring certificate verification failure\n" + reason);
                 }
-                else
+            }
+            else
+            {
+                ostringstream os;
+                os << "IceSSL: certificate verification failure\n" << reason;
+                string msg = os.str();
+                if(_instance->traceLevel() >= 1)
                 {
-                    ostringstream os;
-                    os << "IceSSL: certificate verification failure\n" << reason;
-                    string msg = os.str();
-                    if(_instance->traceLevel() >= 1)
-                    {
-                        _instance->logger()->trace(_instance->traceCategory(), msg);
-                    }
-                    throw SecurityException(__FILE__, __LINE__, msg);
+                    _instance->logger()->trace(_instance->traceCategory(), msg);
                 }
+                throw SecurityException(__FILE__, __LINE__, msg);
             }
         }
     }
+
     _engine->verifyPeer(_stream->fd(), _host, getNativeConnectionInfo());
     _state = StateHandshakeComplete;
 
@@ -971,7 +975,8 @@ IceSSL::TransceiverI::TransceiverI(const InstancePtr& instance,
     _state(StateHandshakeNotStarted),
     _bufferedW(0),
     _sslInitialized(false),
-    _credentialsInitialized(false)
+    _credentialsInitialized(false),
+    _verified(false)
 {
 }
 
@@ -991,6 +996,8 @@ IceSSL::TransceiverI::getNativeConnectionInfo() const
         info->sndSize = IceInternal::getSendBufferSize(_stream->fd());
     }
 
+    info->verified = _verified;
+
     if(_sslInitialized)
     {
         CtxtHandle* ssl = const_cast<CtxtHandle*>(&_ssl);
diff --git a/cpp/src/IceSSL/SChannelTransceiverI.h b/cpp/src/IceSSL/SChannelTransceiverI.h
index bfc1b0e020f..a029cf596bf 100644
--- a/cpp/src/IceSSL/SChannelTransceiverI.h
+++ b/cpp/src/IceSSL/SChannelTransceiverI.h
@@ -121,6 +121,7 @@ private:
     CredHandle _credentials;
     bool _credentialsInitialized;
     SecPkgContext_StreamSizes _sizes;
+    bool _verified;
 };
 typedef IceUtil::Handle<TransceiverI> TransceiverIPtr;
 
diff --git a/cpp/src/IceSSL/SSLEngine.cpp b/cpp/src/IceSSL/SSLEngine.cpp
index 2ad615f2f51..81518ee38ef 100644
--- a/cpp/src/IceSSL/SSLEngine.cpp
+++ b/cpp/src/IceSSL/SSLEngine.cpp
@@ -109,7 +109,7 @@ IceSSL::SSLEngine::initialize()
     // chain, including the peer's certificate. A value of 0 means there is
     // no maximum.
     //
-    _verifyDepthMax = properties->getPropertyAsIntWithDefault(propPrefix + "VerifyDepthMax", 2);
+    _verifyDepthMax = properties->getPropertyAsIntWithDefault(propPrefix + "VerifyDepthMax", 3);
 
     //
     // VerifyPeer determines whether certificate validation failures abort a connection.
diff --git a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
index 3feb7774e86..3ff588cd9e2 100644
--- a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
+++ b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
@@ -95,7 +95,7 @@ socketRead(SSLConnectionRef connection, void* data, size_t* length)
     return transceiver->readRaw(reinterpret_cast<char*>(data), length);
 }
 
-void
+bool
 checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, const InstancePtr& instance)
 {
     OSStatus err = noErr;
@@ -132,14 +132,15 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
         //
         // Trust verify success.
         //
-        break;
+        return true;
     }
-    case kSecTrustResultInvalid:
-    //case kSecTrustResultConfirm: // Used in old OS X versions
-    case kSecTrustResultDeny:
-    case kSecTrustResultRecoverableTrustFailure:
-    case kSecTrustResultFatalTrustFailure:
-    case kSecTrustResultOtherError:
+    default:
+    // case kSecTrustResultInvalid:
+    // //case kSecTrustResultConfirm: // Used in old OS X versions
+    // case kSecTrustResultDeny:
+    // case kSecTrustResultRecoverableTrustFailure:
+    // case kSecTrustResultFatalTrustFailure:
+    // case kSecTrustResultOtherError:
     {
         if(engine->getVerifyPeer() == 0)
         {
@@ -149,7 +150,7 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
                 os << "IceSSL: ignoring certificate verification failure\n" << trustResultDescription(trustResult);
                 instance->logger()->trace(instance->traceCategory(), os.str());
             }
-            break;
+            return false;
         }
         else
         {
@@ -236,7 +237,7 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
             }
             if(err == noErr)
             {
-                checkTrustResult(_trust, _engine, _instance);
+                _verified = checkTrustResult(_trust, _engine, _instance);
                 continue; // Call SSLHandshake to resume the handsake.
             }
             // Let it fall through, this will raise a SecurityException with the SSLCopyPeerTrust error.
@@ -510,6 +511,7 @@ IceSSL::TransceiverI::TransceiverI(const InstancePtr& instance,
     _stream(stream),
     _ssl(0),
     _trust(0),
+    _verified(false),
     _buffered(0)
 {
     //
@@ -551,6 +553,11 @@ IceSSL::TransceiverI::getNativeConnectionInfo() const
         SSLCipherSuite cipher;
         SSLGetNegotiatedCipher(_ssl, &cipher);
         info->cipher = _engine->getCipherName(cipher);
+        info->verified = _verified;
+    }
+    else
+    {
+        info->verified = false;
     }
 
     info->adapterName = _adapterName;
diff --git a/cpp/src/IceSSL/SecureTransportTransceiverI.h b/cpp/src/IceSSL/SecureTransportTransceiverI.h
index afdcccc2018..aaf232c4032 100644
--- a/cpp/src/IceSSL/SecureTransportTransceiverI.h
+++ b/cpp/src/IceSSL/SecureTransportTransceiverI.h
@@ -71,7 +71,8 @@ private:
 
     SSLContextRef _ssl;
     SecTrustRef _trust;
-
+    bool _verified;
+    
     size_t _buffered;
     enum SSLWantFlags
     {