openvpn-devel Mailing List for OpenVPN

Signed-off-by: Richard T Bonhomme <tin...@pr...> --- doc/man-sections/protocol-options.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst index c7aa6b0e..bde91779 100644 --- a/doc/man-sections/protocol-options.rst +++ b/doc/man-sections/protocol-options.rst @@ -18,13 +18,13 @@ configured in a compatible way between both the local and remote side. The ``mode`` argument can be one of the following values: - :code:`asym` (default) + :code:`asym` OpenVPN will only *decompress downlink packets* but *not compress uplink packets*. This also allows migrating to disable compression when changing both server and client configurations to remove compression at the same time is not a feasible option. - :code:`no` + :code:`no` (default) OpenVPN will refuse any non-stub compression. :code:`yes` -- 2.25.1 

This allows to use the same configuration multiple platforms/ssl libraries and include optional algorithms that are not available on all platforms For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to emulate the default behaviour of OpenVPN 2.6. --- Changes.rst | 4 ++++ doc/man-sections/protocol-options.rst | 7 +++++++ src/openvpn/ssl_ncp.c | 16 ++++++++++++++-- tests/unit_tests/openvpn/test_ncp.c | 11 +++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/Changes.rst b/Changes.rst index 7cceffcdb..c1a04deed 100644 --- a/Changes.rst +++ b/Changes.rst @@ -58,6 +58,10 @@ OpenSSL 3.0 support (and other deprecated) algorithm by default and the new option ``--providers`` allows loading the legacy provider to renable these algorithms. +Optional ciphers in ``--data-ciphers`` + Ciphers in ``--data-ciphers`` can now be prefixes with a ``?`` to mark + those as optional and only use them if the SSL library supports them. + Deprecated features ------------------- ``inetd`` has been removed diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst index c7aa6b0e3..7095b6f4d 100644 --- a/doc/man-sections/protocol-options.rst +++ b/doc/man-sections/protocol-options.rst @@ -204,6 +204,13 @@ configured in a compatible way between both the local and remote side. supported by the client will be pushed to clients that support cipher negotiation. + Starting with OpenVPN 2.6 a cipher can be prefixed with a :code:`?` to mark + it as optional. This allows including ciphers in the list that may not be + available on all platforms. + E.g. :code:`AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305` would only enable + Chacha20-Poly1305 if the underlying SSL library (and its configuration) + supports it. + Cipher negotiation is enabled in client-server mode only. I.e. if ``--mode`` is set to 'server' (server-side, implied by setting ``--server`` ), or if ``--pull`` is specified (client-side, implied by diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c index 022a9dc3b..b0b248aae 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -109,7 +109,18 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) * (and translate_cipher_name_from_openvpn/ * translate_cipher_name_to_openvpn) also normalises the cipher name, * e.g. replacing AeS-128-gCm with AES-128-GCM + * + * ciphers that have ? in front of them are considered optional and + * OpenVPN will only warn if they are not found (and remove them from + * the list) */ + + bool optional = false; + if (token[0] == '?') + { + token= token + 1; + optional = true; + } const cipher_kt_t *ktc = cipher_kt_get(token); if (strcmp(token, "none") == 0) { @@ -121,8 +132,9 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) } if (!ktc && strcmp(token, "none") != 0) { - msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token); - error_found = true; + const char* optstr = optional ? "optional ": ""; + msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token); + error_found = !optional; } else { diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c index 6fb0c0e51..faf09a36c 100644 --- a/tests/unit_tests/openvpn/test_ncp.c +++ b/tests/unit_tests/openvpn/test_ncp.c @@ -84,6 +84,17 @@ test_check_ncp_ciphers_list(void **state) assert_ptr_equal(mutate_ncp_cipher_list(bf_chacha, &gc), NULL); } + /* Check that optional ciphers work */ + assert_string_equal(mutate_ncp_cipher_list("AES-256-GCM:?vollbit:AES-128-GCM", &gc), + aes_ciphers); + + /* Check that optional ciphers work */ + assert_string_equal(mutate_ncp_cipher_list("?AES-256-GCM:?AES-128-GCM", &gc), + aes_ciphers); + + /* All unsupported should still yield an empty list */ + assert_ptr_equal(mutate_ncp_cipher_list("?kugelfisch:?grasshopper", &gc), NULL); + /* For testing that with OpenSSL 1.1.0+ that also accepts ciphers in * a different spelling the normalised cipher output is the same */ bool have_chacha_mixed_case = cipher_kt_get("ChaCha20-Poly1305"); -- 2.33.0 

When tls_deauthenticate is called (e.g. by management kicking of a client) the key auth state is changed to KS_AUTH_FALSE while the key state is still in S_GENERATED_KEYS. This triggers the assertion. Remove the assertions and instead check that the auth state is KS_AUTH_TRUE Signed-off-by: Arne Schwabe <ar...@rf...> --- src/openvpn/ssl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index ad3e08274..5e53ad57e 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -3278,9 +3278,9 @@ handle_data_channel_packet(struct tls_multi *multi, * active side is the client which initiates connections). */ if (ks->state >= S_GENERATED_KEYS && key_id == ks->key_id + && ks->authenticated == KS_AUTH_TRUE && (floated || link_socket_actual_match(from, &ks->remote_addr))) { - ASSERT(ks->authenticated == KS_AUTH_TRUE); if (!ks->crypto_options.key_ctx_bi.initialized) { msg(D_MULTI_DROPPED, @@ -3863,9 +3863,8 @@ struct key_state *tls_select_encryption_key(struct tls_multi *multi) for (int i = 0; i < KEY_SCAN_SIZE; ++i) { struct key_state *ks = get_key_scan(multi, i); - if (ks->state >= S_GENERATED_KEYS) + if (ks->state >= S_GENERATED_KEYS && ks->authenticated == KS_AUTH_TRUE) { - ASSERT(ks->authenticated == KS_AUTH_TRUE); ASSERT(ks->crypto_options.key_ctx_bi.initialized); if (!ks_select) -- 2.33.0 

Hi, On Sun, Nov 28, 2021 at 01:27:19AM +0000, tincantech via Openvpn-devel wrote: > Seems I jumped the gun a little.. > > NACK, --compress is not deprecated in 2.4 If you send patches for something that is not "master", it must be made clear in some way - either in the Subject: line, or in the commit message ("this patch is for 2.4 only because ...") gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany ge...@gr... 

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Seems I jumped the gun a little.. NACK, --compress is not deprecated in 2.4 Sorry R Sent with ProtonMail Secure Email. ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday, November 26th, 2021 at 14:48, Antonio Quartulli <a...@un...> wrote: > Hi, > > On 26/11/2021 15:43, Richard T Bonhomme wrote: > > > Signed-off-by: Richard T Bonhomme tin...@pr... > > ----------------------------------------------------------- > > > > doc/openvpn.8 | 1 + > > > > 1 file changed, 1 insertion(+) > > > > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > > > > index 598d5fce..dfe0ad10 100644 > > > > --- a/doc/openvpn.8 > > > > +++ b/doc/openvpn.8 > > > > @@ -2501,6 +2501,7 @@ limit repetitive logging of similar message types. > > > > .\"********************************************************* > > > > .TP > > > > .B \-\-compress [algorithm] > > > > +.B DEPRECATED > > > > Enable a compression algorithm. > > > > The > > We already have this. Against what branch did you create this patch? > > Cheers, > > -------------------------------------------------------------------------------- > > Antonio Quartulli > > Openvpn-devel mailing list > > Ope...@li... > > https://lists.sourceforge.net/lists/listinfo/openvpn-devel -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBzBAEBCAAGBQJhotroACEJEE+XnPZrkLidFiEECbw9RGejjXJ5xVVVT5ec 9muQuJ1VPgf+O45rGSxFsovrhOa2OQ3r9DFCiG8Wsc51hs24jqH8CJd7fiaI Bqy3ZIJsxi/PQocn2JCqE0odfFSiHOQVLpGEkCS2dSnbOG7wrA+LKqWbuTMu xnly3U0a1ekMwunKbXxE6tGgQXrS0aBPkcx5oNWyxvFGNruYoDUwHkOex2zh mUvjQS1F/TWxwBaXm/2awzBOLghIqUNE+l+Fm5eAN+025jUP9g3TUVlxBF6S j6FOjIEXukN75t/K01g2ME/P4RJgvjNcRLw2kQewD8ajtGJhweRQKyaFQOJY xSKE3ePA7wWoTVX0xpY41Gruin3usMXwL6XeU3/WyX6CLZY5nQIQ4g== =nFcI -----END PGP SIGNATURE----- 

Hi, On 26/11/2021 15:43, Richard T Bonhomme wrote: > Signed-off-by: Richard T Bonhomme <tin...@pr...> > --- > doc/openvpn.8 | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index 598d5fce..dfe0ad10 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -2501,6 +2501,7 @@ limit repetitive logging of similar message types. > .\"********************************************************* > .TP > .B \-\-compress [algorithm] > +.B DEPRECATED > Enable a compression algorithm. > > The We already have this. Against what branch did you create this patch? Cheers, -- Antonio Quartulli 

Signed-off-by: Richard T Bonhomme <tin...@pr...> --- doc/openvpn.8 | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 598d5fce..dfe0ad10 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2501,6 +2501,7 @@ limit repetitive logging of similar message types. .\"********************************************************* .TP .B \-\-compress [algorithm] +.B DEPRECATED Enable a compression algorithm. The -- 2.25.1 

Acked-by: Gert Doering <ge...@gr...> Thanks. (Found while looking at the compiler warnings reviewing the OpenSSL config loading patch). Test built on MinGW, warnings gone. Your patch has been applied to the master branch. commit 71371f04afa6eba3ea02a67590a70e018cf203e5 (master) commit 36b3129d47a6dbfcd43ff4773c69618a28eb48bc (release/2.5) Author: Lev Stipakov Date: Wed Nov 24 18:03:47 2021 +0200 ring_buffer.h: fix GCC warning about unused function Signed-off-by: Lev Stipakov <le...@op...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg23260.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

From: Lev Stipakov <le...@op...> With register_ring_buffers() being declared as "static" in header file, all translation units, which include that header, got a copy of that function. This causes GCC warning warning: "register_ring_buffers" defined but not used [-Wunused-function] when compiling C files which include header, but don't use function. Add "inline" keyword to silence this warning. Signed-off-by: Lev Stipakov <le...@op...> --- src/openvpn/ring_buffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/ring_buffer.h b/src/openvpn/ring_buffer.h index 77579e3f..9661ceb3 100644 --- a/src/openvpn/ring_buffer.h +++ b/src/openvpn/ring_buffer.h @@ -94,7 +94,7 @@ struct TUN_PACKET * that data has been written to receive ring * @return true if registration is successful, false otherwise - use GetLastError() */ -static bool +static inline bool register_ring_buffers(HANDLE device, struct tun_ring *send_ring, struct tun_ring *receive_ring, -- 2.23.0.windows.1 

Hi, On Wed, Nov 24, 2021 at 04:21:38PM +0200, Lev Stipakov wrote: > Since we're preparing the next 2.5 release, let's take this in. The > performance numbers, reported in different thread, look good. Yes, sir! commit dab73f90983c929645c74f1c1c045e175d5ca2b6 (HEAD -> release/2.5) Author: Ilya Shipitsin <chi...@gm...> Date: Wed Sep 22 14:57:55 2021 +0500 BUILD: enable CFG and Spectre mitigation for MSVC found by BinSkim Signed-off-by: Ilya Shipitsin <chi...@gm...> Acked-by: Lev Stipakov <lst...@gm...> Message-Id: <202...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg22870.html Signed-off-by: Gert Doering <ge...@gr...> (cherry picked from commit e80e36d75538abff0661b21392f541d946be6d29) ... totally untested, but this is all MSVC build stuff, not code change... gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany ge...@gr... 

Hi, On Wed, Nov 24, 2021 at 9:28 AM Lev Stipakov <lst...@gm...> wrote: > Do we need this fix in openvpn-gui? It only (?) uses openssl to change > private key password, could this functionality be affected by config? > I do not know.. We do not call any functions that would lead to a config loading, so probably not required. Automatic crypto initialization does not load the config, nor does an explicit call to OPENSSL_init_crypto() unless instructed to. OPENSSL_init_ssl() loads the config unless explicitly disabled, but we do not use it in the GUI. However, to be on the safe side, we could set these env vars if not already set by the user. Selva 

Hi On Wed, Nov 24, 2021 at 5:06 AM Gert Doering <ge...@gr...> wrote: > Your patch has been applied to the master and release/2.5 branch > (I consider this a bugfix since the "do not load config!" CVE patch > unintendedly broke functionality for people) > What would be a good location in the man page where we can document this. These are not env vars we natively support so putting it under a section named "ENVIRONMENT VARIABLES" does not seem right. Also we already have a section with that name which refer to env vars we export to scripts. Selva 

Performance report was delayed due to oversize. It was only delivered to Lev and Gert. Thank you Lev for bringing this up:) Performance was captured by WPT. I'd like to make some similar investigation if needed (to compare other perf impact) On Wed, Nov 24, 2021, 7:21 PM Lev Stipakov <lst...@gm...> wrote: > Since we're preparing the next 2.5 release, let's take this in. The > performance numbers, reported in different thread, look good. > > ma 27. syysk. 2021 klo 13.06 Илья Шипицин (chi...@gm...) > kirjoitti: > > > > I'll setup test stand similar to this one > https://community.openvpn.net/openvpn/wiki/PerformanceTestingOpenVPN > > hopefully in next 1-2 weeks > > > > I also believe that spectre mitigation is neglectable, but it is good to > have numbers > > > > пн, 27 сент. 2021 г. в 12:58, Lev Stipakov <lst...@gm...>: > >> > >> I didn't, but here > >> > >> https://devblogs.microsoft.com/cppblog/spectre-mitigations-in-msvc/ > >> > >> it says that > >> > >> "On the MSVC team, we’ve reviewed information in detail and conducted > >> extensive tests, which showed the performance impact of the new > >> /Qspectre switch to be negligible." > >> -- > >> -Lev > > > > -- > -Lev > 

Do we need this fix in openvpn-gui? It only (?) uses openssl to change private key password, could this functionality be affected by config? ke 24. marrask. 2021 klo 12.06 Gert Doering (ge...@gr...) kirjoitti: > > Your patch has been applied to the master and release/2.5 branch > (I consider this a bugfix since the "do not load config!" CVE patch > unintendedly broke functionality for people) > > As instructed I have changed "* " to " *" according to style :-), and > removed the double declaration in buffer.h - the latter is something > on the edge of "can I do that at commit time?" but in this case it's > "the very same declaration", "tun.c and win32.c do include win32.h", > so easy enough. > > I did not change the "static" bit. > > I have not tested this "for real", just did a test compile of master > and release/2.5 on MinGW. But the code change looks good, and it does > not break compilation, even with my changes :-) > > Added the trac reference to #1296 (thanks Lev for digging it up). > > commit 23e6aaef149bd31a7e80af28ee1e3658d2810d4f (master) > commit f911b3f69b0a8296918a06d02eb5144bb4cd8a06 (release/2.5) > Author: Lev Stipakov > Date: Fri Nov 19 03:55:48 2021 +0200 > > Load OpenSSL config on Windows from trusted location > > Signed-off-by: Lev Stipakov <le...@op...> > Acked-by: Selva Nair <sel...@gm...> > Message-Id: <202...@gm...> > URL: https://www.mail-archive.com/ope...@li.../msg23248.html > Signed-off-by: Gert Doering <ge...@gr...> > > > -- > kind regards, > > Gert Doering > > > > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- -Lev 

Since we're preparing the next 2.5 release, let's take this in. The performance numbers, reported in different thread, look good. ma 27. syysk. 2021 klo 13.06 Илья Шипицин (chi...@gm...) kirjoitti: > > I'll setup test stand similar to this one https://community.openvpn.net/openvpn/wiki/PerformanceTestingOpenVPN > hopefully in next 1-2 weeks > > I also believe that spectre mitigation is neglectable, but it is good to have numbers > > пн, 27 сент. 2021 г. в 12:58, Lev Stipakov <lst...@gm...>: >> >> I didn't, but here >> >> https://devblogs.microsoft.com/cppblog/spectre-mitigations-in-msvc/ >> >> it says that >> >> "On the MSVC team, we’ve reviewed information in detail and conducted >> extensive tests, which showed the performance impact of the new >> /Qspectre switch to be negligible." >> -- >> -Lev -- -Lev 

From: Lev Stipakov <le...@op...> Starting from commit 21b2dbd3 "[scripts-audit] nmake buildsystem" vcpkg has removed NO_DEBUG support from nmake buildsystem and now builds debug variant unconditionally. Debug flags contradict build options hardcoded in pkcs11 nmake script (like /O2). Remove hardcoded release options and other options which are (also) set by vcpkg nmake buildsystem. Bump vcpkg commit in GitHub actions. Signed-off-by: Lev Stipakov <le...@op...> --- .github/workflows/build.yaml | 2 +- ...nmake-compatibility-with-vcpkg-nmake.patch | 38 +++++++++++++++++++ .../vcpkg-ports/pkcs11-helper/portfile.cmake | 10 ++--- 3 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 contrib/vcpkg-ports/pkcs11-helper/0002-nmake-compatibility-with-vcpkg-nmake.patch diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index abf32b14..51d9dd4d 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -217,7 +217,7 @@ jobs: - name: Restore artifacts, or run vcpkg, build and cache artifacts uses: lukka/run-vcpkg@v7.4 with: - vcpkgGitCommitId: '71422c627264daedcbcd46f01f1ed0dcd8460f1b' + vcpkgGitCommitId: 'a2fcb03749ff5897b5985092934dc6057680c789' vcpkgArguments: 'openssl lz4 lzo pkcs11-helper tap-windows6' vcpkgTriplet: '${{ matrix.triplet }}-windows-ovpn' cleanAfterBuild: false diff --git a/contrib/vcpkg-ports/pkcs11-helper/0002-nmake-compatibility-with-vcpkg-nmake.patch b/contrib/vcpkg-ports/pkcs11-helper/0002-nmake-compatibility-with-vcpkg-nmake.patch new file mode 100644 index 00000000..a6034f7e --- /dev/null +++ b/contrib/vcpkg-ports/pkcs11-helper/0002-nmake-compatibility-with-vcpkg-nmake.patch @@ -0,0 +1,38 @@ +From 68d12f3e955cc9df435e9289b1244a4c1f24b96b Mon Sep 17 00:00:00 2001 +From: Lev Stipakov <le...@op...> +Date: Wed, 24 Nov 2021 11:21:36 +0200 +Subject: [PATCH] nmake: compatibility with vcpkg nmake + +Remove options which contradict or already set +by vcpkg nmake scripts. + +Signed-off-by: Lev Stipakov <le...@op...> +--- + lib/Makefile.w32-vc | 8 ++------ + 1 file changed, 2 insertions(+), 6 deletions(-) + +diff --git a/lib/Makefile.w32-vc b/lib/Makefile.w32-vc +index 0e64f42..18af03b 100644 +--- a/lib/Makefile.w32-vc ++++ b/lib/Makefile.w32-vc +@@ -75,15 +75,11 @@ OPENSSL_LIBS=-LIBPATH:$(OPENSSL_LIB) user32.lib advapi32.lib $(OPENSSL_STATIC) + CFLAGS = -I../include $(OPENSSL_CFLAGS) -DWIN32 -DWIN32_LEAN_AND_MEAN -D_MBCS -D_CRT_SECURE_NO_DEPRECATE -D_WIN32_WINNT=0x0400 + CC=cl.exe + RC=rc.exe +-CCPARAMS=/nologo /W3 /O2 /FD /c +- +-CCPARAMS=$(CCPARAMS) /MD +-CFLAGS=$(CFLAGS) -DNDEBUG ++CCPARAMS=/c + + LINK32=link.exe + LIB32=lib.exe +-LINK32_FLAGS=/nologo /subsystem:windows /dll /incremental:no +-LIB32_FLAGS=/nologo ++LINK32_FLAGS=/dll + + HEADERS = \ +	config.h \ +-- +2.23.0.windows.1 + diff --git a/contrib/vcpkg-ports/pkcs11-helper/portfile.cmake b/contrib/vcpkg-ports/pkcs11-helper/portfile.cmake index 54a0009d..ad19fccb 100644 --- a/contrib/vcpkg-ports/pkcs11-helper/portfile.cmake +++ b/contrib/vcpkg-ports/pkcs11-helper/portfile.cmake @@ -12,12 +12,12 @@ vcpkg_extract_source_archive_ex( REF ${VERSION} PATCHES 0001-nmake-openssl-1.1.1-support.patch + 0002-nmake-compatibility-with-vcpkg-nmake.patch pkcs11-helper-001-RFC7512.patch ) vcpkg_build_nmake( SOURCE_PATH ${SOURCE_PATH} - NO_DEBUG PROJECT_SUBPATH lib PROJECT_NAME Makefile.w32-vc OPTIONS @@ -26,10 +26,10 @@ vcpkg_build_nmake( ) file(INSTALL ${SOURCE_PATH}/include/pkcs11-helper-1.0 DESTINATION ${CURRENT_PACKAGES_DIR}/include/) -file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}/lib/pkcs11-helper.dll.lib DESTINATION ${CURRENT_PACKAGES_DIR}/lib) -file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}/lib/pkcs11-helper.dll.lib DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib) +file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/lib/pkcs11-helper.dll.lib DESTINATION ${CURRENT_PACKAGES_DIR}/lib) +file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg/lib/pkcs11-helper.dll.lib DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib) -file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}/lib/libpkcs11-helper-1.dll DESTINATION ${CURRENT_PACKAGES_DIR}/bin) -file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}/lib/libpkcs11-helper-1.dll DESTINATION ${CURRENT_PACKAGES_DIR}/debug/bin) +file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/lib/libpkcs11-helper-1.dll DESTINATION ${CURRENT_PACKAGES_DIR}/bin) +file(INSTALL ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg/lib/libpkcs11-helper-1.dll DESTINATION ${CURRENT_PACKAGES_DIR}/debug/bin) file(INSTALL ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright) -- 2.23.0.windows.1 

Your patch has been applied to the master and release/2.5 branch (I consider this a bugfix since the "do not load config!" CVE patch unintendedly broke functionality for people) As instructed I have changed "* " to " *" according to style :-), and removed the double declaration in buffer.h - the latter is something on the edge of "can I do that at commit time?" but in this case it's "the very same declaration", "tun.c and win32.c do include win32.h", so easy enough. I did not change the "static" bit. I have not tested this "for real", just did a test compile of master and release/2.5 on MinGW. But the code change looks good, and it does not break compilation, even with my changes :-) Added the trac reference to #1296 (thanks Lev for digging it up). commit 23e6aaef149bd31a7e80af28ee1e3658d2810d4f (master) commit f911b3f69b0a8296918a06d02eb5144bb4cd8a06 (release/2.5) Author: Lev Stipakov Date: Fri Nov 19 03:55:48 2021 +0200 Load OpenSSL config on Windows from trusted location Signed-off-by: Lev Stipakov <le...@op...> Acked-by: Selva Nair <sel...@gm...> Message-Id: <202...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg23248.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Hi, In our codebase we have vcpkg overlay triplets: openvpn\contrib\vcpkg-triplets arm64-windows-ovpn.cmake x64-windows-ovpn.cmake x86-windows-ovpn.cmake They exist only to support static linking of lz4. All other libraries (lzo, libss, libcrypto, libpkcs11-helper) are linked dynamically. The reason for static linking in MSVC is to maintain compatibility with the generic build system, which links lz4 statically. Since we don't use the generic build system for building releases, we could drop --enable-lz4 option support from it and link lz4 in MSVC dynamically. This would allow us to simplify the MSVC build system, get rid of custom triples and remove some code. Thoughts? -- -Lev 

Hi, Looks good in my tests using the msvc artifacts from https://github.com/lstipakov/openvpn/actions/runs/1496339867#artifacts. Loads config from <install-root>\ssl\openssl.cnf and engines specified with relative paths load from <install-root>\ssl\engines. So the env vars are being seen by OpenSSL and being used as expected. I wasted quite some time using the msvc-built executable with mingw compiled OpenSSL libraries (I was being lazy copying only openvpn.exe). For some reason OpenSSL doesn't detect the env vars in this case --- process explorer shows its all set right, but the library doesn't load openssl.cnf. Not worth exploring further, I guess. Testing OPENSSL_MODULES path is left for later when a 3.0 build with MSVC is available. Some nits below which may be ignored. On Tue, Nov 23, 2021 at 2:31 PM Lev Stipakov <lst...@gm...> wrote: > From: Lev Stipakov <le...@op...> > > Commits > > - 92535b6 ("contrib/vcpkg-ports: add openssl port with > --no-autoload-config option set (CVE-2121-3606)") > - 447cfb4 ("crypto_openssl.c: disable explicit initialization on Windows > (CVE-2121-3606)") > > disabled OpenSSL config loading functionality, which could be > exploited by loading config from untrusted locations. > > This feature might be useful for some users. This brings it back > and sets OpenSSL enviroment variables > > OPENSSL_CONF, OPENSSL_ENGINES, OPENSSL_MODULES > > which are used to load config, engines and modules, to a trusted location. > The location is constructed based on installation path, read from registry > on startup. > If installation path cannot be read, Windows\System32 is used as a > fallback. > > While on it, remove unused "bool impersonate_as_system();" declaration. > > Signed-off-by: Lev Stipakov <le...@op...> > --- > v4: > - make set_openssl_env_vars() code more succint > - use security-enhanced _wputenv_s/_wgetenv_s > > v3: > - do not assume that installation path ends with directory separator > - set enviroment variables only if they're not already set > - bring back explicit initialization on Windows (might be needed on > some cases) > - slightly revamp commit message > > v2: > - add missing "static" modifier to set_openssl_env_vars() declaration > spotted by gcc > > .../vcpkg-triplets/arm64-windows-ovpn.cmake | 2 - > contrib/vcpkg-triplets/x64-windows-ovpn.cmake | 2 - > contrib/vcpkg-triplets/x86-windows-ovpn.cmake | 2 - > src/openvpn/buffer.c | 23 ----- > src/openvpn/crypto_openssl.c | 2 - > src/openvpn/win32.c | 88 +++++++++++++++++++ > src/openvpn/win32.h | 8 +- > 7 files changed, 95 insertions(+), 32 deletions(-) > > diff --git a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > index 89f6a279..dd3c6c0a 100644 > --- a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > +++ b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) > if(PORT STREQUAL "lz4") > set(VCPKG_LIBRARY_LINKAGE static) > endif() > - > -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) > diff --git a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > index d860eed6..7036ed2d 100644 > --- a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > +++ b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) > if(PORT STREQUAL "lz4") > set(VCPKG_LIBRARY_LINKAGE static) > endif() > - > -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) > diff --git a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > index c1ea6ef3..7d3bf340 100644 > --- a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > +++ b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) > if(PORT STREQUAL "lz4") > set(VCPKG_LIBRARY_LINKAGE static) > endif() > - > -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) > diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c > index c82d3d4d..54e758af 100644 > --- a/src/openvpn/buffer.c > +++ b/src/openvpn/buffer.c > @@ -310,29 +310,6 @@ openvpn_snprintf(char *str, size_t size, const char > *format, ...) > return (len >= 0 && len < size); > } > > -/* > - * openvpn_swprintf() is currently only used by Windows code paths > - * and when enabled for all platforms it will currently break older > - * OpenBSD versions lacking vswprintf(3) support in their libc. > - */ > - > -#ifdef _WIN32 > -bool > -openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t > *const format, ...) > -{ > - va_list arglist; > - int len = -1; > - if (size > 0) > - { > - va_start(arglist, format); > - len = vswprintf(str, size, format, arglist); > - va_end(arglist); > - str[size - 1] = L'\0'; > - } > - return (len >= 0 && len < size); > -} > -#endif > - > /* > * write a string to the end of a buffer that was > * truncated by buf_printf > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index c9dc9d0a..ef520928 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -154,13 +154,11 @@ crypto_init_lib_engine(const char *engine_name) > void > crypto_init_lib(void) > { > -#ifndef _WIN32 > #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) > OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL); > #else > OPENSSL_config(NULL); > #endif > -#endif /* _WIN32 */ > /* > * If you build the OpenSSL library and OpenVPN with > * CRYPTO_MDEBUG, you will get a listing of OpenSSL > diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c > index 6cff17b2..ee4d3f66 100644 > --- a/src/openvpn/win32.c > +++ b/src/openvpn/win32.c > @@ -101,6 +101,12 @@ struct semaphore netcmd_semaphore; /* GLOBAL */ > */ > static char *win_sys_path = NULL; /* GLOBAL */ > > +/** > + * Set OpenSSL environment variables to a safe directory > + */ > +static void > +set_openssl_env_vars(); > + > void > init_win32(void) > { > @@ -110,6 +116,8 @@ init_win32(void) > } > window_title_clear(&window_title); > win32_signal_clear(&win32_signal); > + > + set_openssl_env_vars(); > } > > void > @@ -1509,4 +1517,84 @@ send_msg_iservice(HANDLE pipe, const void *data, > size_t size, > return ret; > } > > +bool > +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* > const format, ...) > +{ > + va_list arglist; > + int len = -1; > + if (size > 0) > + { > + va_start(arglist, format); > + len = vswprintf(str, size, format, arglist); > + va_end(arglist); > + str[size - 1] = L'\0'; > + } > + return (len >= 0 && len < size); > +} > + > +static BOOL > +get_install_path(WCHAR *path, DWORD size) > +{ > + WCHAR reg_path[256]; > + HKEY key; > + BOOL res = FALSE; > + openvpn_swprintf(reg_path, _countof(reg_path), L"SOFTWARE\\" > PACKAGE_NAME); + > + LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, > KEY_READ, &key); > + if (status != ERROR_SUCCESS) > + { > + return res; > + } > + > + /* The default value of REG_KEY is the install path */ > + status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL, > (LPBYTE)path, &size); > + res = status == ERROR_SUCCESS; > + > + RegCloseKey(key); > + > + return res; > +} > + > +static void > +set_openssl_env_vars() > +{ > + const WCHAR* ssl_fallback_dir = L"C:\\Windows\\System32"; > + > + WCHAR install_path[MAX_PATH] = { 0 }; > + if (!get_install_path(install_path, _countof(install_path))) > + { > + /* if we cannot find installation path from the registry, > + * use Windows directory as a fallback > + */ > + openvpn_swprintf(install_path, _countof(install_path), L"%ls", > ssl_fallback_dir); > + } > + > + if ((install_path[wcslen(install_path) - 1]) == L'\\') > + { > + install_path[wcslen(install_path) - 1] = L'\0'; > + } > + > + static struct { > + WCHAR* name; > + WCHAR* value; > + } ossl_env[] = { > + {L"OPENSSL_CONF", L"openssl.cnf"}, > + {L"OPENSSL_ENGINES", L"engines"}, > + {L"OPENSSL_MODULES", L"modules"} > + }; > As you defined this inside the function, it doesn't have to be static. Also, name and value could be const. Our preferred style is "TYPE *val", not the "TYPE* val" used here and elsewhere in the patch. Neither are worth a v5, IMO. > + > + for (size_t i = 0; i < SIZE(ossl_env); ++i) > + { > + size_t size = 0; > + > + _wgetenv_s(&size, NULL, 0, ossl_env[i].name); > + if (size == 0) > + { > + WCHAR val[MAX_PATH] = {0}; > + openvpn_swprintf(val, _countof(val), L"%ls\\ssl\\%ls", > install_path, ossl_env[i].value); > + _wputenv_s(ossl_env[i].name, val); > + } > + } > +} > + > #endif /* ifdef _WIN32 */ > diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h > index 5d3371a0..4a992d91 100644 > --- a/src/openvpn/win32.h > +++ b/src/openvpn/win32.h > @@ -327,7 +327,13 @@ bool send_msg_iservice(HANDLE pipe, const void *data, > size_t size, > int > openvpn_execve(const struct argv *a, const struct env_set *es, const > unsigned int flags); > > -bool impersonate_as_system(); > +/* > + * openvpn_swprintf() is currently only used by Windows code paths > + * and when enabled for all platforms it will currently break older > + * OpenBSD versions lacking vswprintf(3) support in their libc. > + */ > +bool > +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* > const format, ...); Now there is a duplicate declaration in buffer.h which could be removed. Multiple declarations is not an error, so let it be... > #endif /* ifndef OPENVPN_WIN32_H */ > #endif /* ifdef _WIN32 */ > Acked-by: Selva Nair <sel...@gm...> 

Thanks, I tried this one and client wasn't able to connect: OpenSSL: error:14201076:SSL routines:tls_choose_sigalg:no suitable signature algorithm So it looks like config loading works. The binaries for V4 could be found here: https://github.com/lstipakov/openvpn/actions/runs/1496339867 ti 23. marrask. 2021 klo 20.58 Selva Nair (sel...@gm...) kirjoitti: > > Hi, > > On Tue, Nov 23, 2021 at 1:37 PM Lev Stipakov <lst...@gm...> wrote: >> >> I don't have a setup to properly test it, like actually loading the >> config - I only checked that the openvpn.exe attempted to access >> openssl.cnf at the correct location. >> >> If someone wants to test - binary artifacts could be found here: >> https://github.com/lstipakov/openvpn/actions/runs/1496114596 >> >> >> >> I could also do testing if someone educates me how :) > > > Try using an openssl.cnf like the one below which restricts signature algorithms to a some non-PSS schemes. Change that line to restrict them further or comment out to use defaults: not including PSS will force non-PSS signature with TLS 1.2 even with OpenSSL 1.1.1 server. And will break TLS 1.3 negotiation. Removing ECC signatures and using an EC key certificate will break the connection etc.. > > # > # OpenSSL configuration file to restrict siglags during handshake > # > openssl_conf = default_conf > > [default_conf] > ssl_conf = ssl_sect > > [ssl_sect] > system_default = system_default_sect > > [system_default_sect] > #MinProtocol = TLSv1.2 > #CipherString = DEFAULT@SECLEVEL=0 > SignatureAlgorithms = RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:RSA+SHA1:ECDSA+SHA1 > # possible values > # PKCS1: rsa_pkcs1_sha256:rsa_pkcs1_sha384:rsa_pkcs1_sha512 > # ECDSA: ecdsa_secp256r1_sha256:ecdsa_secp384r1_sha384:ecdsa_secp521r1_sha512 > # PSS with rsa encryption public key rsa_pss_rsae_sha256:rsa_pss_rsae_sha384:rsa_pss_rsae_sha512 > # EdDSA :ed25519:ed448 > # PSS with PSS public key: rsa_pss_pss_sha256:rsa_pss_pss_sha384:rsa_pss_pss_sha512 > # Legacy rsa_pkcs1_sha1:ecdsa_sha1 -- -Lev 

From: Lev Stipakov <le...@op...> Commits - 92535b6 ("contrib/vcpkg-ports: add openssl port with --no-autoload-config option set (CVE-2121-3606)") - 447cfb4 ("crypto_openssl.c: disable explicit initialization on Windows (CVE-2121-3606)") disabled OpenSSL config loading functionality, which could be exploited by loading config from untrusted locations. This feature might be useful for some users. This brings it back and sets OpenSSL enviroment variables OPENSSL_CONF, OPENSSL_ENGINES, OPENSSL_MODULES which are used to load config, engines and modules, to a trusted location. The location is constructed based on installation path, read from registry on startup. If installation path cannot be read, Windows\System32 is used as a fallback. While on it, remove unused "bool impersonate_as_system();" declaration. Signed-off-by: Lev Stipakov <le...@op...> --- v4: - make set_openssl_env_vars() code more succint - use security-enhanced _wputenv_s/_wgetenv_s v3: - do not assume that installation path ends with directory separator - set enviroment variables only if they're not already set - bring back explicit initialization on Windows (might be needed on some cases) - slightly revamp commit message v2: - add missing "static" modifier to set_openssl_env_vars() declaration spotted by gcc .../vcpkg-triplets/arm64-windows-ovpn.cmake | 2 - contrib/vcpkg-triplets/x64-windows-ovpn.cmake | 2 - contrib/vcpkg-triplets/x86-windows-ovpn.cmake | 2 - src/openvpn/buffer.c | 23 ----- src/openvpn/crypto_openssl.c | 2 - src/openvpn/win32.c | 88 +++++++++++++++++++ src/openvpn/win32.h | 8 +- 7 files changed, 95 insertions(+), 32 deletions(-) diff --git a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake index 89f6a279..dd3c6c0a 100644 --- a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake +++ b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) if(PORT STREQUAL "lz4") set(VCPKG_LIBRARY_LINKAGE static) endif() - -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) diff --git a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake index d860eed6..7036ed2d 100644 --- a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake +++ b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) if(PORT STREQUAL "lz4") set(VCPKG_LIBRARY_LINKAGE static) endif() - -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) diff --git a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake index c1ea6ef3..7d3bf340 100644 --- a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake +++ b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) if(PORT STREQUAL "lz4") set(VCPKG_LIBRARY_LINKAGE static) endif() - -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index c82d3d4d..54e758af 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -310,29 +310,6 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...) return (len >= 0 && len < size); } -/* - * openvpn_swprintf() is currently only used by Windows code paths - * and when enabled for all platforms it will currently break older - * OpenBSD versions lacking vswprintf(3) support in their libc. - */ - -#ifdef _WIN32 -bool -openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...) -{ - va_list arglist; - int len = -1; - if (size > 0) - { - va_start(arglist, format); - len = vswprintf(str, size, format, arglist); - va_end(arglist); - str[size - 1] = L'\0'; - } - return (len >= 0 && len < size); -} -#endif - /* * write a string to the end of a buffer that was * truncated by buf_printf diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index c9dc9d0a..ef520928 100644 --- a/src/openvpn/crypto_openssl.c +++ b/src/openvpn/crypto_openssl.c @@ -154,13 +154,11 @@ crypto_init_lib_engine(const char *engine_name) void crypto_init_lib(void) { -#ifndef _WIN32 #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL); #else OPENSSL_config(NULL); #endif -#endif /* _WIN32 */ /* * If you build the OpenSSL library and OpenVPN with * CRYPTO_MDEBUG, you will get a listing of OpenSSL diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index 6cff17b2..ee4d3f66 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -101,6 +101,12 @@ struct semaphore netcmd_semaphore; /* GLOBAL */ */ static char *win_sys_path = NULL; /* GLOBAL */ +/** + * Set OpenSSL environment variables to a safe directory + */ +static void +set_openssl_env_vars(); + void init_win32(void) { @@ -110,6 +116,8 @@ init_win32(void) } window_title_clear(&window_title); win32_signal_clear(&win32_signal); + + set_openssl_env_vars(); } void @@ -1509,4 +1517,84 @@ send_msg_iservice(HANDLE pipe, const void *data, size_t size, return ret; } +bool +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...) +{ + va_list arglist; + int len = -1; + if (size > 0) + { + va_start(arglist, format); + len = vswprintf(str, size, format, arglist); + va_end(arglist); + str[size - 1] = L'\0'; + } + return (len >= 0 && len < size); +} + +static BOOL +get_install_path(WCHAR *path, DWORD size) +{ + WCHAR reg_path[256]; + HKEY key; + BOOL res = FALSE; + openvpn_swprintf(reg_path, _countof(reg_path), L"SOFTWARE\\" PACKAGE_NAME); + + LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &key); + if (status != ERROR_SUCCESS) + { + return res; + } + + /* The default value of REG_KEY is the install path */ + status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL, (LPBYTE)path, &size); + res = status == ERROR_SUCCESS; + + RegCloseKey(key); + + return res; +} + +static void +set_openssl_env_vars() +{ + const WCHAR* ssl_fallback_dir = L"C:\\Windows\\System32"; + + WCHAR install_path[MAX_PATH] = { 0 }; + if (!get_install_path(install_path, _countof(install_path))) + { + /* if we cannot find installation path from the registry, + * use Windows directory as a fallback + */ + openvpn_swprintf(install_path, _countof(install_path), L"%ls", ssl_fallback_dir); + } + + if ((install_path[wcslen(install_path) - 1]) == L'\\') + { + install_path[wcslen(install_path) - 1] = L'\0'; + } + + static struct { + WCHAR* name; + WCHAR* value; + } ossl_env[] = { + {L"OPENSSL_CONF", L"openssl.cnf"}, + {L"OPENSSL_ENGINES", L"engines"}, + {L"OPENSSL_MODULES", L"modules"} + }; + + for (size_t i = 0; i < SIZE(ossl_env); ++i) + { + size_t size = 0; + + _wgetenv_s(&size, NULL, 0, ossl_env[i].name); + if (size == 0) + { + WCHAR val[MAX_PATH] = {0}; + openvpn_swprintf(val, _countof(val), L"%ls\\ssl\\%ls", install_path, ossl_env[i].value); + _wputenv_s(ossl_env[i].name, val); + } + } +} + #endif /* ifdef _WIN32 */ diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h index 5d3371a0..4a992d91 100644 --- a/src/openvpn/win32.h +++ b/src/openvpn/win32.h @@ -327,7 +327,13 @@ bool send_msg_iservice(HANDLE pipe, const void *data, size_t size, int openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags); -bool impersonate_as_system(); +/* + * openvpn_swprintf() is currently only used by Windows code paths + * and when enabled for all platforms it will currently break older + * OpenBSD versions lacking vswprintf(3) support in their libc. + */ +bool +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...); #endif /* ifndef OPENVPN_WIN32_H */ #endif /* ifdef _WIN32 */ -- 2.23.0.windows.1 

Hi, On Tue, Nov 23, 2021 at 1:37 PM Lev Stipakov <lst...@gm...> wrote: > I don't have a setup to properly test it, like actually loading the > config - I only checked that the openvpn.exe attempted to access > openssl.cnf at the correct location. > > If someone wants to test - binary artifacts could be found here: > https://github.com/lstipakov/openvpn/actions/runs/1496114596 > > I could also do testing if someone educates me how :) > Try using an openssl.cnf like the one below which restricts signature algorithms to a some non-PSS schemes. Change that line to restrict them further or comment out to use defaults: not including PSS will force non-PSS signature with TLS 1.2 even with OpenSSL 1.1.1 server. And will break TLS 1.3 negotiation. Removing ECC signatures and using an EC key certificate will break the connection etc.. # # OpenSSL configuration file to restrict siglags during handshake # openssl_conf = default_conf [default_conf] ssl_conf = ssl_sect [ssl_sect] system_default = system_default_sect [system_default_sect] #MinProtocol = TLSv1.2 #CipherString = DEFAULT@SECLEVEL=0 SignatureAlgorithms = RSA+SHA256:RSA+SHA384:RSA+SHA512:ECDSA+SHA256:ECDSA+SHA384:ECDSA+SHA512:RSA+SHA1:ECDSA+SHA1 # possible values # PKCS1: rsa_pkcs1_sha256:rsa_pkcs1_sha384:rsa_pkcs1_sha512 # ECDSA: ecdsa_secp256r1_sha256:ecdsa_secp384r1_sha384:ecdsa_secp521r1_sha512 # PSS with rsa encryption public key rsa_pss_rsae_sha256:rsa_pss_rsae_sha384:rsa_pss_rsae_sha512 # EdDSA :ed25519:ed448 # PSS with PSS public key: rsa_pss_pss_sha256:rsa_pss_pss_sha384:rsa_pss_pss_sha512 # Legacy rsa_pkcs1_sha1:ecdsa_sha1 

On Tue, Nov 23, 2021 at 1:46 PM Gert Doering <ge...@gr...> wrote: > Hi, > > On Fri, Nov 19, 2021 at 02:53:06AM +0200, Lev Stipakov wrote: > > + if ((install_path[wcslen(install_path) - 1]) == L'\\') > > + { > > + install_path[wcslen(install_path) - 1] = L'\0'; > > + } > > + > > + WCHAR openssl_cnf[MAX_PATH] = {0}; > > + WCHAR openssl_engines[MAX_PATH] = {0}; > > + WCHAR openssl_modules[MAX_PATH] = {0}; > > + > > + openvpn_swprintf(openssl_cnf, _countof(install_path), > > + L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path); > > This needs to be _countof(openssl_cnf) - even if they are the same > today, they might not be tomorrow. > > While at it, I wonder if it is more orderly to move the swprintf() > calls in the "if NULL" clause now... like this: > > if (_wgetenv(L"OPENSSL_CONF") == NULL) > { > WCHAR openssl_cnf[MAX_PATH] = {0}; > > openvpn_swprintf(openssl_cnf, _countof(openssl_cnf), > L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path); > _wputenv(openssl_cnf); > } > > (I would not have brought this up for a v4, but the _countof() needs > to be fixed anyway) > If you are doing a v4 you may want to consider: static struct { wchar_t *name; wchar_t *value; } ossl_env[] = {{L"OPENSSL_CNF", L"openssl.cnf"}, {"OPENSSL_ENGINES", L"engines"}, {..}}; and use a loop. Less local arrays, easy to add more to the env zoo later etc.. Just saying --- I'm okay with the current style too. > Regarding Selva's comment on the scope of the memory passed to _wputenv(), > I checked MS documentation, and it does not say anything. > MS docs say _putenv is their implementation of POSIX putenv but the latter does imply the pointer supplied by the user should be used as is. Anyway, if it works with automatics, good for us. > > OTOH, it points to _wputenv_s(varname,string) which might be worth > considering... > +1 to that especially if MSVC is planning to deprecate _wputenv as they have already done for a host of other such functions. Selva 

Hi, On Fri, Nov 19, 2021 at 02:53:06AM +0200, Lev Stipakov wrote: > + if ((install_path[wcslen(install_path) - 1]) == L'\\') > + { > + install_path[wcslen(install_path) - 1] = L'\0'; > + } > + > + WCHAR openssl_cnf[MAX_PATH] = {0}; > + WCHAR openssl_engines[MAX_PATH] = {0}; > + WCHAR openssl_modules[MAX_PATH] = {0}; > + > + openvpn_swprintf(openssl_cnf, _countof(install_path), > + L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path); This needs to be _countof(openssl_cnf) - even if they are the same today, they might not be tomorrow. While at it, I wonder if it is more orderly to move the swprintf() calls in the "if NULL" clause now... like this: if (_wgetenv(L"OPENSSL_CONF") == NULL) { WCHAR openssl_cnf[MAX_PATH] = {0}; openvpn_swprintf(openssl_cnf, _countof(openssl_cnf), L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path); _wputenv(openssl_cnf); } (I would not have brought this up for a v4, but the _countof() needs to be fixed anyway) Regarding Selva's comment on the scope of the memory passed to _wputenv(), I checked MS documentation, and it does not say anything. OTOH, it points to _wputenv_s(varname,string) which might be worth considering... https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/putenv-s-wputenv-s?view=msvc-170 gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany ge...@gr... 

I don't have a setup to properly test it, like actually loading the config - I only checked that the openvpn.exe attempted to access openssl.cnf at the correct location. If someone wants to test - binary artifacts could be found here: https://github.com/lstipakov/openvpn/actions/runs/1496114596 I could also do testing if someone educates me how :) ti 23. marrask. 2021 klo 20.27 Lev Stipakov (lst...@gm...) kirjoitti: > > From: Lev Stipakov <le...@op...> > > Commits > > - 92535b6 ("contrib/vcpkg-ports: add openssl port with --no-autoload-config option set (CVE-2121-3606)") > - 447cfb4 ("crypto_openssl.c: disable explicit initialization on Windows (CVE-2121-3606)") > > disabled OpenSSL config loading functionality, which could be > exploited by loading config from untrusted locations. > > However, ability to load config might be useful for some users. > > This brings config loading back and sets OpenSSL enviroment variables > > OPENSSL_CONF, OPENSSL_ENGINES, OPENSSL_MODULES > > which are used to load config, engines and modules, to a trusted location. > The location is constructed based on installation path, read from registry on startup. > If installation path cannot be read, Windows\System32 is used as a fallback. > > While on it, remove unused "bool impersonate_as_system();" declaration. > > Signed-off-by: Lev Stipakov <le...@op...> > --- > > v3: > - do not assume that installation path ends with directory separator > - set enviroment variables only if they're not already set > - bring back explicit initialization on Windows (might be needed on > some cases) > - slightly revamp commit message > > v2: > - add missing "static" modifier to set_openssl_env_vars() declaration > spotted by gcc > > .../vcpkg-triplets/arm64-windows-ovpn.cmake | 2 - > contrib/vcpkg-triplets/x64-windows-ovpn.cmake | 2 - > contrib/vcpkg-triplets/x86-windows-ovpn.cmake | 2 - > src/openvpn/buffer.c | 23 ----- > src/openvpn/crypto_openssl.c | 2 - > src/openvpn/win32.c | 92 +++++++++++++++++++ > src/openvpn/win32.h | 8 +- > 7 files changed, 99 insertions(+), 32 deletions(-) > > diff --git a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > index 89f6a279..dd3c6c0a 100644 > --- a/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > +++ b/contrib/vcpkg-triplets/arm64-windows-ovpn.cmake > @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) > if(PORT STREQUAL "lz4") > set(VCPKG_LIBRARY_LINKAGE static) > endif() > - > -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) > diff --git a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > index d860eed6..7036ed2d 100644 > --- a/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > +++ b/contrib/vcpkg-triplets/x64-windows-ovpn.cmake > @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) > if(PORT STREQUAL "lz4") > set(VCPKG_LIBRARY_LINKAGE static) > endif() > - > -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) > diff --git a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > index c1ea6ef3..7d3bf340 100644 > --- a/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > +++ b/contrib/vcpkg-triplets/x86-windows-ovpn.cmake > @@ -5,5 +5,3 @@ set(VCPKG_LIBRARY_LINKAGE dynamic) > if(PORT STREQUAL "lz4") > set(VCPKG_LIBRARY_LINKAGE static) > endif() > - > -set(OPENSSL_NO_AUTOLOAD_CONFIG ON) > diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c > index c82d3d4d..54e758af 100644 > --- a/src/openvpn/buffer.c > +++ b/src/openvpn/buffer.c > @@ -310,29 +310,6 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...) > return (len >= 0 && len < size); > } > > -/* > - * openvpn_swprintf() is currently only used by Windows code paths > - * and when enabled for all platforms it will currently break older > - * OpenBSD versions lacking vswprintf(3) support in their libc. > - */ > - > -#ifdef _WIN32 > -bool > -openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...) > -{ > - va_list arglist; > - int len = -1; > - if (size > 0) > - { > - va_start(arglist, format); > - len = vswprintf(str, size, format, arglist); > - va_end(arglist); > - str[size - 1] = L'\0'; > - } > - return (len >= 0 && len < size); > -} > -#endif > - > /* > * write a string to the end of a buffer that was > * truncated by buf_printf > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index c9dc9d0a..ef520928 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -154,13 +154,11 @@ crypto_init_lib_engine(const char *engine_name) > void > crypto_init_lib(void) > { > -#ifndef _WIN32 > #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) > OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL); > #else > OPENSSL_config(NULL); > #endif > -#endif /* _WIN32 */ > /* > * If you build the OpenSSL library and OpenVPN with > * CRYPTO_MDEBUG, you will get a listing of OpenSSL > diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c > index 6cff17b2..ac7775e4 100644 > --- a/src/openvpn/win32.c > +++ b/src/openvpn/win32.c > @@ -101,6 +101,12 @@ struct semaphore netcmd_semaphore; /* GLOBAL */ > */ > static char *win_sys_path = NULL; /* GLOBAL */ > > +/** > + * Set OpenSSL environment variables to a safe directory > + */ > +static void > +set_openssl_env_vars(); > + > void > init_win32(void) > { > @@ -110,6 +116,8 @@ init_win32(void) > } > window_title_clear(&window_title); > win32_signal_clear(&win32_signal); > + > + set_openssl_env_vars(); > } > > void > @@ -1509,4 +1517,88 @@ send_msg_iservice(HANDLE pipe, const void *data, size_t size, > return ret; > } > > +bool > +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...) > +{ > + va_list arglist; > + int len = -1; > + if (size > 0) > + { > + va_start(arglist, format); > + len = vswprintf(str, size, format, arglist); > + va_end(arglist); > + str[size - 1] = L'\0'; > + } > + return (len >= 0 && len < size); > +} > + > +static BOOL > +get_install_path(WCHAR *path, DWORD size) > +{ > + WCHAR reg_path[256]; > + HKEY key; > + BOOL res = FALSE; > + openvpn_swprintf(reg_path, _countof(reg_path), L"SOFTWARE\\" PACKAGE_NAME); > + > + LONG status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &key); > + if (status != ERROR_SUCCESS) > + { > + return res; > + } > + > + /* The default value of REG_KEY is the install path */ > + status = RegGetValueW(key, NULL, NULL, RRF_RT_REG_SZ, NULL, (LPBYTE)path, &size); > + res = status == ERROR_SUCCESS; > + > + RegCloseKey(key); > + > + return res; > +} > + > +static void > +set_openssl_env_vars() > +{ > + const WCHAR* ssl_fallback_dir = L"C:\\Windows\\System32"; > + > + WCHAR install_path[MAX_PATH] = { 0 }; > + if (!get_install_path(install_path, _countof(install_path))) > + { > + /* if we cannot find installation path from the registry, > + * use Windows directory as a fallback > + */ > + openvpn_swprintf(install_path, _countof(install_path), L"%ls", ssl_fallback_dir); > + } > + > + if ((install_path[wcslen(install_path) - 1]) == L'\\') > + { > + install_path[wcslen(install_path) - 1] = L'\0'; > + } > + > + WCHAR openssl_cnf[MAX_PATH] = {0}; > + WCHAR openssl_engines[MAX_PATH] = {0}; > + WCHAR openssl_modules[MAX_PATH] = {0}; > + > + openvpn_swprintf(openssl_cnf, _countof(install_path), > + L"OPENSSL_CONF=%ls\\ssl\\openssl.cnf", install_path); > + openvpn_swprintf(openssl_engines, _countof(openssl_engines), > + L"OPENSSL_ENGINES=%ls\\ssl\\engines", install_path); > + openvpn_swprintf(openssl_modules, _countof(openssl_modules), > + L"OPENSSL_MODULES=%ls\\ssl\\modules", install_path); > + > + if (_wgetenv(L"OPENSSL_CONF") == NULL) > + { > + _wputenv(openssl_cnf); > + } > + > + if (_wgetenv(L"OPENSSL_ENGINES") == NULL) > + { > + _wputenv(openssl_engines); > + } > + > + if (_wgetenv(L"OPENSSL_MODULES") == NULL) > + { > + _wputenv(openssl_modules); > + } > +} > + > #endif /* ifdef _WIN32 */ > diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h > index 5d3371a0..4a992d91 100644 > --- a/src/openvpn/win32.h > +++ b/src/openvpn/win32.h > @@ -327,7 +327,13 @@ bool send_msg_iservice(HANDLE pipe, const void *data, size_t size, > int > openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags); > > -bool impersonate_as_system(); > +/* > + * openvpn_swprintf() is currently only used by Windows code paths > + * and when enabled for all platforms it will currently break older > + * OpenBSD versions lacking vswprintf(3) support in their libc. > + */ > +bool > +openvpn_swprintf(wchar_t* const str, const size_t size, const wchar_t* const format, ...); > > #endif /* ifndef OPENVPN_WIN32_H */ > #endif /* ifdef _WIN32 */ > -- > 2.23.0.windows.1 > -- -Lev