From 11ec627a930a5edf9820be9f80ed393430f220a8 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Thu, 17 Aug 2023 09:30:15 +0200 Subject: [PATCH 01/12] Allow ovpn::string::join to work also with other contains than std::vector Signed-off-by: Arne Schwabe --- openvpn/common/string.hpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openvpn/common/string.hpp b/openvpn/common/string.hpp index 01e9cfe56..15a98a695 100644 --- a/openvpn/common/string.hpp +++ b/openvpn/common/string.hpp @@ -525,10 +525,13 @@ inline std::vector split(const std::string &str, return ret; } -inline std::string join(const std::vector &strings, +template +inline std::string join(const T &strings, const std::string &delim, const bool tail = false) { + /* ensure we have a container with strings as values */ + static_assert(std::is_same_v); std::string ret; bool first = true; for (const auto &s : strings) From 181dafdb0de68e44640cd11dd66efde96a02cf94 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Fri, 13 Oct 2023 12:16:50 +0200 Subject: [PATCH 02/12] Add unsupported option name in error message Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 8 +++++++- test/unittests/test_cliopt.cpp | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index ddddf82c7..2efdcf57f 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -30,6 +30,7 @@ #include #include +#include #include @@ -918,6 +919,9 @@ class ClientOptions : public RC void showOptionsByFunction(const OptionList &opt, T func, const std::string &message, bool fatal) { bool messageShown = false; + /* Remember options that caused a fatal error */ + std::set unsupported_option_in_config; + for (size_t i = 0; i < opt.size(); ++i) { auto &o = opt[i]; @@ -929,13 +933,15 @@ class ClientOptions : public RC messageShown = true; } o.touch(); + unsupported_option_in_config.emplace(o.get(0, 0)); OPENVPN_LOG_NTNL(std::to_string(i) << ' ' << o.render(Option::RENDER_BRACKET | Option::RENDER_TRUNC_64) << std::endl); } } if (fatal && messageShown) { - throw option_error("sorry, unsupported options present in configuration: " + message); + throw option_error("sorry, unsupported options present in configuration: " + message + + " (" + string::join(unsupported_option_in_config, ",") + ")"); } } diff --git a/test/unittests/test_cliopt.cpp b/test/unittests/test_cliopt.cpp index 25a4a840c..2bdf8b480 100644 --- a/test/unittests/test_cliopt.cpp +++ b/test/unittests/test_cliopt.cpp @@ -182,6 +182,42 @@ TEST(config, dco_compatibility) } } + +TEST(config, server_options_present_in_error_msg) +{ + std::vector server_options = {"server 10.0.0.0 255.255.255.0", + "push \"foo bar\""}; + + for (auto &option : server_options) + { + auto optname = option.substr(0, option.find(' ')); + auto expected_error_string = "Server only option (" + optname; + + OVPN_EXPECT_THROW( + load_client_config(minimalConfig + option), + option_error, + expected_error_string); + } +} + +TEST(config, unknown_options_present_in_error_msg) +{ + std::vector server_options = {"make-a-lot-of-noise", "water-the-plants"}; + + for (auto &option : server_options) + { + auto optname = option.substr(0, option.find(' ')); + auto expected_error_string = "UNKNOWN/UNSUPPORTED OPTIONS (" + optname; + + OVPN_EXPECT_THROW( + load_client_config(minimalConfig + option), + option_error, + expected_error_string); + } +} + + + TEST(config, client_missing_in_config) { std::string configNoClient = certconfig + "\nremote 1.2.3.4\n"; From 153c80cfefbbfe7e823fd80ecd93e45fafeda362 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Fri, 13 Oct 2023 12:21:01 +0200 Subject: [PATCH 03/12] Add route-delay to ignored but warned options Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index 2efdcf57f..2389c9349 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -647,6 +647,7 @@ class ClientOptions : public RC "replay-window", "resolv-retry", "route-method", /* Windows specific fine tuning option */ + "route-delay", "show-net-up", "socket-flags", "suppress-timestamps", /* harmless to ignore */ From bf5df01ac8cf904cab202037a1de53e2b95761e3 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Mon, 16 Oct 2023 18:11:46 +0200 Subject: [PATCH 04/12] Add no-iv to the removed options that we cannot support anymore Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index 2389c9349..900110001 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -793,7 +793,7 @@ class ClientOptions : public RC /* Deprecated/throwing error in OpenVPN 2.x already: */ std::unordered_set settings_removedOptions = { - "mtu-dynamic", "no-replay", "no-name-remapping", "compat-names", "ncp-disable"}; + "mtu-dynamic", "no-replay", "no-name-remapping", "compat-names", "ncp-disable", "no-iv"}; std::unordered_set settings_ignoreSilently = { "ecdh-curve", /* Deprecated in v2, not needed with modern OpenSSL */ From 0be01b88594dfeefe182110036c673ca8772ac74 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Mon, 16 Oct 2023 18:14:53 +0200 Subject: [PATCH 05/12] Do not warn twice about options in settings_feature_not_implemented_warn Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index 900110001..2cde0fa20 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -880,7 +880,6 @@ class ClientOptions : public RC showUnusedOptionsByList(opt, settings_feature_not_implemented_warn, "Feature not implemented (option ignored)", false); showUnusedOptionsByList(opt, settings_pushonlyoptions, "Option allowed only to be pushed by the server", true); - showUnusedOptionsByList(opt, settings_feature_not_implemented_warn, "feature not implemented/available", false); showUnusedOptionsByList(opt, settings_script_plugin_feature, "Ignored (no script/plugin support)", false); showUnusedOptionsByList(opt, ignore_unknown_option_list, "Ignored by option 'ignore-unknown-option'", false); showUnusedOptionsByList(opt, settings_ignoreWithWarning, "Unsupported option (ignored)", false); From 89d382853e5f7569e91bb54d05c0b587ecfd34a0 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Mon, 16 Oct 2023 18:30:34 +0200 Subject: [PATCH 06/12] Allow key-method 2 in OpenVPN3 client configs Also this is a very rare option to be used today as it was for compatibility with OpenVPN 1.x we should still not error out when it is present. Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 14 ++++++++++++-- test/unittests/test_cliopt.cpp | 10 ++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index 2cde0fa20..6213cb5cb 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -603,11 +603,21 @@ class ClientOptions : public RC if (opt.exists("mode")) { auto mode = opt.get("mode"); - if (mode.size() != 1 || mode.get(1, 128) != "p2p") + if (mode.size() != 2 || mode.get(1, 128) != "p2p") { throw option_error("Only 'mode p2p' supported"); } } + + // key-method 2 is the only thing that 2.5+ and 3.x support + if (opt.exists("key-method")) + { + auto keymethod = opt.get("key-method"); + if (keymethod.size() != 2 || keymethod.get(1, 128) != "2") + { + throw option_error("Only 'key-method 2' is supported: " + keymethod.get(1, 128)); + } + } } std::unordered_set settings_ignoreWithWarning = { @@ -723,7 +733,7 @@ class ClientOptions : public RC "status-version", "syslog", "tls-server", /* No p2p mode in v3 */ - "tun-mtu-extra", /*(only really used in tap in OpenVPN 2.x)*/ + "tun-mtu-extra", /* (only really used in tap in OpenVPN 2.x)*/ "verify-hash", "win-sys", "writepid", diff --git a/test/unittests/test_cliopt.cpp b/test/unittests/test_cliopt.cpp index 2bdf8b480..a86aeecc2 100644 --- a/test/unittests/test_cliopt.cpp +++ b/test/unittests/test_cliopt.cpp @@ -76,11 +76,21 @@ TEST(config, parse_missing_option) TEST(config, parse_forbidden_option) { + OVPN_EXPECT_THROW( + load_client_config(minimalConfig + "mode"), + option_error, + "Only 'mode p2p' supported"); + OVPN_EXPECT_THROW( load_client_config(minimalConfig + "mode server"), option_error, "Only 'mode p2p' supported"); + OVPN_EXPECT_THROW( + load_client_config(minimalConfig + "key-method 1"), + option_error, + "Only 'key-method 2' is supported"); + OVPN_EXPECT_THROW( load_client_config(minimalConfig + "fragment"), option_error, From 66da32a859f8df0268827e74dabef5dde5ef18d9 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Tue, 24 Oct 2023 19:54:01 +0200 Subject: [PATCH 07/12] Add block-outside-dns and tun-mtu-extra to ignore and warn list Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index 6213cb5cb..55b213b57 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -625,6 +625,7 @@ class ClientOptions : public RC "allow-recursive-routing", "auth-nocache", "auth-retry", + "block-outside-dns", /* Core will decide on its own when to block outside dns, so this is not 100% identical in behaviour, so still warn */ "compat-mode", "connect-retry", "connect-retry-max", @@ -663,6 +664,7 @@ class ClientOptions : public RC "suppress-timestamps", /* harmless to ignore */ "tcp-nodelay", "tls-version-max", /* We don't allow restricting max version */ + "tun-mtu-extra", /* (only really used in tap in OpenVPN 2.x)*/ "udp-mtu", /* Alias for link-mtu */ "user", }; @@ -732,8 +734,7 @@ class ClientOptions : public RC "status", "status-version", "syslog", - "tls-server", /* No p2p mode in v3 */ - "tun-mtu-extra", /* (only really used in tap in OpenVPN 2.x)*/ + "tls-server", /* No p2p mode in v3 */ "verify-hash", "win-sys", "writepid", From 384c74975454edad8d51197ad6fa057d3595e207 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Wed, 25 Oct 2023 13:31:10 +0200 Subject: [PATCH 08/12] Add ifconfig, topology and route-gateway to push only options Signed-off-by: Arne Schwabe --- openvpn/client/cliopt.hpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index 55b213b57..d452da167 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -769,7 +769,10 @@ class ClientOptions : public RC "key-derivation", "peer-id", "protocol-flags", - }; + "ifconfig", + "ifconfig-ipv6", + "topology", + "route-gateway"}; /* Features related to scripts/plugins */ std::unordered_set settings_script_plugin_feature = { From 6af5b505f1e0d48ef894be150b623ea4d8738a78 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Wed, 29 Nov 2023 16:05:21 +0100 Subject: [PATCH 09/12] Fix --compress initialising the wrong stub method The option compress is extremely seldom used since there virtually no sense in using it as all clients that support the compress option also support pushing compression, so adding a stub only compression method by default in the configuration does not give any benefit, only downsides. When compress is in the config *and* the server never pushes any compression option (even "push compress" is fine), we initialise "comp-lzo no" instead. And comp-lzo and compress are different compression stub methods (byte swap vs no byte swap) that are incompatible. compress without argument in config is extremely seldom used. Signed-off-by: Arne Schwabe --- openvpn/compress/compress.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openvpn/compress/compress.hpp b/openvpn/compress/compress.hpp index 0353a04fa..9830a71b7 100644 --- a/openvpn/compress/compress.hpp +++ b/openvpn/compress/compress.hpp @@ -221,10 +221,10 @@ class CompressContext { case NONE: return new CompressNull(frame, stats); - case ANY: case ANY_LZO: case LZO_STUB: return new CompressStub(frame, stats, false); + case ANY: case COMP_STUB: return new CompressStub(frame, stats, true); case COMP_STUBv2: From afdfe1bb3f4c54e879429950b95e5c0e29b1a77d Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Tue, 21 Nov 2023 12:29:37 +0100 Subject: [PATCH 10/12] Fix accessing a null pointer when PKCS7 is invalid If we get a valid but almost empty PKCS7 structure we otherwise try to access invalid fields. CVE: CVE-2023-6247 Reported-by: Bahaa Naamneh Signed-off-by: Arne Schwabe --- openvpn/openssl/sign/pkcs7verify.hpp | 10 ++- openvpn/openssl/sign/verify.hpp | 2 + test/unittests/CMakeLists.txt | 1 + test/unittests/test_openssl_misc.cpp | 123 +++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 4 deletions(-) create mode 100644 test/unittests/test_openssl_misc.cpp diff --git a/openvpn/openssl/sign/pkcs7verify.hpp b/openvpn/openssl/sign/pkcs7verify.hpp index 12dd93425..b324c8579 100644 --- a/openvpn/openssl/sign/pkcs7verify.hpp +++ b/openvpn/openssl/sign/pkcs7verify.hpp @@ -65,7 +65,7 @@ inline void verify_pkcs7(const std::list &certs, /* get signature */ in = BIO_new_mem_buf(sig.c_str(), sig.length()); - p7 = PEM_read_bio_PKCS7(in, NULL, NULL, NULL); + p7 = PEM_read_bio_PKCS7(in, nullptr, nullptr, nullptr); if (!p7) throw OpenSSLException("OpenSSLSign::verify_pkcs7: failed to parse pkcs7 signature"); BIO_free(in); @@ -75,11 +75,13 @@ inline void verify_pkcs7(const std::list &certs, in = BIO_new_mem_buf(data.c_str(), data.length()); /* OpenSSL 1.0.2e and higher no longer allows calling PKCS7_verify - with both data and content. Empty out the content. */ - p7->d.sign->contents->d.ptr = 0; + with both data and content. Empty out the content if present. + Just calling PKCS7_set_detached call lead to a null pointer access */ + if (!PKCS7_is_detached(p7)) + PKCS7_set_detached(p7, 1); /* do the verify */ - if (PKCS7_verify(p7, x509_stack, NULL, in, NULL, PKCS7_NOVERIFY) != 1) + if (PKCS7_verify(p7, x509_stack, nullptr, in, nullptr, PKCS7_NOVERIFY) != 1) throw OpenSSLException("OpenSSLSign::verify_pkcs7: verification failed"); } } // namespace OpenSSLSign diff --git a/openvpn/openssl/sign/verify.hpp b/openvpn/openssl/sign/verify.hpp index f1d276732..e35016419 100644 --- a/openvpn/openssl/sign/verify.hpp +++ b/openvpn/openssl/sign/verify.hpp @@ -25,12 +25,14 @@ #define OPENVPN_OPENSSL_SIGN_VERIFY_H #include +#include #include #include #include #include +#include #include #include diff --git a/test/unittests/CMakeLists.txt b/test/unittests/CMakeLists.txt index e0e9eeb08..95e94f080 100644 --- a/test/unittests/CMakeLists.txt +++ b/test/unittests/CMakeLists.txt @@ -99,6 +99,7 @@ else () test_openssl_x509certinfo.cpp test_openssl_authcert.cpp test_opensslpki.cpp + test_openssl_misc.cpp test_session_id.cpp ) endif () diff --git a/test/unittests/test_openssl_misc.cpp b/test/unittests/test_openssl_misc.cpp new file mode 100644 index 000000000..ed93627ab --- /dev/null +++ b/test/unittests/test_openssl_misc.cpp @@ -0,0 +1,123 @@ +#include "test_common.h" +#include + +#include +#include +#include + +using namespace openvpn; + +constexpr const char *broken_pkcs7 = "-----BEGIN PKCS7-----\n" + "MAsGCSqGSIb3DQEHAg==\n" + "-----END PKCS7-----\n"; + +constexpr const char *unit_test_ca = "-----BEGIN CERTIFICATE-----\n" + "MIIBuTCCAUCgAwIBAgIUTLtjSBzx53qZRvZ6Ur7D9kgoOHkwCgYIKoZIzj0EAwIw\n" + "EzERMA8GA1UEAwwIdW5pdHRlc3QwIBcNMjMxMTIxMDk1NDQ3WhgPMjA3ODA4MjQw\n" + "OTU0NDdaMBMxETAPBgNVBAMMCHVuaXR0ZXN0MHYwEAYHKoZIzj0CAQYFK4EEACID\n" + "YgAEHYB2hn2xx3f4lClXDtdi36P19pMZA+kI1Dkv/Vn10vBZ/j9oa+P99T8duz/e\n" + "QlPeHpesNJO4fX8iEDj6+vMeWejOT7jAQ4MmG5EZjpcBKxCfwFooEvzu8bVujUcu\n" + "wTQEo1MwUTAdBgNVHQ4EFgQUPcgBEVXjF5vYfDsInoE3dF6UfQswHwYDVR0jBBgw\n" + "FoAUPcgBEVXjF5vYfDsInoE3dF6UfQswDwYDVR0TAQH/BAUwAwEB/zAKBggqhkjO\n" + "PQQDAgNnADBkAjBLPAGrQAyinigqiu0RomoV8TVaknVLFSq6H6A8jgvzfsFCUK1O\n" + "dvNZhFPM6idKB+oCME2JLOBANCSV8o7aJzq7SYHKwPyb1J4JFlwKe/0Jpv7oh9b1\n" + "IJbuaM9Z/VSKbrIXGg==\n" + "-----END CERTIFICATE-----\n"; + + +constexpr const char *unit_test_ca_key [[maybe_unused]] = "-----BEGIN PRIVATE KEY-----\n" + "MIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDCJ92tBE1WmpBkPwgcN\n" + "5xJ93tVilpsS5hi22V/VIGpCwKplSzTdB61TkB5RRWuQMAuhZANiAAQdgHaGfbHH\n" + "d/iUKVcO12Lfo/X2kxkD6QjUOS/9WfXS8Fn+P2hr4/31Px27P95CU94el6w0k7h9\n" + "fyIQOPr68x5Z6M5PuMBDgyYbkRmOlwErEJ/AWigS/O7xtW6NRy7BNAQ=\n" + "-----END PRIVATE KEY-----\n"; + +/* first stanza of a German children's song for Saint Martin */ +constexpr const char *laterne = "Laterne, Laterne, Sonne, Mond und Sterne,\n" + "brenne auf mein Licht,\n" + "brenne auf mein Licht,\n" + "aber du, meine Liebe Laterne, nicht.\n"; + + +// created used openssl smime -in laterne --binary -sign -signer unittest.pem -inkey unittest.pem -outform pem +constexpr const char *laterne_sig = "-----BEGIN PKCS7-----\n" + "MIIDmAYJKoZIhvcNAQcCoIIDiTCCA4UCAQExDzANBglghkgBZQMEAgEFADALBgkq\n" + "hkiG9w0BBwGgggG9MIIBuTCCAUCgAwIBAgIUTLtjSBzx53qZRvZ6Ur7D9kgoOHkw\n" + "CgYIKoZIzj0EAwIwEzERMA8GA1UEAwwIdW5pdHRlc3QwIBcNMjMxMTIxMDk1NDQ3\n" + "WhgPMjA3ODA4MjQwOTU0NDdaMBMxETAPBgNVBAMMCHVuaXR0ZXN0MHYwEAYHKoZI\n" + "zj0CAQYFK4EEACIDYgAEHYB2hn2xx3f4lClXDtdi36P19pMZA+kI1Dkv/Vn10vBZ\n" + "/j9oa+P99T8duz/eQlPeHpesNJO4fX8iEDj6+vMeWejOT7jAQ4MmG5EZjpcBKxCf\n" + "wFooEvzu8bVujUcuwTQEo1MwUTAdBgNVHQ4EFgQUPcgBEVXjF5vYfDsInoE3dF6U\n" + "fQswHwYDVR0jBBgwFoAUPcgBEVXjF5vYfDsInoE3dF6UfQswDwYDVR0TAQH/BAUw\n" + "AwEB/zAKBggqhkjOPQQDAgNnADBkAjBLPAGrQAyinigqiu0RomoV8TVaknVLFSq6\n" + "H6A8jgvzfsFCUK1OdvNZhFPM6idKB+oCME2JLOBANCSV8o7aJzq7SYHKwPyb1J4J\n" + "FlwKe/0Jpv7oh9b1IJbuaM9Z/VSKbrIXGjGCAZ8wggGbAgEBMCswEzERMA8GA1UE\n" + "AwwIdW5pdHRlc3QCFEy7Y0gc8ed6mUb2elK+w/ZIKDh5MA0GCWCGSAFlAwQCAQUA\n" + "oIHkMBgGCSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJKoZIhvcNAQkFMQ8XDTIz\n" + "MTEyMTExNTYxOFowLwYJKoZIhvcNAQkEMSIEIL6nbAP3MXDvmWwGIpts8nUoOyHn\n" + "aDA9IjR3QooF/IYvMHkGCSqGSIb3DQEJDzFsMGowCwYJYIZIAWUDBAEqMAsGCWCG\n" + "SAFlAwQBFjALBglghkgBZQMEAQIwCgYIKoZIhvcNAwcwDgYIKoZIhvcNAwICAgCA\n" + "MA0GCCqGSIb3DQMCAgFAMAcGBSsOAwIHMA0GCCqGSIb3DQMCAgEoMAoGCCqGSM49\n" + "BAMCBGcwZQIwGjRweguw3AXhfSBu4czIiOk/kdncLIAzz0S78YURt5wYlbHSnMuO\n" + "YSNyVn97Uc+UAjEA6+tj2o1i42yiF5WNMp/92QtfCV7TZE3ssiLxqst2aqlIY29H\n" + "G9j5hdY2ZRkhUCHL\n" + "-----END PKCS7-----"; + +// created used openssl smime -in laterne --binary -sign -signer unittest.pem -inkey unittest.pem -outform pem -nodetach +constexpr const char *laterne_signd = "-----BEGIN PKCS7-----\n" + "MIIEGwYJKoZIhvcNAQcCoIIEDDCCBAgCAQExDzANBglghkgBZQMEAgEFADCBjAYJ\n" + "KoZIhvcNAQcBoH8EfUxhdGVybmUsIExhdGVybmUsIFNvbm5lLCBNb25kIHVuZCBT\n" + "dGVybmUsCmJyZW5uZSBhdWYgbWVpbiBMaWNodCwKYnJlbm5lIGF1ZiBtZWluIExp\n" + "Y2h0LAphYmVyIGR1LCBtZWluZSBMaWViZSBMYXRlcm5lLCBuaWNodC4KoIIBvTCC\n" + "AbkwggFAoAMCAQICFEy7Y0gc8ed6mUb2elK+w/ZIKDh5MAoGCCqGSM49BAMCMBMx\n" + "ETAPBgNVBAMMCHVuaXR0ZXN0MCAXDTIzMTEyMTA5NTQ0N1oYDzIwNzgwODI0MDk1\n" + "NDQ3WjATMREwDwYDVQQDDAh1bml0dGVzdDB2MBAGByqGSM49AgEGBSuBBAAiA2IA\n" + "BB2AdoZ9scd3+JQpVw7XYt+j9faTGQPpCNQ5L/1Z9dLwWf4/aGvj/fU/Hbs/3kJT\n" + "3h6XrDSTuH1/IhA4+vrzHlnozk+4wEODJhuRGY6XASsQn8BaKBL87vG1bo1HLsE0\n" + "BKNTMFEwHQYDVR0OBBYEFD3IARFV4xeb2Hw7CJ6BN3RelH0LMB8GA1UdIwQYMBaA\n" + "FD3IARFV4xeb2Hw7CJ6BN3RelH0LMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0E\n" + "AwIDZwAwZAIwSzwBq0AMop4oKortEaJqFfE1WpJ1SxUquh+gPI4L837BQlCtTnbz\n" + "WYRTzOonSgfqAjBNiSzgQDQklfKO2ic6u0mBysD8m9SeCRZcCnv9Cab+6IfW9SCW\n" + "7mjPWf1Uim6yFxoxggGgMIIBnAIBATArMBMxETAPBgNVBAMMCHVuaXR0ZXN0AhRM\n" + "u2NIHPHneplG9npSvsP2SCg4eTANBglghkgBZQMEAgEFAKCB5DAYBgkqhkiG9w0B\n" + "CQMxCwYJKoZIhvcNAQcBMBwGCSqGSIb3DQEJBTEPFw0yMzExMjExMTU5MzZaMC8G\n" + "CSqGSIb3DQEJBDEiBCC+p2wD9zFw75lsBiKbbPJ1KDsh52gwPSI0d0KKBfyGLzB5\n" + "BgkqhkiG9w0BCQ8xbDBqMAsGCWCGSAFlAwQBKjALBglghkgBZQMEARYwCwYJYIZI\n" + "AWUDBAECMAoGCCqGSIb3DQMHMA4GCCqGSIb3DQMCAgIAgDANBggqhkiG9w0DAgIB\n" + "QDAHBgUrDgMCBzANBggqhkiG9w0DAgIBKDAKBggqhkjOPQQDAgRoMGYCMQD7s/oo\n" + "MspfBQyDQ3RbEmJnub3d0JQjVFJjpSuQZuFfR4tn061LM0txurggtkCLU3MCMQCl\n" + "rkK0zZs3G7UH2W4XXmzBQsfYGooTSyt5hASTo14xnA8GssngKQcztjxQ19nIic0=\n" + "-----END PKCS7-----"; + + +TEST(OpenSSL, verify_broken_pkcs7) +{ + std::list certs; + + certs.emplace_back(unit_test_ca, "unit test certificate"); + + const std::string ident = "nothing to see here"; + + OVPN_EXPECT_THROW( + OpenSSLSign::verify_pkcs7(certs, broken_pkcs7, ident), + OpenSSLException, + "OpenSSLSign::verify_pkcs7: verification failed"); +} + +TEST(OpenSSL, verify_valid_pkcs7) +{ + std::list certs; + + certs.emplace_back(unit_test_ca, "unit test certificate"); + + EXPECT_NO_THROW(OpenSSLSign::verify_pkcs7(certs, laterne_sig, laterne)); +} + +TEST(OpenSSL, verify_nodetach_pkcs7) +{ + std::list certs; + + certs.emplace_back(unit_test_ca, "unit test certificate"); + + EXPECT_NO_THROW(OpenSSLSign::verify_pkcs7(certs, laterne_signd, laterne)); +} \ No newline at end of file From b4a400f6fed635bf2182cd92a66ff2f0e6f3f923 Mon Sep 17 00:00:00 2001 From: Lev Stipakov Date: Mon, 5 Feb 2024 15:38:28 +0200 Subject: [PATCH 11/12] Improve handling of unknown options Currently we error out on the first unsupported option which belongs to the "fatal" category, such as "removed deprecated option" or "Option allowed only to be pushed by the server". To improve user experice and allow application code to display all problematic options and their categories, collect options into a category->options map and then serialize it into multiline string: cat1: opt1,opt2 cat2: opt3 Introduce a new error code UNUSED_OPTIONS, which is placed into ClientAPI::Status::status. The serialized options map is placed into ClientAPI::Status::message. Signed-off-by: Lev Stipakov --- client/ovpncli.cpp | 2 +- openvpn/client/cliopt.hpp | 103 ++++++++++++++++++++++----------- openvpn/error/error.hpp | 2 + test/ovpncli/cli.cpp | 2 +- test/unittests/test_cliopt.cpp | 26 ++++++--- 5 files changed, 92 insertions(+), 43 deletions(-) diff --git a/client/ovpncli.cpp b/client/ovpncli.cpp index 9ab02a35d..545f8d3ca 100644 --- a/client/ovpncli.cpp +++ b/client/ovpncli.cpp @@ -1156,7 +1156,7 @@ OPENVPN_CLIENT_EXPORT Status OpenVPNClient::status_from_exception(const std::exc { Status ret; ret.error = true; - ret.message = Unicode::utf8_printable(e.what(), 256); + ret.message = Unicode::utf8_printable(e.what(), 256 | Unicode::UTF8_PASS_FMT); // if exception is an ExceptionCode, translate the code // to return status string diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index d452da167..81591549e 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -30,6 +30,7 @@ #include #include +#include #include #include @@ -825,6 +826,54 @@ class ClientOptions : public RC "txqueuelen", /* so platforms evaluate that in tun, some do not, do not warn about that */ "verb"}; + class OptionErrors + { + public: + void add_failed_opt(const Option &o, const std::string &message, bool fatal_arg) + { + if (options_per_category.find(message) == options_per_category.end()) + { + options_per_category[message] = {}; + } + + fatal |= fatal_arg; + options_per_category[message].push_back(o); + } + + void print_option_errors() + { + std::ostringstream os; + + for (const auto &[category, options] : options_per_category) + { + if (!options.empty()) + { + OPENVPN_LOG(category); + + os << category << ": "; + std::vector opts; + for (size_t i = 0; i < options.size(); ++i) + { + auto &o = options[i]; + OPENVPN_LOG(std::to_string(i) << ' ' << o.render(Option::RENDER_BRACKET | Option::RENDER_TRUNC_64)); + opts.push_back(o.get(0, 64)); + } + + os << string::join(opts, ",") << std::endl; + } + } + + if (fatal) + { + throw ErrorCode(Error::UNUSED_OPTIONS, true, os.str()); + } + } + + private: + std::map> options_per_category; + bool fatal = false; + }; + /** * This groups all the options that OpenVPN 2.x supports that the * OpenVPN v3 client does not support into a number of different groups @@ -886,77 +935,65 @@ class ClientOptions : public RC OPENVPN_LOG_NTNL("NOTE: This configuration contains options that were not used:" << std::endl); + OptionErrors errors{}; + /* Go through all options and check all options that have not been * touched (parsed) yet */ - showUnusedOptionsByList(opt, settings_removedOptions, "Removed deprecated option", true); - showUnusedOptionsByList(opt, settings_serverOnlyOptions, "Server only option", true); - showUnusedOptionsByList(opt, settings_standalone_options, "OpenVPN 2.x command line operation", true); - showUnusedOptionsByList(opt, settings_feature_not_implemented_warn, "Feature not implemented (option ignored)", false); - showUnusedOptionsByList(opt, settings_pushonlyoptions, "Option allowed only to be pushed by the server", true); - - showUnusedOptionsByList(opt, settings_script_plugin_feature, "Ignored (no script/plugin support)", false); - showUnusedOptionsByList(opt, ignore_unknown_option_list, "Ignored by option 'ignore-unknown-option'", false); - showUnusedOptionsByList(opt, settings_ignoreWithWarning, "Unsupported option (ignored)", false); + showUnusedOptionsByList(opt, settings_removedOptions, "Removed deprecated option", true, errors); + showUnusedOptionsByList(opt, settings_serverOnlyOptions, "Server only option", true, errors); + showUnusedOptionsByList(opt, settings_standalone_options, "OpenVPN 2.x command line operation", true, errors); + showUnusedOptionsByList(opt, settings_feature_not_implemented_warn, "Feature not implemented (option ignored)", false, errors); + showUnusedOptionsByList(opt, settings_pushonlyoptions, "Option allowed only to be pushed by the server", true, errors); + showUnusedOptionsByList(opt, settings_script_plugin_feature, "Ignored (no script/plugin support)", false, errors); + showUnusedOptionsByList(opt, ignore_unknown_option_list, "Ignored by option 'ignore-unknown-option'", false, errors); + showUnusedOptionsByList(opt, settings_ignoreWithWarning, "Unsupported option (ignored)", false, errors); auto ignoredBySetenvOpt = [](const Option &option) { return !option.touched() && option.warnonlyunknown(); }; - showOptionsByFunction(opt, ignoredBySetenvOpt, "Ignored options prefixed with 'setenv opt'", false); + showOptionsByFunction(opt, ignoredBySetenvOpt, "Ignored options prefixed with 'setenv opt'", false, errors); auto unusedMetaOpt = [](const Option &option) { return !option.touched() && option.meta(); }; - showOptionsByFunction(opt, unusedMetaOpt, "Unused ignored meta options", false); + showOptionsByFunction(opt, unusedMetaOpt, "Unused ignored meta options", false, errors); auto managmentOpt = [](const Option &option) { return !option.touched() && option.get(0, 0).rfind("management", 0) == 0; }; - showOptionsByFunction(opt, managmentOpt, "OpenVPN management interface is not supported by this client", true); + showOptionsByFunction(opt, managmentOpt, "OpenVPN management interface is not supported by this client", true, errors); // If we still have options that are unaccounted for, we print them and throw an error or just warn about them auto onlyLightlyTouchedOptions = [](const Option &option) { return option.touched_lightly(); }; - showOptionsByFunction(opt, onlyLightlyTouchedOptions, "Unused options, probably specified multiple times in the configuration file", false); + showOptionsByFunction(opt, onlyLightlyTouchedOptions, "Unused options, probably specified multiple times in the configuration file", false, errors); auto nonTouchedOptions = [](const Option &option) { return !option.touched() && !option.touched_lightly(); }; - showOptionsByFunction(opt, nonTouchedOptions, OPENVPN_UNUSED_OPTIONS, true); + showOptionsByFunction(opt, nonTouchedOptions, OPENVPN_UNUSED_OPTIONS, true, errors); + + errors.print_option_errors(); } - void showUnusedOptionsByList(const OptionList &optlist, std::unordered_set option_set, const std::string &message, bool fatal) + void showUnusedOptionsByList(const OptionList &optlist, std::unordered_set option_set, const std::string &message, bool fatal, OptionErrors &errors) { auto func = [&option_set](const Option &opt) { return !opt.touched() && option_set.find(opt.get(0, 0)) != option_set.end(); }; - showOptionsByFunction(optlist, func, message, fatal); + showOptionsByFunction(optlist, func, message, fatal, errors); } /* lambda expression that capture variables have complex signatures, avoid these by letting the compiler * itself figure it out with a template */ template - void showOptionsByFunction(const OptionList &opt, T func, const std::string &message, bool fatal) + void showOptionsByFunction(const OptionList &opt, T func, const std::string &message, bool fatal, OptionErrors &errors) { - bool messageShown = false; - /* Remember options that caused a fatal error */ - std::set unsupported_option_in_config; - for (size_t i = 0; i < opt.size(); ++i) { auto &o = opt[i]; if (func(o)) { - if (!messageShown) - { - OPENVPN_LOG(message); - messageShown = true; - } o.touch(); - unsupported_option_in_config.emplace(o.get(0, 0)); - OPENVPN_LOG_NTNL(std::to_string(i) << ' ' << o.render(Option::RENDER_BRACKET | Option::RENDER_TRUNC_64) << std::endl); + errors.add_failed_opt(o, message, fatal); } } - if (fatal && messageShown) - { - throw option_error("sorry, unsupported options present in configuration: " + message - + " (" + string::join(unsupported_option_in_config, ",") + ")"); - } } static PeerInfo::Set::Ptr build_peer_info(const Config &config, const ParseClientConfig &pcc, const bool autologin_sessions) diff --git a/openvpn/error/error.hpp b/openvpn/error/error.hpp index 8e9a99be1..b5a7df3fd 100644 --- a/openvpn/error/error.hpp +++ b/openvpn/error/error.hpp @@ -93,6 +93,7 @@ enum Type PROXY_NEED_CREDS, // HTTP proxy needs credentials EARLY_NEG_INVALID, // Early protoctol negotiation information invalid/parse error NTLM_MISSING_CRYPTO, // crypto primitives requires for NTLM are unavailable + UNUSED_OPTIONS, // unused/unknown options found in configuration // key event errors KEV_NEGOTIATE_ERROR, @@ -178,6 +179,7 @@ inline const char *name(const size_t type) "PROXY_NEED_CREDS", "EARLY_NEG_INVALID", "NTLM_MISSING_CRYPTO", + "UNUSED_OPTIONS_ERROR", "KEV_NEGOTIATE_ERROR", "KEV_PENDING_ERROR", "N_KEV_EXPIRE", diff --git a/test/ovpncli/cli.cpp b/test/ovpncli/cli.cpp index 490d26e8b..9139feea4 100644 --- a/test/ovpncli/cli.cpp +++ b/test/ovpncli/cli.cpp @@ -727,7 +727,7 @@ static void worker_thread() { std::cout << "connect error: "; if (!connect_status.status.empty()) - std::cout << connect_status.status << ": "; + std::cout << connect_status.status << ": " << std::endl; std::cout << connect_status.message << std::endl; } } diff --git a/test/unittests/test_cliopt.cpp b/test/unittests/test_cliopt.cpp index a86aeecc2..1a881da06 100644 --- a/test/unittests/test_cliopt.cpp +++ b/test/unittests/test_cliopt.cpp @@ -101,8 +101,8 @@ TEST(config, parse_unknown_option) { OVPN_EXPECT_THROW( load_client_config(minimalConfig + "bikeshed-color green"), - option_error, - "sorry, unsupported options present in configuration: UNKNOWN/UNSUPPORTED OPTIONS"); + ErrorCode, + "UNKNOWN/UNSUPPORTED OPTIONS"); } TEST(config, duplicate_option) @@ -131,12 +131,12 @@ TEST(config, parse_management) { OVPN_EXPECT_THROW( load_client_config(minimalConfig + "management-is-blue"), - option_error, + ErrorCode, "OpenVPN management interface is not supported by this client"); OVPN_EXPECT_THROW( load_client_config(minimalConfig + "management"), - option_error, + ErrorCode, "OpenVPN management interface is not supported by this client"); } @@ -201,11 +201,11 @@ TEST(config, server_options_present_in_error_msg) for (auto &option : server_options) { auto optname = option.substr(0, option.find(' ')); - auto expected_error_string = "Server only option (" + optname; + auto expected_error_string = "Server only option: " + optname; OVPN_EXPECT_THROW( load_client_config(minimalConfig + option), - option_error, + ErrorCode, expected_error_string); } } @@ -217,16 +217,26 @@ TEST(config, unknown_options_present_in_error_msg) for (auto &option : server_options) { auto optname = option.substr(0, option.find(' ')); - auto expected_error_string = "UNKNOWN/UNSUPPORTED OPTIONS (" + optname; + auto expected_error_string = "UNKNOWN/UNSUPPORTED OPTIONS: " + optname; OVPN_EXPECT_THROW( load_client_config(minimalConfig + option), - option_error, + ErrorCode, expected_error_string); } } +TEST(config, multiple_option_errors) +{ + std::ostringstream os; + os << "OpenVPN management interface is not supported by this client: management\n"; + os << "UNKNOWN/UNSUPPORTED OPTIONS: lol,lal"; + OVPN_EXPECT_THROW( + load_client_config(minimalConfig + "management\nlol\nlal"), + ErrorCode, + os.str()); +} TEST(config, client_missing_in_config) { From 8f4cd953b26eae74d36afe62fdb473418f09e9e2 Mon Sep 17 00:00:00 2001 From: David Sommerseth Date: Tue, 13 Feb 2024 18:07:01 +0100 Subject: [PATCH 12/12] Release: OpenVPN 3 Core Library, version 3.8.4 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 c9093fee0..e31021da2 100644 --- a/openvpn/common/version.hpp +++ b/openvpn/common/version.hpp @@ -24,5 +24,5 @@ #pragma once #ifndef OPENVPN_VERSION -#define OPENVPN_VERSION "3.8.3" +#define OPENVPN_VERSION "3.8.4" #endif