7 files changed, 135 insertions, 71 deletions
diff --git a/Swiften/TLS/OpenSSL/OpenSSLCertificate.cpp b/Swiften/TLS/OpenSSL/OpenSSLCertificate.cpp
index 8d2d965..66b650d 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLCertificate.cpp
+++ b/Swiften/TLS/OpenSSL/OpenSSLCertificate.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2019 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -32,11 +32,19 @@ OpenSSLCertificate::OpenSSLCertificate(const ByteArray& der) {
 #endif
     cert = std::shared_ptr<X509>(d2i_X509(nullptr, &p, der.size()), X509_free);
     if (!cert) {
-        SWIFT_LOG(warning) << "Error creating certificate from DER data" << std::endl;
+//        SWIFT_LOG(warning) << "Error creating certificate from DER data";
     }
     parse();
 }
 
+void OpenSSLCertificate::incrementReferenceCount() const {
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L
+    X509_up_ref(cert.get());
+#else
+    CRYPTO_add(&(cert.get()->references), 1, CRYPTO_LOCK_EVP_PKEY);
+#endif
+}
+
 ByteArray OpenSSLCertificate::toDER() const {
     ByteArray result;
     if (!cert) {
diff --git a/Swiften/TLS/OpenSSL/OpenSSLCertificate.h b/Swiften/TLS/OpenSSL/OpenSSLCertificate.h
index 186caea..64da82a 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLCertificate.h
+++ b/Swiften/TLS/OpenSSL/OpenSSLCertificate.h
@@ -45,6 +45,8 @@ namespace Swift {
                 return cert;
             }
 
+            void incrementReferenceCount() const;
+
         private:
             void parse();
 
diff --git a/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.cpp b/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.cpp
index 5eb626b..73058a5 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.cpp
+++ b/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.cpp
@@ -7,6 +7,7 @@
 #include <Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.h>
 
 #include <openssl/pem.h>
+#include <openssl/err.h>
 
 namespace Swift {
 
@@ -20,8 +21,8 @@ Certificate* OpenSSLCertificateFactory::createCertificateFromDER(const ByteArray
     return new OpenSSLCertificate(der);
 }
 
-std::vector<std::unique_ptr<Certificate>> OpenSSLCertificateFactory::createCertificateChain(const ByteArray& data) {
-    std::vector<std::unique_ptr<Certificate>> certificateChain;
+std::vector<std::shared_ptr<Certificate>> OpenSSLCertificateFactory::createCertificateChain(const ByteArray& data) {
+    std::vector<std::shared_ptr<Certificate>> certificateChain;
 
     if (data.size() > std::numeric_limits<int>::max()) {
         return certificateChain;
@@ -35,15 +36,20 @@ std::vector<std::unique_ptr<Certificate>> OpenSSLCertificateFactory::createCerti
     auto x509certFromPEM = PEM_read_bio_X509(bio.get(), &openSSLCert, nullptr, nullptr);
     if (x509certFromPEM && openSSLCert) {
         std::shared_ptr<X509> x509Cert(openSSLCert, X509_free);
-        certificateChain.emplace_back(std::make_unique<OpenSSLCertificate>(x509Cert));
+        certificateChain.emplace_back(std::make_shared<OpenSSLCertificate>(x509Cert));
         openSSLCert = nullptr;
         while ((x509certFromPEM = PEM_read_bio_X509(bio.get(), &openSSLCert, nullptr, nullptr)) != nullptr) {
             std::shared_ptr<X509> x509Cert(openSSLCert, X509_free);
-            certificateChain.emplace_back(std::make_unique<OpenSSLCertificate>(x509Cert));
+            certificateChain.emplace_back(std::make_shared<OpenSSLCertificate>(x509Cert));
             openSSLCert = nullptr;
         }
     }
 
+    // Clear any (expected) errors which resulted from PEM parsing
+    // If we don't do this, any existing TLS context will detect these
+    // spurious errors and fail to work
+    ERR_clear_error();
+
     return certificateChain;
 }
 
diff --git a/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.h b/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.h
index 48e9b2c..a6974c8 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.h
+++ b/Swiften/TLS/OpenSSL/OpenSSLCertificateFactory.h
@@ -16,6 +16,6 @@ namespace Swift {
             virtual ~OpenSSLCertificateFactory() override final;
 
             virtual Certificate* createCertificateFromDER(const ByteArray& der) override final;
-            virtual std::vector<std::unique_ptr<Certificate>> createCertificateChain(const ByteArray& data) override final;
+            virtual std::vector<std::shared_ptr<Certificate>> createCertificateChain(const ByteArray& data) override final;
     };
 }
diff --git a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
index 5c80976..86b0504 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
+++ b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
@@ -121,52 +121,57 @@ OpenSSLContext::OpenSSLContext(const TLSOptions& options, Mode mode) : mode_(mod
 
     // TODO: implement OCSP support
     // TODO: handle OCSP stapling see https://www.rfc-editor.org/rfc/rfc4366.txt
-    // Load system certs
+
+    // Default for ignoreSystemTrustAnchors is false, i.e. load System TAs by default,
+    // to preserve previous behaviour
+    if (!options.ignoreSystemTrustAnchors) {
+        // Load system certs
 #if defined(SWIFTEN_PLATFORM_WINDOWS)
-    X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
-    HCERTSTORE systemStore = CertOpenSystemStore(0, "ROOT");
-    if (systemStore) {
-        PCCERT_CONTEXT certContext = nullptr;
-        while (true) {
-            certContext = CertFindCertificateInStore(systemStore, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, 0, CERT_FIND_ANY, nullptr, certContext);
-            if (!certContext) {
-                break;
-            }
-            OpenSSLCertificate cert(createByteArray(certContext->pbCertEncoded, certContext->cbCertEncoded));
-            if (store && cert.getInternalX509()) {
-                X509_STORE_add_cert(store, cert.getInternalX509().get());
+        X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
+        HCERTSTORE systemStore = CertOpenSystemStore(0, "ROOT");
+        if (systemStore) {
+            PCCERT_CONTEXT certContext = nullptr;
+            while (true) {
+                certContext = CertFindCertificateInStore(systemStore, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, 0, CERT_FIND_ANY, nullptr, certContext);
+                if (!certContext) {
+                    break;
+                }
+                OpenSSLCertificate cert(createByteArray(certContext->pbCertEncoded, certContext->cbCertEncoded));
+                if (store && cert.getInternalX509()) {
+                    X509_STORE_add_cert(store, cert.getInternalX509().get());
+                }
             }
         }
-    }
 #elif !defined(SWIFTEN_PLATFORM_MACOSX)
-    SSL_CTX_set_default_verify_paths(context_.get());
+        SSL_CTX_set_default_verify_paths(context_.get());
 #elif defined(SWIFTEN_PLATFORM_MACOSX) && !defined(SWIFTEN_PLATFORM_IPHONE)
-    // On Mac OS X 10.5 (OpenSSL < 0.9.8), OpenSSL does not automatically look in the system store.
-    // On Mac OS X 10.6 (OpenSSL >= 0.9.8), OpenSSL *does* look in the system store to determine trust.
-    // However, if there is a certificate error, it will always emit the "Invalid CA" error if we didn't add
-    // the certificates first. See
-    //        http://opensource.apple.com/source/OpenSSL098/OpenSSL098-27/src/crypto/x509/x509_vfy_apple.c
-    // to understand why. We therefore add all certs from the system store ourselves.
-    X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
-    CFArrayRef anchorCertificates;
-    if (SecTrustCopyAnchorCertificates(&anchorCertificates) == 0) {
-        for (int i = 0; i < CFArrayGetCount(anchorCertificates); ++i) {
-            SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(const_cast<void*>(CFArrayGetValueAtIndex(anchorCertificates, i)));
-            CSSM_DATA certCSSMData;
-            if (SecCertificateGetData(cert, &certCSSMData) != 0 || certCSSMData.Length == 0) {
-                continue;
-            }
-            std::vector<unsigned char> certData;
-            certData.resize(certCSSMData.Length);
-            memcpy(&certData[0], certCSSMData.Data, certCSSMData.Length);
-            OpenSSLCertificate certificate(certData);
-            if (store && certificate.getInternalX509()) {
-                X509_STORE_add_cert(store, certificate.getInternalX509().get());
+        // On Mac OS X 10.5 (OpenSSL < 0.9.8), OpenSSL does not automatically look in the system store.
+        // On Mac OS X 10.6 (OpenSSL >= 0.9.8), OpenSSL *does* look in the system store to determine trust.
+        // However, if there is a certificate error, it will always emit the "Invalid CA" error if we didn't add
+        // the certificates first. See
+        //        http://opensource.apple.com/source/OpenSSL098/OpenSSL098-27/src/crypto/x509/x509_vfy_apple.c
+        // to understand why. We therefore add all certs from the system store ourselves.
+        X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
+        CFArrayRef anchorCertificates;
+        if (SecTrustCopyAnchorCertificates(&anchorCertificates) == 0) {
+            for (int i = 0; i < CFArrayGetCount(anchorCertificates); ++i) {
+                SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(const_cast<void*>(CFArrayGetValueAtIndex(anchorCertificates, i)));
+                CSSM_DATA certCSSMData;
+                if (SecCertificateGetData(cert, &certCSSMData) != 0 || certCSSMData.Length == 0) {
+                    continue;
+                }
+                std::vector<unsigned char> certData;
+                certData.resize(certCSSMData.Length);
+                memcpy(&certData[0], certCSSMData.Data, certCSSMData.Length);
+                OpenSSLCertificate certificate(certData);
+                if (store && certificate.getInternalX509()) {
+                    X509_STORE_add_cert(store, certificate.getInternalX509().get());
+                }
             }
+            CFRelease(anchorCertificates);
         }
-        CFRelease(anchorCertificates);
-    }
 #endif
+    }
     configure(options);
 }
 
@@ -202,7 +207,7 @@ static int certVerifyCallback(X509_STORE_CTX* store_ctx, void* arg)
     if (cb != nullptr) {
         ret = cb(static_cast<const OpenSSLContext*>(context));
     } else {
-        SWIFT_LOG(warning) << "certVerifyCallback called but context.verifyCertCallback is unset" << std::endl;
+        SWIFT_LOG(debug) << "certVerifyCallback called but context.verifyCertCallback is unset";
         ret = 0;
     }
 
@@ -245,12 +250,12 @@ static int verifyCallback(int preverifyOk, X509_STORE_CTX* ctx)
     SSL* ssl = static_cast<SSL*>(X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
     SSL_CTX* sslctx = ssl ? SSL_get_SSL_CTX(ssl) : nullptr;
     if (!sslctx) {
-        SWIFT_LOG(error) << "verifyCallback: internal error" << std::endl;
+        SWIFT_LOG(debug) << "verifyCallback: internal error";
         return preverifyOk;
     }
 
     if (SSL_CTX_get_verify_mode(sslctx) == SSL_VERIFY_NONE) {
-        SWIFT_LOG(info) << "verifyCallback: no verification required" << std::endl;
+        SWIFT_LOG(debug) << "verifyCallback: no verification required";
         // No verification requested
         return 1;
     }
@@ -281,14 +286,18 @@ static int verifyCallback(int preverifyOk, X509_STORE_CTX* ctx)
             X509_NAME* issuerName = X509_get_issuer_name(errCert);
             issuerString = X509_NAME_to_text(issuerName);
         }
-        SWIFT_LOG(error) << "verifyCallback: verification error" <<
-            X509_verify_cert_error_string(err) << " depth: " <<
-            depth << " issuer: " << ((issuerString.length() > 0) ? issuerString : "<unknown>") << std::endl;
-    } else {
-        SWIFT_LOG(info) << "verifyCallback: SSL depth: " << depth << " Subject: " <<
-            ((subjectString.length() > 0) ? subjectString : "<>")  << std::endl;
-    }
-    return preverifyOk;
+        SWIFT_LOG(debug) << "verifyCallback: verification error " <<
+          X509_verify_cert_error_string(err) << " depth: " <<
+          depth << " issuer: " << ((issuerString.length() > 0) ? issuerString : "<unknown>");
+     } else {
+        SWIFT_LOG(debug) << "verifyCallback: SSL depth: " << depth << " Subject: " <<
+          ((subjectString.length() > 0) ? subjectString : "<>");
+    }
+    // Always return "OK", as check on verification status
+    // will be performed once TLS handshake has completed,
+    // by calling OpenSSLContext::getVerificationErrorTypeForResult() to
+    // get the value set via X509_STORE_CTX_set_error() above.
+    return 1;
 }
 
 bool OpenSSLContext::configure(const TLSOptions &options)
@@ -296,7 +305,7 @@ bool OpenSSLContext::configure(const TLSOptions &options)
     if (options.cipherSuites) {
         std::string cipherSuites = *(options.cipherSuites);
         if (SSL_CTX_set_cipher_list(context_.get(), cipherSuites.c_str()) != 1 ) {
-            SWIFT_LOG(error) << "Failed to set cipher-suites" << std::endl;
+            SWIFT_LOG(debug) << "Failed to set cipher-suites";
             return false;
         }
     }
@@ -307,7 +316,7 @@ bool OpenSSLContext::configure(const TLSOptions &options)
         if (SSL_CTX_set_session_id_context(context_.get(),
                                            reinterpret_cast<const unsigned char *>(contextId.c_str()),
                                            contextId.length()) != 1) {
-            SWIFT_LOG(error) << "Failed to set context-id" << std::endl;
+            SWIFT_LOG(debug) << "Failed to set context-id";
             return false;
         }
     }
@@ -315,12 +324,12 @@ bool OpenSSLContext::configure(const TLSOptions &options)
     if (options.sessionCacheTimeout) {
         int scto = *options.sessionCacheTimeout;
         if (scto <= 0) {
-            SWIFT_LOG(error) << "Invalid value for session-cache-timeout" << std::endl;
+            SWIFT_LOG(debug) << "Invalid value for session-cache-timeout";
             return false;
         }
         (void)SSL_CTX_set_timeout(context_.get(), scto);
         if (SSL_CTX_get_timeout(context_.get()) != scto) {
-            SWIFT_LOG(error) << "Failed to set session-cache-timeout" << std::endl;
+            SWIFT_LOG(debug) << "Failed to set session-cache-timeout";
             return false;
         }
     }
@@ -362,7 +371,7 @@ bool OpenSSLContext::configure(const TLSOptions &options)
     if (options.verifyDepth) {
         int depth = *options.verifyDepth;
         if (depth <= 0) {
-            SWIFT_LOG(error) << "Invalid value for verify-depth" << std::endl;
+            SWIFT_LOG(debug) << "Invalid value for verify-depth";
             return false;
         }
 
@@ -396,6 +405,21 @@ bool OpenSSLContext::configure(const TLSOptions &options)
     updateOptionIfPresent(options.disableTLSRollBackBug, SSL_OP_TLS_ROLLBACK_BUG);
     updateOptionIfPresent(options.singleDHUse, SSL_OP_SINGLE_DH_USE);
 
+    if (options.trustAnchors) {
+        // Add any additional Trust Anchors which are present in the TLSOptions
+        X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
+
+        if (store) {
+            for (auto& certificate : *options.trustAnchors) {
+                auto openSSLCert = dynamic_cast<OpenSSLCertificate*>(certificate.get());
+                if (openSSLCert && openSSLCert->getInternalX509()) {
+                    X509_STORE_add_cert(store, openSSLCert->getInternalX509().get());
+                    // Don't need to increment reference count as X509_STORE_add_cert does thiS
+                }
+            }
+        }
+    }
+
     return true;
 }
 
@@ -567,9 +591,9 @@ void OpenSSLContext::sendPendingDataToApplication() {
     }
 }
 
-bool OpenSSLContext::setCertificateChain(std::vector<std::unique_ptr<Certificate>>&& certificateChain) {
+bool OpenSSLContext::setCertificateChain(const std::vector<std::shared_ptr<Certificate>>& certificateChain) {
     if (certificateChain.size() == 0) {
-        SWIFT_LOG(warning) << "Trying to load empty certificate chain." << std::endl;
+        SWIFT_LOG(debug) << "Trying to load empty certificate chain.";
         return false;
     }
 
@@ -579,6 +603,7 @@ bool OpenSSLContext::setCertificateChain(std::vector<std::unique_ptr<Certificate
         return false;
     }
 
+    // This increments the reference count on the X509 certificate automatically
     if (SSL_CTX_use_certificate(context_.get(), openSSLCert->getInternalX509().get()) != 1) {
         return false;
     }
@@ -589,11 +614,13 @@ bool OpenSSLContext::setCertificateChain(std::vector<std::unique_ptr<Certificate
             if (!openSSLCert) {
                 return false;
             }
+
             if (SSL_CTX_add_extra_chain_cert(context_.get(), openSSLCert->getInternalX509().get()) != 1) {
-                SWIFT_LOG(warning) << "Trying to load empty certificate chain." << std::endl;
+                SWIFT_LOG(debug) << "Trying to load empty certificate chain.";
                 return false;
             }
-            certificate->release();
+            // Have to manually increment reference count as SSL_CTX_add_extra_chain_cert does not do so
+            openSSLCert->incrementReferenceCount();
         }
     }
 
@@ -639,16 +666,17 @@ bool OpenSSLContext::setPrivateKey(const PrivateKey::ref& privateKey) {
         safePassword.push_back(0);
         password = safePassword.data();
     }
-    auto resultKey = PEM_read_bio_PrivateKey(bio.get(), nullptr, empty_or_preset_password_cb, password);
+    // Make sure resultKey is tidied up by wrapping it in a shared_ptr
+    auto resultKey = std::shared_ptr<EVP_PKEY>(PEM_read_bio_PrivateKey(bio.get(), nullptr, empty_or_preset_password_cb, password), EVP_PKEY_free);
     if (resultKey) {
         if (handle_) {
-            auto result = SSL_use_PrivateKey(handle_.get(), resultKey);;
+            auto result = SSL_use_PrivateKey(handle_.get(), resultKey.get());
             if (result != 1) {
                 return false;
             }
         }
         else {
-            auto result = SSL_CTX_use_PrivateKey(context_.get(), resultKey);
+            auto result = SSL_CTX_use_PrivateKey(context_.get(), resultKey.get());
             if (result != 1) {
                 return false;
             }
@@ -727,13 +755,33 @@ bool OpenSSLContext::setDiffieHellmanParameters(const ByteArray& parametersInOpe
 
 std::vector<Certificate::ref> OpenSSLContext::getPeerCertificateChain() const {
     std::vector<Certificate::ref> result;
+
+    // When this context is a server, the peer (client) certificate
+    // is obtained via SSL_get_peer_certificate, and any other
+    // certificates set by the peer are available via SSL_get_peer_cert_chain.
+    // When this context is a client, all of the server's certificates are
+    // obtained using SSL_get_peer_cert_chain
+    if (mode_ == Mode::Server) {
+        auto cert = SSL_get_peer_certificate(handle_.get());
+        if (cert) {
+            // Do not need to copy the returned cert as SSL_get_peer_certificate
+            // increments the reference count on the certificate
+            std::shared_ptr<X509> x509Cert(cert, X509_free);
+            Certificate::ref cert = std::make_shared<OpenSSLCertificate>(x509Cert);
+            result.push_back(cert);
+        }
+    }
+
     STACK_OF(X509)* chain = SSL_get_peer_cert_chain(handle_.get());
     for (int i = 0; i < sk_X509_num(chain); ++i) {
+        // Here we do need to copy the returned cert, since SSL_get_peer_cert_chain
+        // does not increment the reference count on each certificate
         std::shared_ptr<X509> x509Cert(X509_dup(sk_X509_value(chain, i)), X509_free);
 
         Certificate::ref cert = std::make_shared<OpenSSLCertificate>(x509Cert);
         result.push_back(cert);
     }
+
     return result;
 }
 
diff --git a/Swiften/TLS/OpenSSL/OpenSSLContext.h b/Swiften/TLS/OpenSSL/OpenSSLContext.h
index 885b1fe..8eb5758 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLContext.h
+++ b/Swiften/TLS/OpenSSL/OpenSSLContext.h
@@ -46,7 +46,7 @@ namespace Swift {
             void connect() override final;
             void connect(const std::string& requestHostname) override final;
 
-            bool setCertificateChain(std::vector<std::unique_ptr<Certificate>>&& certificateChain) override final;
+            bool setCertificateChain(const std::vector<std::shared_ptr<Certificate>>& certificateChain) override final;
             bool setPrivateKey(const PrivateKey::ref& privateKey) override final;
             bool setClientCertificate(CertificateWithKey::ref cert) override final;
             void setAbortTLSHandshake(bool abort) override final;
diff --git a/Swiften/TLS/OpenSSL/OpenSSLContextFactory.cpp b/Swiften/TLS/OpenSSL/OpenSSLContextFactory.cpp
index 12445fd..e332ca8 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLContextFactory.cpp
+++ b/Swiften/TLS/OpenSSL/OpenSSLContextFactory.cpp
@@ -47,14 +47,14 @@ ByteArray OpenSSLContextFactory::convertDHParametersFromPEMToDER(const std::stri
 
 void OpenSSLContextFactory::setCheckCertificateRevocation(bool check) {
     if (check) {
-        SWIFT_LOG(warning) << "CRL Checking not supported for OpenSSL" << std::endl;
+        SWIFT_LOG(warning) << "CRL Checking not supported for OpenSSL";
         assert(false);
     }
 }
 
 void OpenSSLContextFactory::setDisconnectOnCardRemoval(bool check) {
     if (check) {
-        SWIFT_LOG(warning) << "Smart cards not supported for OpenSSL" << std::endl;
+        SWIFT_LOG(warning) << "Smart cards not supported for OpenSSL";
     }
 }