From 3b46b64d13f720ab941e244994971d51af64751b Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Mon, 31 Jul 2023 17:28:32 +0200 Subject: [PATCH 01/13] Do not enforce DH parameters in TLS server mode As also explained in OpenVPN 2.x commit bd9aa06feb4, Diffie Hellman key exchanges can be optionally be disabled and OpenSSL will then use only ECDH instead. Signed-off-by: Arne Schwabe --- openvpn/mbedtls/ssl/sslctx.hpp | 2 +- openvpn/openssl/ssl/sslctx.hpp | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/openvpn/mbedtls/ssl/sslctx.hpp b/openvpn/mbedtls/ssl/sslctx.hpp index d65ed2eb..275e484d 100644 --- a/openvpn/mbedtls/ssl/sslctx.hpp +++ b/openvpn/mbedtls/ssl/sslctx.hpp @@ -533,7 +533,7 @@ class MbedTLSContext : public SSLFactoryAPI } // DH - if (mode.is_server()) + if (mode.is_server() && opt.exists("dh")) { const std::string &dh_txt = opt.get("dh", 1, Option::MULTILINE); load_dh(dh_txt); diff --git a/openvpn/openssl/ssl/sslctx.hpp b/openvpn/openssl/ssl/sslctx.hpp index f77f0147..804ed393 100644 --- a/openvpn/openssl/ssl/sslctx.hpp +++ b/openvpn/openssl/ssl/sslctx.hpp @@ -434,7 +434,7 @@ class OpenSSLContext : public SSLFactoryAPI } // DH - if (mode.is_server()) + if (mode.is_server() && opt.exists("dh")) { const std::string &dh_txt = opt.get("dh", 1, Option::MULTILINE); load_dh(dh_txt); @@ -1194,15 +1194,16 @@ class OpenSSLContext : public SSLFactoryAPI throw OpenSSLException("OpenSSLContext: SSL_CTX_new_ex failed for server method"); // Set DH object - if (!config->dh.defined()) - OPENVPN_THROW(ssl_context_error, "OpenSSLContext: DH not defined"); + if (config->dh.defined()) + { #if OPENSSL_VERSION_NUMBER >= 0x30000000L - if (!SSL_CTX_set0_tmp_dh_pkey(ctx.get(), config->dh.obj_release())) - throw OpenSSLException("OpenSSLContext: SSL_CTX_set0_tmp_dh_pkey failed"); + if (!SSL_CTX_set0_tmp_dh_pkey(ctx.get(), config->dh.obj_release())) + throw OpenSSLException("OpenSSLContext: SSL_CTX_set0_tmp_dh_pkey failed"); #else - if (!SSL_CTX_set_tmp_dh(ctx.get(), config->dh.obj())) - throw OpenSSLException("OpenSSLContext: SSL_CTX_set_tmp_dh failed"); + if (!SSL_CTX_set_tmp_dh(ctx.get(), config->dh.obj())) + throw OpenSSLException("OpenSSLContext: SSL_CTX_set_tmp_dh failed"); #endif + } if (config->flags & SSLConst::SERVER_TO_SERVER) SSL_CTX_set_purpose(ctx.get(), X509_PURPOSE_SSL_SERVER); From 53614a0cce7775ba0ae4a43887ee03aa2fa098cc Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Mon, 31 Jul 2023 12:43:44 +0200 Subject: [PATCH 02/13] Properly implement OpenVPN3 checking of --client mode Earlier implementations just assumed that --client mode is always present in the config, which lead to config behaving different in OpenVPN 2.x and 3.x. This creates hard to debug corner cases. Additionally OpenVPN 3.x was not parsing the tls-client and pull options. This lead to OpenVPN 3.x erroring on a perfectly legal config with --pull in it. Note the original patch was by Merten Fermont but his patch got mangled in the email and when I started to apply it manually I instead wrote my own version of it since we need unit tests anyway. --- openvpn/client/cliopt.hpp | 6 ++-- openvpn/client/cliopthelper.hpp | 2 +- test/unittests/test_cliopt.cpp | 54 ++++++++++++++++++++++++++++----- 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index f7be44a8..ddddf82c 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -595,6 +595,9 @@ class ClientOptions : public RC if (opt.exists("fragment")) throw option_error("sorry, 'fragment' directive is not supported, nor is connecting to a server that uses 'fragment' directive"); + if (!opt.exists("client")) + throw option_error("Neither 'client' nor both 'tls-client' and 'pull' options declared. OpenVPN3 client only supports --client mode."); + // Only p2p mode accept if (opt.exists("mode")) { @@ -801,7 +804,6 @@ class ClientOptions : public RC "replay-persist", /* Makes little sense in TLS mode */ "script-security", "sndbuf", - "tls-client", /* Always enabled */ "tmp-dir", "tun-ipv6", /* ignored in v2 as well */ "txqueuelen", /* so platforms evaluate that in tun, some do not, do not warn about that */ @@ -1180,8 +1182,6 @@ class ClientOptions : public RC cc->set_tls_cert_profile_override(config.tls_cert_profile_override); cc->set_tls_cipher_list(config.tls_cipher_list); cc->set_tls_ciphersuite_list(config.tls_ciphersuite_list); - if (!cc->get_mode().is_client()) - throw option_error("only client configuration supported"); // client ProtoContext config Client::ProtoConfig::Ptr cp(new Client::ProtoConfig()); diff --git a/openvpn/client/cliopthelper.hpp b/openvpn/client/cliopthelper.hpp index 95aa6664..20384a9e 100644 --- a/openvpn/client/cliopthelper.hpp +++ b/openvpn/client/cliopthelper.hpp @@ -367,7 +367,7 @@ class ParseClientConfig bool added = false; // client - if (!options.exists("client")) + if (options.exists("tls-client") && options.exists("pull")) { Option opt; opt.push_back("client"); diff --git a/test/unittests/test_cliopt.cpp b/test/unittests/test_cliopt.cpp index 9ba72609..ab842067 100644 --- a/test/unittests/test_cliopt.cpp +++ b/test/unittests/test_cliopt.cpp @@ -27,13 +27,16 @@ std::string dummysecp256key = "-----BEGIN PRIVATE KEY-----\n" "3oP+eSyQewIqu8XJECJhmt8NoXqNNYRUF0P+Jit8LC2a+2WZeyAwuIYT\n" "-----END PRIVATE KEY-----\n"; -std::string minimalConfig = "remote wooden.box\n" - "\n" - + dummysecp256cert + "\n" - + "\n" - + dummysecp256cert + "\n" - + "\n" + dummysecp256key - + "\n"; +std::string certconfig = "\n" + + dummysecp256cert + "\n" + + "\n" + + dummysecp256cert + "\n" + + "\n" + dummysecp256key + + "\n"; + +std::string minimalConfig = certconfig + "\n" + + "client\n" + "remote wooden.box\n"; void load_client_config(const std::string &config_content) { @@ -178,3 +181,40 @@ TEST(config, dco_compatibility) "option_error: dco_compatibility: config/options are not compatible with dco"); } } + +TEST(config, client_missing_in_config) +{ + std::string configNoClient = certconfig + "\nremote 1.2.3.4\n"; + OVPN_EXPECT_THROW( + load_client_config(configNoClient), + option_error, + "option_error: Neither 'client' nor both 'tls-client' and 'pull' options declared. OpenVPN3 client only supports --client mode."); +} + +TEST(config, pull_and_tlsclient_in_config) +{ + std::string configNoClient = certconfig + "\nremote 1.2.3.4\ntls-client\npull\n"; + /* Should not trigger an error, even without --client in place */ + load_client_config(configNoClient); +} + +TEST(config, pull_and_client_and_tlsclient_in_config) +{ + std::string configNoClient = certconfig + "\nremote 1.2.3.4\ntls-client\npull\nclient\n"; + /* Should not trigger an error. Redundant options are no problem */ + load_client_config(configNoClient); +} + +TEST(config, onlypullortlsclient) +{ + std::array options{"tls-client", "pull"}; + + for (const auto &opt : options) + { + std::string configNoClient = certconfig + "\nremote 1.2.3.4\n" + opt + "\n"; + OVPN_EXPECT_THROW( + load_client_config(configNoClient), + option_error, + "option_error: Neither 'client' nor both 'tls-client' and 'pull' options declared. OpenVPN3 client only supports --client mode."); + } +} \ No newline at end of file From 85b92afe96ee1ed3d8fd5c153c8dda07c00b79bd Mon Sep 17 00:00:00 2001 From: Lev Stipakov Date: Fri, 28 Jul 2023 14:09:26 +0300 Subject: [PATCH 03/13] win/client/tunsetup.hpp: fix IPv4 redirect-gw with IPv6 remote redirect-gw is implemented by changing default route to a GW provided by VPN. For IPv4 before doing that we add a bypass route to a remote. This is needed only when remote is not on local network. The check "is remote on local network" has a wrong assumtion that remote is IPv4. This is obviously not always the case since remote could be IPv6. In this case if we want to redirect IPv4 traffic an exception is thrown inside BestGateway class while trying to convert IPv6 address to IPv4. Fix by specifying correct address family based on remote's "ipv6" flag. Later we add bypass route only if remote is IPv4. Fixes OVPN3-1004. Signed-off-by: Lev Stipakov --- openvpn/tun/win/client/tunsetup.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openvpn/tun/win/client/tunsetup.hpp b/openvpn/tun/win/client/tunsetup.hpp index ef0f4848..e3543a02 100644 --- a/openvpn/tun/win/client/tunsetup.hpp +++ b/openvpn/tun/win/client/tunsetup.hpp @@ -549,7 +549,8 @@ class Setup : public SetupBase if (pull.reroute_gw.ipv4) { // get default gateway - const Util::BestGateway gw{AF_INET, pull.remote_address.address, tap.index}; + ADDRESS_FAMILY af = pull.remote_address.ipv6 ? AF_INET6 : AF_INET; + const Util::BestGateway gw{af, pull.remote_address.address, tap.index}; if (!gw.local_route()) { From df5f6d58102f5f5e03f5345ae0fbf3146fea3182 Mon Sep 17 00:00:00 2001 From: Frank Lichtenheld Date: Mon, 31 Jul 2023 17:03:16 +0200 Subject: [PATCH 04/13] mingw/build: Fix xxHash build Adapt to vcpkg changes. Signed-off-by: Frank Lichtenheld --- scripts/mingw/build | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/mingw/build b/scripts/mingw/build index 248ce4a8..232ae99d 100755 --- a/scripts/mingw/build +++ b/scripts/mingw/build @@ -55,9 +55,9 @@ download_deps() rm -rf xxHash git clone https://github.com/Cyan4973/xxHash.git - portfile_url=https://raw.githubusercontent.com/microsoft/vcpkg/master/ports/xxhash/portfile.cmake - gitref=$(wget -q -O- "$portfile_url" | grep -oP '\bREF\s+\S+' | cut -d' ' -f2) - git -C xxHash checkout "${gitref}" + portfile_url=https://raw.githubusercontent.com/microsoft/vcpkg/master/ports/xxhash/vcpkg.json + gitref=$(wget -q -O- "$portfile_url" | grep -oP '\"version": \"\S+' | cut -d' ' -f2 | tr -d "\",") + git -C xxHash checkout "v${gitref}" popd } From b755783a13257237f85d633fe2ae60e4118415a0 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Wed, 9 Aug 2023 18:07:32 +0200 Subject: [PATCH 05/13] Fix reading MAC address on macOS The confusing overlapping structs and memory accesses with the struct lead to use missing a few bytes from being copied. Fix this by copying from the correct struct. Signed-off-by: Arne Schwabe --- openvpn/tun/mac/gwv4.hpp | 41 +++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/openvpn/tun/mac/gwv4.hpp b/openvpn/tun/mac/gwv4.hpp index 6508f91f..5cd1ffe0 100644 --- a/openvpn/tun/mac/gwv4.hpp +++ b/openvpn/tun/mac/gwv4.hpp @@ -201,7 +201,7 @@ class MacGatewayInfoV4 struct ifconf ifc; const int bufsize = 4096; - std::unique_ptr buffer(new char[bufsize]); + const std::unique_ptr buffer(new char[bufsize]); std::memset(buffer.get(), 0, bufsize); sockfd.reset(socket(AF_INET, SOCK_DGRAM, 0)); if (!sockfd.defined()) @@ -218,26 +218,41 @@ class MacGatewayInfoV4 { ifreq ifr = {}; std::memcpy(&ifr, cp, sizeof(ifr)); - const size_t len = sizeof(ifr.ifr_name) + std::max(sizeof(ifr.ifr_addr), size_t(ifr.ifr_addr.sa_len)); + const size_t len = sizeof(ifr.ifr_name) + std::max(sizeof(ifr.ifr_addr), size_t{ifr.ifr_addr.sa_len}); + if (!ifr.ifr_addr.sa_family) break; if (!::strncmp(ifr.ifr_name, iface_, IFNAMSIZ)) { if (ifr.ifr_addr.sa_family == AF_LINK) { - /* This is a broken member access. struct sockaddr_dl has - * 20 bytes while if_addr has only 16 bytes. But sockaddr_dl - * has 12 bytes space for the hw address and Ethernet only uses - * 6 bytes. So the last 4 that are truncated can be ignored here + /* This is a confusing member access on multiple levels. * - * So we use a memcpy here to avoid the warnings with ASAN that we - * are doing a very nasty cast here + * struct sockaddr_dl is 20 bytes in size and has + * 12 bytes space for the hw address (6 bytes) + * and Ethernet interface name (max 16 bytes) + * + * So if the interface name is more than 6 byte, it + * extends beyond the struct. + * + * This struct is embedded into ifreq that has + * 16 bytes for a sockaddr and also expects this + * struct to potentially extend beyond the bounds of + * the struct. + * + * Since we only copied 32 bytes from cp to ifr but sdl + * might extend after ifr's end, we need to copy from + * cp directly to avoid missing out on extra bytes + * behind the struct */ - static_assert(sizeof(ifr.ifr_addr) >= 12, "size of if_addr too small to contain MAC"); - static_assert(sizeof(sockaddr_dl) >= sizeof(ifr.ifr_addr), "dest struct needs to be larger than source struct"); - sockaddr_dl sdl{}; - std::memcpy(&sdl, &ifr.ifr_addr, sizeof(ifr.ifr_addr)); - hwaddr_.reset((const unsigned char *)LLADDR(&sdl)); + const size_t sock_dl_len = std::max(sizeof(sockaddr_dl), size_t{ifr.ifr_addr.sa_len}); + + const std::unique_ptr sock_dl_buf(new char[sock_dl_len]); + std::memcpy(sock_dl_buf.get(), cp + offsetof(struct ifreq, ifr_addr), sock_dl_len); + + const struct sockaddr_dl *sockaddr_dl = reinterpret_cast(sock_dl_buf.get()); + + hwaddr_.reset(reinterpret_cast(LLADDR(sockaddr_dl))); flags_ |= HWADDR_DEFINED; } } From 1fa0e9589f367fb72629aa1bf7513310150c3eb1 Mon Sep 17 00:00:00 2001 From: Frank Lichtenheld Date: Wed, 23 Aug 2023 11:56:01 +0200 Subject: [PATCH 06/13] deps: update mbedTLS to 2.28.4 We're specifically interested in the fix for the unit tests. ("Update test data to avoid failures of unit tests after 2023-08-07") Signed-off-by: Frank Lichtenheld --- deps/lib-versions | 4 +-- .../0001-relax-x509-date-format-check.patch | 35 ++++++++----------- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/deps/lib-versions b/deps/lib-versions index 1b2f9b79..77aabcfa 100644 --- a/deps/lib-versions +++ b/deps/lib-versions @@ -4,8 +4,8 @@ export ASIO_CSUM=cbcaaba0f66722787b1a7c33afe1befb3a012b5af3ad7da7ff0f6b8c9b7a8a5 export LZ4_VERSION=lz4-1.8.3 export LZ4_CSUM=33af5936ac06536805f9745e0b6d61da606a1f8b4cc5c04dd3cbaca3b9b4fc43 -export MBEDTLS_VERSION=mbedtls-2.28.2 -export MBEDTLS_CSUM=bc55232bf71fd66045122ba9050a29ea7cb2e8f99b064a9e6334a82f715881a0 +export MBEDTLS_VERSION=mbedtls-2.28.4 +export MBEDTLS_CSUM=578c4dcd15bbff3f5cd56aa07cd4f850fc733634e3d5947be4f7157d5bfd81ac export JSONCPP_VERSION=1.8.4 export JSONCPP_CSUM=c49deac9e0933bcb7044f08516861a2d560988540b23de2ac1ad443b219afdb6 diff --git a/deps/mbedtls/patches/0001-relax-x509-date-format-check.patch b/deps/mbedtls/patches/0001-relax-x509-date-format-check.patch index d5406e90..c4ebde64 100644 --- a/deps/mbedtls/patches/0001-relax-x509-date-format-check.patch +++ b/deps/mbedtls/patches/0001-relax-x509-date-format-check.patch @@ -12,39 +12,34 @@ diff --git a/library/x509.c b/library/x509.c index 264c7fb0c..9372bcb92 100644 --- a/library/x509.c +++ b/library/x509.c -@@ -556,13 +556,20 @@ static int x509_parse_time( unsigned char **p, size_t len, size_t yearlen, +@@ -624,11 +624,16 @@ static int x509_parse_time(unsigned char /* * Parse seconds if present */ -- if ( len >= 2 ) -+ if ( len >= 2 && **p >= '0' && **p <= '9' ) - { - CHECK( x509_parse_int( p, 2, &tm->sec ) ); +- if (len >= 2) { ++ if (len >= 2 && **p >= '0' && **p <= '9') { + CHECK(x509_parse_int(p, 2, &tm->sec)); len -= 2; - } - else -+ { + } else { +#if defined(MBEDTLS_RELAXED_X509_DATE) -+ /* if relaxed mode, allow seconds to be absent */ -+ tm->sec = 0; ++ /* if relaxed mode, allow seconds to be absent */ ++ tm->sec = 0; +#else - return ( MBEDTLS_ERR_X509_INVALID_DATE ); + return MBEDTLS_ERR_X509_INVALID_DATE; +#endif -+ } + } /* - * Parse trailing 'Z' if present -@@ -572,6 +579,15 @@ static int x509_parse_time( unsigned char **p, size_t len, size_t yearlen, +@@ -638,6 +643,14 @@ static int x509_parse_time(unsigned char (*p)++; len--; } +#if defined(MBEDTLS_RELAXED_X509_DATE) -+ else if ( len == 5 && **p == '+' ) -+ { -+ int tz; /* throwaway timezone */ -+ (*p)++; -+ CHECK( x509_parse_int( p, 4, &tz ) ); -+ return 0; ++ else if ( len == 5 && **p == '+' ) { ++ int tz; /* throwaway timezone */ ++ (*p)++; ++ CHECK( x509_parse_int( p, 4, &tz ) ); ++ return 0; + } +#endif From 0a690f5dff6516cbc3a66c3b023b021e5fefe579 Mon Sep 17 00:00:00 2001 From: David Sommerseth Date: Wed, 23 Aug 2023 15:54:58 +0200 Subject: [PATCH 07/13] ssl/proto: Clarify sending peer-info debug details Making it more explicit that the listed items is data being sent to the server. Signed-off-by: David Sommerseth --- openvpn/ssl/proto.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openvpn/ssl/proto.hpp b/openvpn/ssl/proto.hpp index ae11583d..77b005a3 100644 --- a/openvpn/ssl/proto.hpp +++ b/openvpn/ssl/proto.hpp @@ -997,8 +997,8 @@ class ProtoContext if (relay_mode) out << "IV_RELAY=1\n"; const std::string ret = out.str(); - OPENVPN_LOG_PROTO("Peer Info:" << std::endl - << ret); + OPENVPN_LOG_PROTO("Sending Peer Info:" << std::endl + << ret); return ret; } From 7fc0b701a119bbb4f9d963086faeed405ea5acde Mon Sep 17 00:00:00 2001 From: Lev Stipakov Date: Thu, 24 Aug 2023 15:48:59 +0300 Subject: [PATCH 08/13] Parse meta options from content_list At the moment meta options are parsed only from content. This doesn't work well with iOS where config is imported via content_list. The config might contain meta options, which currently won't be recognized as meta and connection won't be established due to "unknown option" error. This adds meta options parsing to content_list. Signed-off-by: Lev Stipakov --- openvpn/client/cliopthelper.hpp | 2 +- openvpn/common/options.hpp | 21 +++++++++++++++++---- test/unittests/test_cliopt.cpp | 26 +++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/openvpn/client/cliopthelper.hpp b/openvpn/client/cliopthelper.hpp index 20384a9e..5bb3050b 100644 --- a/openvpn/client/cliopthelper.hpp +++ b/openvpn/client/cliopthelper.hpp @@ -358,7 +358,7 @@ class ParseClientConfig if (content_list) { content_list->preprocess(); - options.parse_from_key_value_list(*content_list, &limits); + options.parse_from_key_value_list(*content_list, "OVPN_ACCESS_SERVER", &limits); } process_setenv_opt(options); options.update_map(); diff --git a/openvpn/common/options.hpp b/openvpn/common/options.hpp index d594c41a..0fea4483 100644 --- a/openvpn/common/options.hpp +++ b/openvpn/common/options.hpp @@ -710,12 +710,22 @@ class OptionList : public std::vector