From 19bec178fb682ce791c7ccfced14d4518b4119bb Mon Sep 17 00:00:00 2001 From: David Sommerseth Date: Thu, 6 Jun 2024 15:38:41 +0200 Subject: [PATCH 1/9] Preparing QA cycle for OpenVPN 3 Core library release v3.10 Signed-off-by: David Sommerseth --- openvpn/common/version.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openvpn/common/version.hpp b/openvpn/common/version.hpp index 52b5a408..487e500b 100644 --- a/openvpn/common/version.hpp +++ b/openvpn/common/version.hpp @@ -24,5 +24,5 @@ #pragma once #ifndef OPENVPN_VERSION -#define OPENVPN_VERSION "3.9_qa" +#define OPENVPN_VERSION "3.10_qa" #endif From 1477df691e13504b5f95c499c515aee5a6a9ac88 Mon Sep 17 00:00:00 2001 From: Heiko Hund Date: Tue, 11 Jun 2024 15:45:44 +0200 Subject: [PATCH 2/9] mac agent: reinstall host route during restart The host route to the VPN server disappeared when a mac client, using the agent, was reconnecting. That was causing --redirect-gateway tunnels to break because no traffic could be sent anymore. Cause for this was some internal state in the agent not being reset when the utun device is temporarily removed during the restart. Signed-off-by: Heiko Hund --- openvpn/ovpnagent/mac/ovpnagent.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/openvpn/ovpnagent/mac/ovpnagent.cpp b/openvpn/ovpnagent/mac/ovpnagent.cpp index 1eda1549..3b85569a 100644 --- a/openvpn/ovpnagent/mac/ovpnagent.cpp +++ b/openvpn/ovpnagent/mac/ovpnagent.cpp @@ -290,6 +290,7 @@ class MyListener : public WS::Server::Listener remove_cmds_bypass_hosts.execute(os); remove_cmds_bypass_hosts.clear(); + bypass_host.clear(); } void set_watchdog(pid_t pid) From 5e83af3e2c99095ebdb87a714e8146746fe19d3f Mon Sep 17 00:00:00 2001 From: David Sommerseth Date: Wed, 19 Jun 2024 20:57:15 +0200 Subject: [PATCH 3/9] Fix spelling errors raised by Debian linter Reported-by: Marc Leeman Signed-off-by: David Sommerseth --- openvpn/client/cliconnect.hpp | 2 +- openvpn/openssl/ssl/sslctx.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openvpn/client/cliconnect.hpp b/openvpn/client/cliconnect.hpp index 14ebf7c4..2ac81162 100644 --- a/openvpn/client/cliconnect.hpp +++ b/openvpn/client/cliconnect.hpp @@ -446,7 +446,7 @@ class ClientConnect : ClientProto::NotifyCallback, auto timer_left = std::chrono::duration_cast(conn_timer.expiry() - AsioTimer::clock_type::now()).count(); if (timer_left < timeout) { - OPENVPN_LOG("Extending connection timeout from " << timer_left << " to " << timeout << " for pending authentification"); + OPENVPN_LOG("Extending connection timeout from " << timer_left << " to " << timeout << " for pending authentication"); conn_timer.cancel(); conn_timer_pending = false; conn_timer_start(timeout); diff --git a/openvpn/openssl/ssl/sslctx.hpp b/openvpn/openssl/ssl/sslctx.hpp index 13c0ad07..5fe5b394 100644 --- a/openvpn/openssl/ssl/sslctx.hpp +++ b/openvpn/openssl/ssl/sslctx.hpp @@ -631,7 +631,7 @@ class OpenSSLContext : public SSLFactoryAPI default_provider.reset(OSSL_PROVIDER_load(lib_ctx.get(), "default")); if (!default_provider) - throw OpenSSLException("OpenSSLContext: laoding default provider failed"); + throw OpenSSLException("OpenSSLContext: loading default provider failed"); } #endif } From ac4f7a5c19a4c6080f188469a29c712a01f70363 Mon Sep 17 00:00:00 2001 From: Charlie Vigue Date: Wed, 5 Jun 2024 15:28:18 +0000 Subject: [PATCH 4/9] Change cxa1 protocol tag to dpc1 Minor change to a string tag. Signed-off-by: Charlie Vigue --- test/ovpncli/cli.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/ovpncli/cli.cpp b/test/ovpncli/cli.cpp index a4479f9b..d3a1c1ed 100644 --- a/test/ovpncli/cli.cpp +++ b/test/ovpncli/cli.cpp @@ -216,7 +216,7 @@ class ClientBase : public ClientAPI::OpenVPNClient class Client : public ClientBase { // This is the protocol tag that appcontrol expects to trigger the certcheck start. - static constexpr char certcheck_init_verb[] = "cxa1"; // TODO: std::string in C++ '20 + static constexpr char certcheck_init_verb[] = "dpc1"; // TODO: std::string in C++ '20 public: enum ClockTickAction @@ -369,7 +369,7 @@ class Client : public ClientBase } /** - @brief Begin a cck1 (certcheck) handshake in response to a cxa1 server request. + @brief Begin a cck1 (certcheck) handshake in response to a dpc1 server request. */ void handle_certcheck_request() { From fdead3f04c0b66ae6a515e2e92bda4d50aeb61bb Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Thu, 20 Jun 2024 18:22:17 +0200 Subject: [PATCH 5/9] Allow setting a maximum TLS version This is something useful for debugging. We do not expose this feature to avoid it being used for real connections. Signed-off-by: Arne Schwabe --- openvpn/mbedtls/ssl/sslctx.hpp | 4 ++++ openvpn/openssl/ssl/sslctx.hpp | 33 ++++++++++++++++++++++++++++++--- openvpn/ssl/sslapi.hpp | 1 + 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/openvpn/mbedtls/ssl/sslctx.hpp b/openvpn/mbedtls/ssl/sslctx.hpp index 7789608a..f6c3d28a 100644 --- a/openvpn/mbedtls/ssl/sslctx.hpp +++ b/openvpn/mbedtls/ssl/sslctx.hpp @@ -412,6 +412,10 @@ class MbedTLSContext : public SSLFactoryAPI tls_version_min = tvm; } + virtual void set_tls_version_max(const TLSVersion::Type tvm) + { + } + virtual void set_tls_version_min_override(const std::string &override) { TLSVersion::apply_override(tls_version_min, override); diff --git a/openvpn/openssl/ssl/sslctx.hpp b/openvpn/openssl/ssl/sslctx.hpp index 5fe5b394..cd7036cc 100644 --- a/openvpn/openssl/ssl/sslctx.hpp +++ b/openvpn/openssl/ssl/sslctx.hpp @@ -322,6 +322,11 @@ class OpenSSLContext : public SSLFactoryAPI tls_version_min = tvm; } + void set_tls_version_max(const TLSVersion::Type tvm) override + { + tls_version_max = tvm; + } + void set_tls_version_min_override(const std::string &override) override { TLSVersion::apply_override(tls_version_min, override); @@ -678,9 +683,10 @@ class OpenSSLContext : public SSLFactoryAPI std::vector ku; // if defined, peer cert X509 key usage must match one of these values std::string eku; // if defined, peer cert X509 extended key usage must match this OID/string std::string tls_remote; - VerifyX509Name verify_x509_name; // --verify-x509-name feature - PeerFingerprints peer_fingerprints; // --peer-fingerprint - TLSVersion::Type tls_version_min{TLSVersion::Type::V1_2}; // minimum TLS version that we will negotiate + VerifyX509Name verify_x509_name; // --verify-x509-name feature + PeerFingerprints peer_fingerprints; // --peer-fingerprint + TLSVersion::Type tls_version_min{TLSVersion::Type::V1_2}; // minimum TLS version that we will negotiate + TLSVersion::Type tls_version_max{TLSVersion::Type::UNDEF}; // maximum TLS version that we will negotiate, use for testing only (NOT exposed via tls-version-max) TLSCertProfile::Type tls_cert_profile{TLSCertProfile::UNDEF}; std::string tls_cipher_list; std::string tls_ciphersuite_list; @@ -1260,6 +1266,27 @@ class OpenSSLContext : public SSLFactoryAPI if (config->tls_version_min > TLSVersion::Type::V1_3) sslopt |= SSL_OP_NO_TLSv1_3; #endif +#endif +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + if (config->tls_version_max != TLSVersion::Type::UNDEF) + { + SSL_CTX_set_max_proto_version(ctx.get(), TLSVersion::toTLSVersion(config->tls_version_max)); + } +#else + if (config->tls_version_max < TLSVersion::Type::V1_0) + sslopt |= SSL_OP_NO_TLSv1; +#ifdef SSL_OP_NO_TLSv1_1 + if (config->tls_version_max < TLSVersion::Type::V1_1) + sslopt |= SSL_OP_NO_TLSv1_1; +#endif +#ifdef SSL_OP_NO_TLSv1_2 + if (config->tls_version_max < TLSVersion::Type::V1_2) + sslopt |= SSL_OP_NO_TLSv1_2; +#endif +#ifdef SSL_OP_NO_TLSv1_3 + if (config->tls_version_max < TLSVersion::Type::V1_3) + sslopt |= SSL_OP_NO_TLSv1_3; +#endif #endif SSL_CTX_set_options(ctx.get(), sslopt); diff --git a/openvpn/ssl/sslapi.hpp b/openvpn/ssl/sslapi.hpp index ef193808..1cc9f8f2 100644 --- a/openvpn/ssl/sslapi.hpp +++ b/openvpn/ssl/sslapi.hpp @@ -176,6 +176,7 @@ class SSLConfigAPI : public RC virtual void set_remote_cert_tls(const KUParse::TLSWebType wt) = 0; virtual void set_tls_remote(const std::string &tls_remote_arg) = 0; virtual void set_tls_version_min(const TLSVersion::Type tvm) = 0; + virtual void set_tls_version_max(const TLSVersion::Type tvm) = 0; virtual void set_tls_version_min_override(const std::string &override) = 0; virtual void set_tls_cert_profile(const TLSCertProfile::Type type) = 0; virtual void set_tls_cert_profile_override(const std::string &override) = 0; From 2747bfc1d16c513e6dc9b8748e00f8eb4a69a192 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Thu, 20 Jun 2024 18:23:35 +0200 Subject: [PATCH 6/9] Implement changes to allow test dpc certcheck to be tested Signed-off-by: Arne Schwabe --- test/ovpncli/cli.cpp | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/test/ovpncli/cli.cpp b/test/ovpncli/cli.cpp index d3a1c1ed..89bdb3dd 100644 --- a/test/ovpncli/cli.cpp +++ b/test/ovpncli/cli.cpp @@ -343,6 +343,33 @@ class Client : public ClientBase } } + void handle_dpc1_protocol(const ClientAPI::AppCustomControlMessageEvent &acev) + { + if (string::starts_with(acev.payload, "{\"dpc_request\"") && acev.payload.find("certificate") != std::string::npos) + { + std::cout << "ACC CERTCHECK challenge initiated" << std::endl; + handle_certcheck_request(); + } + else if (string::starts_with(acev.payload, "{\"dpc_request\"") && acev.payload.find("client_info") != std::string::npos) + { + std::string fakeResponse{R"("dpc_response\": { + "client_info" : { + "os" : {"type" : "FakeOS", "version" : "1.2.3.4" } + } + })"}; + + send_app_control_channel_msg("dpc1", fakeResponse); + } + else if (string::starts_with(acev.payload, "{\"dpc_request\"")) + { + std::cout << "Cannot parse dpc request message:" << acev.payload << std::endl; + } + else + { + std::cout << "Cannot parse device posture message:" << acev.payload << std::endl; + } + } + /** @brief Handles ACC messages @param acev The current ACC event @@ -355,15 +382,11 @@ class Client : public ClientBase } else if (acev.protocol == certcheck_init_verb) { - if (string::starts_with(acev.payload, "{\"dpc_certcheck_cert_req\"")) - { - std::cout << "ACC CERTCHECK challenge initiated\n"; - handle_certcheck_request(); - } + handle_dpc1_protocol(acev); } else { - std::cout << "received app custom control message for protocol " << acev.protocol + std::cout << "received unhandled app custom control message for protocol " << acev.protocol << " msg payload: " << acev.payload << std::endl; } } From dca41905a53e5d782c65b3f4a8b9c12d7e00bc91 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Thu, 20 Jun 2024 18:24:26 +0200 Subject: [PATCH 7/9] Allow disabling TLS 1.3 in certcheck to more easily debug problems Jira: OVPN3-1216 Signed-off-by: Arne Schwabe --- client/ovpncli.cpp | 16 +++++++++++----- client/ovpncli.hpp | 5 +++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/client/ovpncli.cpp b/client/ovpncli.cpp index d4d897e5..542e76fe 100644 --- a/client/ovpncli.cpp +++ b/client/ovpncli.cpp @@ -1339,7 +1339,8 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::send_app_control_channel_msg(const std static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string &client_cert, const std::string &extra_certs, - const std::optional &ca) + const std::optional &ca, + bool disabletls13) { SSLLib::SSLAPI::Config::Ptr config = new SSLLib::SSLAPI::Config; config->set_frame(new Frame(Frame::Context(128, 4096, 4096 - 128, 0, 16, 0))); @@ -1352,6 +1353,9 @@ static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string else flags |= SSLConfigAPI::LF_ALLOW_CLIENT_CERT_NOT_REQUIRED; + if (disabletls13) + config->set_tls_version_max(TLSVersion::Type::V1_2); + config->set_flags(flags); return config; @@ -1362,6 +1366,7 @@ static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string @param client_cert String containing the properly encoded client certificate @param clientkey String containing the properly encoded private key for \p client_cert @param ca String containing the properly encoded authority + @param disableTLS13 disable TLS 1.3 support Creates, initializes,and installs an SSLLib::SSLAPI::Config object into the TLS handshake object we use for the certcheck function. Then begins the handshake @@ -1369,14 +1374,15 @@ static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string */ OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check(const std::string &client_cert, const std::string &clientkey, - const std::optional &ca) + const std::optional &ca, + bool disableTLS13) { if (state->is_foreign_thread_access()) { ClientConnect *session = state->session.get(); if (session) { - SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(client_cert, "", ca); + SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(client_cert, "", ca, disableTLS13); config->load_private_key(clientkey); session->start_acc_certcheck(config); @@ -1384,7 +1390,7 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check(const std::string &cl } } -OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check_epki(const std::string &alias, const std::optional &ca) +OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check_epki(const std::string &alias, const std::optional &ca, bool disableTLS13) { if (state->is_foreign_thread_access()) { @@ -1401,7 +1407,7 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check_epki(const std::strin return; } - SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(req.cert, req.supportingChain, ca); + SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(req.cert, req.supportingChain, ca, disableTLS13); config->set_external_pki_callback(this, alias); diff --git a/client/ovpncli.hpp b/client/ovpncli.hpp index 7c03f2ed..e73e62c5 100644 --- a/client/ovpncli.hpp +++ b/client/ovpncli.hpp @@ -705,7 +705,8 @@ class OpenVPNClient : public TunBuilderBase, // expose tun builder v */ void start_cert_check(const std::string &client_cert, const std::string &clientkey, - const std::optional &ca = std::nullopt); + const std::optional &ca = std::nullopt, + bool disableTLS13 = false); /** @brief Start up the cert check handshake using the given epki_alias string @@ -716,7 +717,7 @@ class OpenVPNClient : public TunBuilderBase, // expose tun builder v session ACC certcheck TLS handshake object. Every time this function is called the state of the handshake object will be reset and the handshake will be restarted. */ - void start_cert_check_epki(const std::string &alias, const std::optional &ca); + void start_cert_check_epki(const std::string &alias, const std::optional &ca, bool disableTLS13 = false); // Callback for delivering events during connect() call. // Will be called from the thread executing connect(). From 5022f305f59ecb5b2e30b5a6ebd13eb2fa7aef19 Mon Sep 17 00:00:00 2001 From: Lev Stipakov Date: Wed, 26 Jun 2024 11:04:36 +0000 Subject: [PATCH 8/9] aws: account for RandomAPI change Signed-off-by: Lev Stipakov --- openvpn/aws/awsroute.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openvpn/aws/awsroute.hpp b/openvpn/aws/awsroute.hpp index 02993573..c361cfdf 100644 --- a/openvpn/aws/awsroute.hpp +++ b/openvpn/aws/awsroute.hpp @@ -74,7 +74,7 @@ class Route public: Context(PCQuery::Info instance_info_arg, Creds creds_arg, - RandomAPI::Ptr rng, + StrongRandomAPI::Ptr rng, Stop *async_stop_arg, const int debug_level) : instance_info(std::move(instance_info_arg)), From b201027807f4aa6717ec2a976382499da4577dac Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Thu, 4 Jul 2024 18:38:00 +0200 Subject: [PATCH 9/9] Do not reject control message with trailing newlines The previous fix to reject invalid control message was a bit too aggressive as scripts often accidentally include an extra newline at the end of the control message. Jira: OVPN3-1225 Signed-off-by: Arne Schwabe --- openvpn/ssl/proto.hpp | 13 ++++++++--- test/unittests/test_proto.cpp | 41 +++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/openvpn/ssl/proto.hpp b/openvpn/ssl/proto.hpp index dd00b1fa..e3c12720 100644 --- a/openvpn/ssl/proto.hpp +++ b/openvpn/ssl/proto.hpp @@ -1527,12 +1527,19 @@ class ProtoContext : public logging::LoggingMixin 0 && (buf[size - 1] == 0 || buf[size - 1] == '\r' || buf[size - 1] == '\n')) + { --size; + } + if (size) - return S((const char *)buf.c_data(), size); + { + return S{reinterpret_cast(buf.c_data()), size}; + } } - return S(); + return {}; } template diff --git a/test/unittests/test_proto.cpp b/test/unittests/test_proto.cpp index 70637a5f..f01aaf23 100644 --- a/test/unittests/test_proto.cpp +++ b/test/unittests/test_proto.cpp @@ -1327,3 +1327,44 @@ TEST(proto, iv_ciphers_legacy) EXPECT_EQ(ivciphers, expectedstr); } + +TEST(proto, controlmessage_invalidchar) +{ + std::string valid_auth_fail{"AUTH_FAILED: go away"}; + std::string valid_auth_fail_newline_end{"AUTH_FAILED: go away\n"}; + std::string invalid_auth_fail{"AUTH_FAILED: go\n away\n"}; + std::string lot_of_whitespace{"AUTH_FAILED: a lot of white space\n\n\r\n\r\n\r\n"}; + std::string only_whitespace{"\n\n\r\n\r\n\r\n"}; + std::string empty{""}; + + BufferAllocated valid_auth_fail_buf{reinterpret_cast(valid_auth_fail.c_str()), valid_auth_fail.size(), BufferAllocated::GROW}; + BufferAllocated valid_auth_fail_newline_end_buf{reinterpret_cast(valid_auth_fail_newline_end.c_str()), valid_auth_fail_newline_end.size(), BufferAllocated::GROW}; + BufferAllocated invalid_auth_fail_buf{reinterpret_cast(invalid_auth_fail.c_str()), invalid_auth_fail.size(), BufferAllocated::GROW}; + BufferAllocated lot_of_whitespace_buf{reinterpret_cast(lot_of_whitespace.c_str()), lot_of_whitespace.size(), BufferAllocated::GROW}; + BufferAllocated only_whitespace_buf{reinterpret_cast(only_whitespace.c_str()), only_whitespace.size(), BufferAllocated::GROW}; + BufferAllocated empty_buf{reinterpret_cast(empty.c_str()), empty.size(), BufferAllocated::GROW}; + + auto msg = ProtoContext::read_control_string(valid_auth_fail_buf); + EXPECT_EQ(msg, valid_auth_fail); + EXPECT_TRUE(Unicode::is_valid_utf8(msg, Unicode::UTF8_NO_CTRL)); + + auto msg2 = ProtoContext::read_control_string(valid_auth_fail_newline_end_buf); + EXPECT_EQ(msg2, valid_auth_fail); + EXPECT_TRUE(Unicode::is_valid_utf8(msg2, Unicode::UTF8_NO_CTRL)); + + auto msg3 = ProtoContext::read_control_string(invalid_auth_fail_buf); + EXPECT_EQ(msg3, "AUTH_FAILED: go\n away"); + EXPECT_FALSE(Unicode::is_valid_utf8(msg3, Unicode::UTF8_NO_CTRL)); + + auto msg4 = ProtoContext::read_control_string(lot_of_whitespace_buf); + EXPECT_EQ(msg4, "AUTH_FAILED: a lot of white space"); + EXPECT_TRUE(Unicode::is_valid_utf8(msg4, Unicode::UTF8_NO_CTRL)); + + auto msg5 = ProtoContext::read_control_string(only_whitespace_buf); + EXPECT_EQ(msg5, ""); + EXPECT_TRUE(Unicode::is_valid_utf8(msg5, Unicode::UTF8_NO_CTRL)); + + auto msg6 = ProtoContext::read_control_string(empty_buf); + EXPECT_EQ(msg6, ""); + EXPECT_TRUE(Unicode::is_valid_utf8(msg5, Unicode::UTF8_NO_CTRL)); +} \ No newline at end of file