From 47d8484227fa4393c078c71b3dbeeda950bb10b1 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Thu, 11 Jul 2024 17:51:07 +0200 Subject: [PATCH 1/5] Use the Windows trusted root certificates for TLS --- src/realm/sync/CMakeLists.txt | 10 +- src/realm/sync/network/network_ssl.cpp | 125 ++++++++++++++++++++++++- src/realm/sync/network/network_ssl.hpp | 2 + test/test_util_network_ssl.cpp | 73 +++++++++++++++ 4 files changed, 202 insertions(+), 8 deletions(-) diff --git a/src/realm/sync/CMakeLists.txt b/src/realm/sync/CMakeLists.txt index ac6b64f7b2e..ed2fce43446 100644 --- a/src/realm/sync/CMakeLists.txt +++ b/src/realm/sync/CMakeLists.txt @@ -108,13 +108,11 @@ elseif(REALM_HAVE_OPENSSL) target_link_libraries(Sync PUBLIC OpenSSL::SSL) endif() -if(WIN32 AND NOT WINDOWS_STORE) - target_link_libraries(Sync INTERFACE Version.lib) - if(CMAKE_VERSION VERSION_LESS "3.21") - # This is needed for OpenSSL, but CMake's FindOpenSSL didn't declare it - # on the OpenSSL::Crypto target until CMake 3.21.0. - target_link_libraries(Sync INTERFACE Crypt32.lib) +if(WIN32) + if(NOT WINDOWS_STORE) + target_link_libraries(Sync INTERFACE Version.lib) endif() + target_link_libraries(Sync INTERFACE Crypt32.lib) endif() install(TARGETS Sync EXPORT realm diff --git a/src/realm/sync/network/network_ssl.cpp b/src/realm/sync/network/network_ssl.cpp index 9f3e59c2269..d06d4a5c269 100644 --- a/src/realm/sync/network/network_ssl.cpp +++ b/src/realm/sync/network/network_ssl.cpp @@ -7,13 +7,15 @@ #include #if REALM_HAVE_OPENSSL +#include +#include #ifdef _WIN32 +using osslX509_NAME = X509_NAME; // alias this before including wincrypt.h because it gets clobbered #include +#include #else #include #endif -#include -#include #elif REALM_HAVE_SECURE_TRANSPORT #include #include @@ -64,6 +66,122 @@ void populate_cert_store_with_included_certs(X509_STORE* store, std::error_code& #endif // REALM_INCLUDE_CERTS +#if REALM_HAVE_OPENSSL && _WIN32 + +/// Allow OpenSSL to look up certificates in the Windows Trusted Root Certification Authority list by implementing the +/// X509_LOOKUP interface. +class CapiLookup { +public: + CapiLookup() + { + // Try to open the store in all of these locations sequentially. Many of them might not exist, and the + // CERT_STORE_OPEN_EXISTING_FLAG flag + // will cause CertOpenStore to return null in which case we just move on to the next. + // The order is important - we go from the most likely to the least likely to optimize lookup. + static std::initializer_list store_locations{CERT_SYSTEM_STORE_CURRENT_USER, + CERT_SYSTEM_STORE_LOCAL_MACHINE, + CERT_SYSTEM_STORE_CURRENT_SERVICE, + CERT_SYSTEM_STORE_SERVICES, + CERT_SYSTEM_STORE_USERS, + CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, + CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, + CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE}; + + for (DWORD location : store_locations) { + constexpr DWORD flags = + CERT_STORE_READONLY_FLAG | CERT_STORE_SHARE_CONTEXT_FLAG | CERT_STORE_OPEN_EXISTING_FLAG; + HCERTSTORE store = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, NULL, flags | location, L"ROOT"); + if (store) + m_stores.push_back(store); + } + } + + ~CapiLookup() + { + for (auto store : m_stores) { + CertCloseStore(store, 0); + } + } + + int get_by_subject(X509_LOOKUP* ctx, X509_LOOKUP_TYPE type, const osslX509_NAME* name, X509_OBJECT* ret) + { + if (type != X509_LU_X509) + return 0; + + // Convert the OpenSSL X509_NAME structure into its ASN.1 representation and construct a CAPI search parameter + CERT_NAME_BLOB capi_name = {0}; + capi_name.cbData = i2d_X509_NAME(name, &capi_name.pbData); + int result = 0; + + for (auto store : m_stores) { + PCCERT_CONTEXT cert = + CertFindCertificateInStore(store, X509_ASN_ENCODING, 0, CERT_FIND_SUBJECT_NAME, &capi_name, NULL); + if (!cert) + continue; + + // Convert the ASN.1 representation of the CAPI certificate into an OpenSSL certificate and add it to the + // OpenSSL store + const unsigned char* encoded_cert_data = cert->pbCertEncoded; + X509* ossl_cert = d2i_X509(NULL, &encoded_cert_data, cert->cbCertEncoded); + result = X509_STORE_add_cert(X509_LOOKUP_get_store(ctx), ossl_cert); + X509_free(ossl_cert); + break; + } + + OPENSSL_free(capi_name.pbData); + + // if we previously added a certificate to the store we need to look it up again from the store and return + // that + if (result) + result = get_cached_object(ctx, type, name, ret); + return result; + } + +private: + int get_cached_object(X509_LOOKUP* ctx, X509_LOOKUP_TYPE type, const osslX509_NAME* name, X509_OBJECT* ret) + { + REALM_ASSERT_RELEASE(type == X509_LU_X509); + + // Loop through the objects already in the store to find the one we just added in get_by_subject. + // retrieve_by_subject returns a cert with refcount 1 but set1_X509 increases it. + // That's why we need to free it after before returning, otherwise it will leak. + + STACK_OF(X509_OBJECT)* objects = X509_STORE_get0_objects(X509_LOOKUP_get_store(ctx)); + X509_OBJECT* tmp = X509_OBJECT_retrieve_by_subject(objects, type, name); + if (!tmp) + return 0; + + X509* cert = X509_OBJECT_get0_X509(tmp); + int result = X509_OBJECT_set1_X509(ret, cert); + X509_free(cert); + + return result; + } + + std::vector m_stores; +}; + +void add_windows_certificate_store_lookup(X509_STORE* store) +{ + X509_LOOKUP_METHOD* capi_lookup = X509_LOOKUP_meth_new("capi"); + + X509_LOOKUP_meth_set_new_item(capi_lookup, [](X509_LOOKUP* ctx) { + auto* data = new CapiLookup(); + return X509_LOOKUP_set_method_data(ctx, data); + }); + X509_LOOKUP_meth_set_free(capi_lookup, [](X509_LOOKUP* ctx) { + auto* data = reinterpret_cast(X509_LOOKUP_get_method_data(ctx)); + delete data; + }); + X509_LOOKUP_meth_set_get_by_subject(capi_lookup, [](auto ctx, auto type, auto name, auto ret) { + auto* data = reinterpret_cast(X509_LOOKUP_get_method_data(ctx)); + return data->get_by_subject(ctx, type, name, ret); + }); + + X509_STORE_add_lookup(store, capi_lookup); +} + +#endif // REALM_HAVE_OPENSSL && _WIN32 #if REALM_HAVE_OPENSSL && (OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)) @@ -330,6 +448,9 @@ void Context::ssl_use_default_verify(std::error_code& ec) ec = std::error_code(int(ERR_get_error()), openssl_error_category); return; } +#endif +#ifdef _WIN32 + add_windows_certificate_store_lookup(SSL_CTX_get_cert_store(m_ssl_ctx)); #endif ec = std::error_code(); } diff --git a/src/realm/sync/network/network_ssl.hpp b/src/realm/sync/network/network_ssl.hpp index 97060b67511..07b60794764 100644 --- a/src/realm/sync/network/network_ssl.hpp +++ b/src/realm/sync/network/network_ssl.hpp @@ -131,6 +131,8 @@ class Context { /// default certificates for server verification. For OpenSSL, /// use_default_verify() corresponds to /// SSL_CTX_set_default_verify_paths(SSL_CTX*); + /// + /// On Windows this also adds a lookup to the system Trusted Root Certification Authorities list. void use_default_verify(); /// The verify file is a PEM file containing trust certificates that the diff --git a/test/test_util_network_ssl.cpp b/test/test_util_network_ssl.cpp index 9d260527333..0e123073981 100644 --- a/test/test_util_network_ssl.cpp +++ b/test/test_util_network_ssl.cpp @@ -6,6 +6,10 @@ #include "test.hpp" #include "util/semaphore.hpp" +#ifdef _WIN32 +#include +#endif + using namespace realm; using namespace realm::sync; using namespace realm::test_util; @@ -993,6 +997,7 @@ TEST(Util_Network_SSL_Certificate_CN_SAN) ssl_context_1.use_certificate_chain_file(ca_dir + "dns-chain.crt.pem"); ssl_context_1.use_private_key_file(ca_dir + "dns-checked-server.key.pem"); ssl_context_2.use_verify_file(ca_dir + "crt.pem"); + ssl_context_2.use_default_verify(); network::ssl::Stream ssl_stream_1{socket_1, ssl_context_1, network::ssl::Stream::server}; network::ssl::Stream ssl_stream_2{socket_2, ssl_context_2, network::ssl::Stream::client}; @@ -1216,4 +1221,72 @@ TEST(Util_Network_SSL_Certificate_Failure) thread_2.join(); } +#ifdef _WIN32 + +// Adding a trusted root certificate causes a blocking popup to appear so only run this test +// if in an interactive session with a debugger attached, otherwise CI machines will block. +// TODO: maybe use the CI environment variable for the condition? +TEST_IF(Util_Network_SSL_Certificate_From_Windows_Cert_Store, IsDebuggerPresent()) +{ + std::string ca_dir = get_test_resource_path(); + + BIO* file = BIO_new_file((ca_dir + "crt.pem").c_str(), "rb"); + X509* cert = PEM_read_bio_X509(file, NULL, 0, NULL); + BIO_free(file); + + BIO* mem = BIO_new(BIO_s_mem()); + int size = i2d_X509_bio(mem, cert); + BUF_MEM* buffer; + BIO_get_mem_ptr(mem, &buffer); + + PCCERT_CONTEXT cert_context; + HCERTSTORE store = CertOpenSystemStoreA(NULL, "ROOT"); + CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, reinterpret_cast(buffer->data), + buffer->length, CERT_STORE_ADD_USE_EXISTING, &cert_context); + BIO_free(mem); + + network::Service service_1, service_2; + network::Socket socket_1{service_1}, socket_2{service_2}; + network::ssl::Context ssl_context_1; + network::ssl::Context ssl_context_2; + + ssl_context_1.use_certificate_chain_file(ca_dir + "dns-chain.crt.pem"); + ssl_context_1.use_private_key_file(ca_dir + "dns-checked-server.key.pem"); + ssl_context_2.use_default_verify(); // this will import the Windows certificates in the context's certificate store + + network::ssl::Stream ssl_stream_1{socket_1, ssl_context_1, network::ssl::Stream::server}; + network::ssl::Stream ssl_stream_2{socket_2, ssl_context_2, network::ssl::Stream::client}; + ssl_stream_1.set_logger(test_context.logger.get()); + ssl_stream_2.set_logger(test_context.logger.get()); + + ssl_stream_2.set_verify_mode(network::ssl::VerifyMode::peer); + + // We expect success because the certificate is signed for www.example.com + // in both Common Name and SAN. + ssl_stream_2.set_host_name("www.example.com"); + + connect_sockets(socket_1, socket_2); + + auto connector = [&] { + std::error_code ec; + ssl_stream_2.handshake(ec); + CHECK_EQUAL(std::error_code(), ec); + }; + auto acceptor = [&] { + std::error_code ec; + ssl_stream_1.handshake(ec); + CHECK_EQUAL(std::error_code(), ec); + }; + + std::thread thread_1(std::move(connector)); + std::thread thread_2(std::move(acceptor)); + thread_1.join(); + thread_2.join(); + + CertDeleteCertificateFromStore(cert_context); + CertCloseStore(store, 0); +} + +#endif // _WIN32 + #endif // !REALM_MOBILE From 87aab872b4baa4ebc431b9d9f9675a397d9d4fb9 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Thu, 11 Jul 2024 17:57:30 +0200 Subject: [PATCH 2/5] revert change --- test/test_util_network_ssl.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test_util_network_ssl.cpp b/test/test_util_network_ssl.cpp index 0e123073981..446c54cf934 100644 --- a/test/test_util_network_ssl.cpp +++ b/test/test_util_network_ssl.cpp @@ -997,7 +997,6 @@ TEST(Util_Network_SSL_Certificate_CN_SAN) ssl_context_1.use_certificate_chain_file(ca_dir + "dns-chain.crt.pem"); ssl_context_1.use_private_key_file(ca_dir + "dns-checked-server.key.pem"); ssl_context_2.use_verify_file(ca_dir + "crt.pem"); - ssl_context_2.use_default_verify(); network::ssl::Stream ssl_stream_1{socket_1, ssl_context_1, network::ssl::Stream::server}; network::ssl::Stream ssl_stream_2{socket_2, ssl_context_2, network::ssl::Stream::client}; From dc966f6183bcf6235cca6a0e08d612aff36454a9 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Thu, 11 Jul 2024 18:10:48 +0200 Subject: [PATCH 3/5] lint --- test/test_util_network_ssl.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test_util_network_ssl.cpp b/test/test_util_network_ssl.cpp index 446c54cf934..2a69b037fbf 100644 --- a/test/test_util_network_ssl.cpp +++ b/test/test_util_network_ssl.cpp @@ -1251,7 +1251,8 @@ TEST_IF(Util_Network_SSL_Certificate_From_Windows_Cert_Store, IsDebuggerPresent( ssl_context_1.use_certificate_chain_file(ca_dir + "dns-chain.crt.pem"); ssl_context_1.use_private_key_file(ca_dir + "dns-checked-server.key.pem"); - ssl_context_2.use_default_verify(); // this will import the Windows certificates in the context's certificate store + ssl_context_2 + .use_default_verify(); // this will import the Windows certificates in the context's certificate store network::ssl::Stream ssl_stream_1{socket_1, ssl_context_1, network::ssl::Stream::server}; network::ssl::Stream ssl_stream_2{socket_2, ssl_context_2, network::ssl::Stream::client}; From c0bea32d6c425d9da50258696bec2ddc63d0b623 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Thu, 11 Jul 2024 18:17:33 +0200 Subject: [PATCH 4/5] changelog --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c0032ba6e1..67ba754843e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,7 @@ # NEXT RELEASE ### Enhancements -* (PR [#????](https://github.com/realm/realm-core/pull/????)) -* None. +* On Windows devices Device Sync will additionally look up SSL certificates in the Windows Trusted Root Certification Authorities certificate store when establishing a connection. (PR [#7882](https://github.com/realm/realm-core/pull/7882)) ### Fixed * When a public name is defined on a property, calling `realm::Results::sort()` or `realm::Results::distinct()` with the internal name could throw an error like `Cannot sort on key path 'NAME': property 'PersonObject.NAME' does not exist`. ([realm/realm-js#6779](https://github.com/realm/realm-js/issues/6779), since v12.12.0) From 8fe4fa2877aa8a4dff0f88c38e3113d9760402a1 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Fri, 12 Jul 2024 17:46:26 +0200 Subject: [PATCH 5/5] fix warning --- test/test_util_network_ssl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_util_network_ssl.cpp b/test/test_util_network_ssl.cpp index 2a69b037fbf..c4fbe65eb6c 100644 --- a/test/test_util_network_ssl.cpp +++ b/test/test_util_network_ssl.cpp @@ -1241,7 +1241,7 @@ TEST_IF(Util_Network_SSL_Certificate_From_Windows_Cert_Store, IsDebuggerPresent( PCCERT_CONTEXT cert_context; HCERTSTORE store = CertOpenSystemStoreA(NULL, "ROOT"); CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, reinterpret_cast(buffer->data), - buffer->length, CERT_STORE_ADD_USE_EXISTING, &cert_context); + static_cast(buffer->length), CERT_STORE_ADD_USE_EXISTING, &cert_context); BIO_free(mem); network::Service service_1, service_2;