Re: [Openvpn-devel] [PATCH v11] Implement support for larger packet counter sizes
Hi, TL;DR: I don't think this should be merged yet. My primary concern is that we don't have any means to limit key usage to a safe value. I raised this concern back in December 2023: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27791.html If we allow for packet counter of 64 bits, we would allow users to exceed the safe limits determined for AES-GCM in the context of TLS. As far as I can tell, we don't have a reason to allow for looser limits. So before this patch goes in, I really think we should discuss what those limits should be for OpenVPN *and* enforce them. This might have been overlooked, because attention was drawn to my proposal to make the upper bits "implicit" in the same mail. On 10-09-2024 18:34, Gert Doering wrote: From: Arne Schwabe With DCO and possible future hardware assisted OpenVPN acceleration we are approaching the point where 32 bit IVs are not cutting it any more. s/IVs/packet counters/. See my mail from Dec 2023. To illustrate the problem, some back of the envelope math here: If we want to keep the current 3600s renegotiation interval and have a safety margin of 25% (when we trigger renegotiation) we have about 3.2 million packets (2*32 * 0.7) to work with. That translates to about 835k packets per second. With 1300 Byte packets that translates into 8-9 Gbit/s. That is far from unrealistic any more. Current DCO implementations are already in spitting distance to that or might even reach (for a single client connection) that if you have extremely fast single core performance CPU. This introduces the 64bit packet counters for AEAD data channel ciphers in TLS mode ciphers. No effort has been made to support larger packet counters in any other scenario since those are all legacy. Note that for AES-GCM, assuming limits similar to TLS, we likely won't be able to postpone key refresh for much longer than we currently do. For ChaCha-Poly we can, because of the larger auth tag. So if we want to improve things for AES-GCM, we probably need other optimizations. I have some ideas, but was hoping to do some research and a write-up during the train ride to the hackathon, so we could discuss it further in Karlsruhe. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Fix IPv6 route add/delete message log level
We have D_ROUTE for route addition/deletion messages, which prints at loglevel 3. Use that for IPv6, like we do for IPv4 to reduce terminal spam for non-legacy-networking setups. Prvious code would print the messages at --verb 1. Signed-off-by: Steffan Karger --- src/openvpn/route.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 2f472a1f..e784782b 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1940,10 +1940,10 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, #endif #ifndef _WIN32 -msg( M_INFO, "add_route_ipv6(%s/%d -> %s metric %d) dev %s", +msg(D_ROUTE, "add_route_ipv6(%s/%d -> %s metric %d) dev %s", network, r6->netbits, gateway, r6->metric, device ); #else -msg( M_INFO, "add_route_ipv6(%s/%d -> %s metric %d) IF %lu", +msg(D_ROUTE, "add_route_ipv6(%s/%d -> %s metric %d) IF %lu", network, r6->netbits, gateway, r6->metric, r6->adapter_index ? r6->adapter_index : tt->adapter_index); #endif @@ -2394,7 +2394,7 @@ delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt, } #endif -msg( M_INFO, "delete_route_ipv6(%s/%d)", network, r6->netbits ); +msg(D_ROUTE, "delete_route_ipv6(%s/%d)", network, r6->netbits ); #if defined(TARGET_LINUX) int metric = -1; -- 2.34.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] OpenVPN data channel format using 64bit IV
Hi, I've been just lurking for a while, but you've managed to nerd-snipe me in responding. On 11-12-2023 13:31, Arne Schwabe wrote: with DCO and possible future hardware assisted OpenVPN acceleration we are approaching the point where 32 bit IVs are not cutting it any more. Agreed. Though to be precise, we use 96-bit IVs in our AEAD mode, which include a 32-bit packet counter. To illustrate the problem, some back of the envelope math here: If we want to keep the current 3600s renogotiation interval and have a safety margin of 30% we have about 3 million packets (2*32 * 0.7) to work with. That translates to about 835k packets per second. Nitpicking, but we currently maintain a reneg threshold at 75% (pid >= 0xFF00). With 1300 Byte packets that translates into 8-9 Gbit/s. That is from unrealistic any more. Current DCO implementations are already in spitting distance to that or might even reach (for a single client connection) that if you have extremely fast single core performance CPU. Agreed. Though it seems to me that with so much processing power and network throughput, it wouldn't be much of an issue to renegotiate slightly more often. So I think we need to consider adding 64bit IV now rather than later. Even considering my remarks above, I do agree it's time. But, we will also need to make sure that we won't exceed the limits of the ciphers we use, which might need additional logic. I don't have time right now to dig up all the specifics, but this IRTF draft might serve as a good starting point for reading: https://www.ietf.org/archive/id/draft-irtf-cfrg-aead-limits-07.html So the proposal is the following: - add IV_PACKET_FORMAT_AEAD_V2 flag to the protocol flags. This signal that the other side supports the new AEAD data channel packet format that supports 64 bit IVs. I don't think we need to extend this feature to the CBC data channel format. I just don't see a use case where people would be able to upgrade to a new OpenVPN version to use 64 bit IVs but not to also change to use AEAD ciphers. Ack. Do note that CBC has a random IV per packet, so this doesn't apply to the IV in CBC, just the packet counter. - add protocol-flag aead-packet-format-v2 This signals the client to switch to the new data channel format. And finally have the data channel format. Since this format is negotiated like the cipher, there is no need to use another opcode if keep the peer id to just 24 bit. But we might want to extend the format to have it 8 byte aligned to also allow peer-id to be extended in the future. I'd at least consider to not send the upper 32 bits of the counter over the wire. With some simple arithmetic you can detect the counter overflow and keep the high bits internal to the application. I'd expect such per-packet arithmetic to be much cheaper than transmitting 4 bytes extra for each packet. If you take that approach, you would not even need to change the wire format. Even more, you might not even have to negotiate the option with the peer, because the peer will initiate a renegotiation after 0xFF00 packets if it doesn't support the "implicit long PID". New peers will postpone the reneg to 0xFF00 packets. Regardless of what choice we take, this is a good opportunity to rectify the position of the AEAD tag in our packet. Especially for hardware implementations it is quite advantageous to have the AEAD at the end of the packet instead of the beginning and since we need to have a new data format, there is no reason to keep the tag at the start of the packet. Agreed (as I already stated in 2015: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg09879.html). So the choice is basically Variant A: Bytes 1 234 5-13 13- (n+13) (n+14) - (n+30) [OP_CODE_DATA_V2][peerid] [64 bit IV] [ payload ][ 16 byte auth tag] Variant B: 1 3-8 [OP_CODE_DATA_V3] [ peer-id/padding] [rest identical] or: Variant C: Leave the on-the-wire PID as-is. Just move the auth tag to the end of the packet. variant D: Keep wire format as-is. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [RFC PATCH] Add connection test suite
Add a local connection test suite, that is able to set up connections
and verify in the process output that the process believes the
connection was successful.
This is not a replacement for the t_client test suite, but rather a
fast, simple, local suite to validate .e.g control channel setup. It can
replace t_cltsrv.sh (as a much faster alternative), but does require
python3 and pytest to run.
Signed-off-by: Steffan Karger
---
This is an RFC to get comments on whether you believe this approach is a
useful addition to our current test suite. We can add many more useful
tests once we agree on the approach.
tests/Makefile.am | 6 +-
tests/connection_tests/test_examples.py | 303
tests/t_connection.sh | 24 ++
3 files changed, 331 insertions(+), 2 deletions(-)
create mode 100644 tests/connection_tests/test_examples.py
create mode 100755 tests/t_connection.sh
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 89180f60..96b8565b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,12 +14,14 @@ MAINTAINERCLEANFILES = \
SUBDIRS = unit_tests
-test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
+test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh t_connection.sh
if HAVE_SITNL
test_scripts += t_net.sh
endif
-TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
+TESTS_ENVIRONMENT = \
+ top_builddir="$(top_builddir)" \
+ top_srcdir="$(top_srcdir)"
TESTS = $(test_scripts)
dist_noinst_SCRIPTS = \
diff --git a/tests/connection_tests/test_examples.py
b/tests/connection_tests/test_examples.py
new file mode 100644
index ..030fa21b
--- /dev/null
+++ b/tests/connection_tests/test_examples.py
@@ -0,0 +1,303 @@
+import os
+import pytest
+import re
+import subprocess
+import tempfile
+import threading
+import time
+
+from pathlib import Path
+
+OPENVPN = Path(os.environ["OPENVPN_BINARY"])
+CD_PATH = Path(os.environ["WORK_DIR"])
+
+
+class BasicOption:
+def __init__(self, name, *args):
+self.name = name
+self.value = " ".join(args)
+
+def toconfigfileitem(self):
+return f"{self.name} {self.value}\n"
+
+
+class InlineOption(BasicOption):
+def __init__(self, name, *args, infile=None):
+self.name = name
+if infile is not None:
+path = Path(infile)
+if not path.is_absolute():
+path = CD_PATH / infile
+self.value = open(path).read()
+else:
+self.value = " ".join(args)
+
+def toconfigfileitem(self):
+return f"<{self.name}>\n{self.value.strip()}\n".strip()
+
+
+class OpenVPNConfig:
+DEFAULT_CONFIG_BASE = [
+BasicOption("dev", "null"),
+BasicOption("local", "localhost"),
+BasicOption("remote", "localhost"),
+BasicOption("verb", "3"),
+BasicOption("reneg-sec", "10"),
+BasicOption("ping", "1"),
+BasicOption("cd", str(CD_PATH)),
+BasicOption("ca", "sample-keys/ca.crt"),
+]
+DEFAULT_SERVER_PORT = "16010"
+DEFAULT_CLIENT_PORT = "16011"
+
+def __init__(self, name="OpenVPN", options=DEFAULT_CONFIG_BASE,
extra_options=[]):
+self.options = options + extra_options
+self.name = name
+
+def toconfigfile(self):
+c = tempfile.NamedTemporaryFile(mode="w+")
+for option in self.options:
+c.write(option.toconfigfileitem() + "\n")
+c.flush()
+return c
+
+
+class ServerConfig(OpenVPNConfig):
+DEFAULT_SERVER_OPTIONS = [
+BasicOption("lport", OpenVPNConfig.DEFAULT_SERVER_PORT),
+BasicOption("rport", OpenVPNConfig.DEFAULT_CLIENT_PORT),
+BasicOption("tls-server"),
+BasicOption("dh", "none"),
+BasicOption("key", "sample-keys/server.key"),
+BasicOption("cert", "sample-keys/server.crt"),
+]
+
+def __init__(self, name="Server", extra_options=[]):
+super().__init__(name=name, extra_options=self.DEFAULT_SERVER_OPTIONS)
+
+self.options += extra_options
+
+
+class ClientConfig(OpenVPNConfig):
+DEFAULT_SERVER_OPTIONS = [
+BasicOption("lport", OpenVPNConfig.DEFAULT_CLIENT_PORT),
+BasicOption("rport", OpenVPNConfig.DEFAULT_SERVER_PORT),
+BasicOption("tls-client"),
+BasicOption("remote-cert-tls", "server"),
+BasicOption("key", "sample-keys/client.key"),
+BasicOption("cert", "sample-keys/client.crt"),
+]
+
+def __init__(self, name="Client", extra_
Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges
Hi, On Mon, 15 Aug 2022 at 12:50, Gert Doering wrote: > On Mon, Aug 15, 2022 at 12:40:55PM +0200, Timo Rothenpieler wrote: > > or don't even try to retain capabilities, since > > they're not needed either way. I'd prefer the later, since fewer > > capabilities is generally better. > > I could see arguments for "we want to do the ifconfig/route setup in > an --up script" - for example to do VRF/NetNS stuff that OpenVPN can not > do itself. So for these scenarios having the capability around would > be useful (= thus, try-and-warn)... > > Different scenario, same options. We don't always know what users want. Let me just second the "fewer capabilities is generally better" argument. CAP_NET_ADMIN is a broad set of effective capabilties and has been a popular path for privilege escalation vulnerabilities in the past. See for example these two recent CVEs: CVE-2022-2586 A use-after-free in the Netfilter subsystem may result in local privilege escalation for a user with the CAP_NET_ADMIN capability in any user or network namespace. CVE-2022-2588 Zhenpeng Lin discovered a use-after-free flaw in the cls_route filter implementation which may result in local privilege escalation for a user with the CAP_NET_ADMIN capability in any user or network namespace. So I'm really not in favour of retaining CAP_NET_ADMIN "just in case". I would even like to be able to not retain it at all. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Decouple MSS fix calculation from frame calculation
Hi, On 30-12-2021 18:28, Arne Schwabe wrote: > That BF-CBC seems have an extra 8 bytes that I somehow missed. CBC is a > odd since it always gives you a multiple of the blocksize (64 bit or 8 > byte) and if you evenly divide by the blocksize you get an extra block > just for the padding. I need to reinvestigate that code and send a fixup > patch for it. You probably know this, but for clarity: this is how CBC padding works, not just for BF. It is easier to trigger with BF though, because of the smaller (64-bit) block, compared to AES (128-bit block). -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Remove custom PRNG function
Hi,
On 07-11-2021 10:01, Arne Schwabe wrote:
> Remove the custom PRNG from OpenVPN and instead rely always on the random
> number generator from the SSL library. The only place that this is in a
> performance critical place is the CBC IV generation. Even with that in mind
> a micro benchmark shows no significant enough change with OpenSSL 3.0:
>
>
> Benchmark Time CPU Iterations
>
> BM_OpenSSL_RAND 842 ns 842 ns 753401
> BM_OpenVPN_RAND 743 ns 743 ns 826690
> BM_Encrypt_AES_CBC_dummy1044 ns 1044 ns 631530
> BM_Encrypt_AES_CBC_RAND_bytes 1892 ns 1891 ns 346566
> BM_Encrypt_AES_CBC_prng_bytes 1818 ns 1817 ns 373970
>
> (source https://gist.github.com/schwabe/029dc5e5a690df8e2e3f774a13ec7bce)
Feature-ACK. The performance of the PRNGs once was much larger, *and*
OpenVPN has moved along from CBC mode to (AES-)GCM. So there's not much
reason left to keep our own prng implementation.
> Signed-off-by: Arne Schwabe
> ---
> Changes.rst | 6 ++
> doc/man-sections/advanced-options.rst | 17 --
> src/openvpn/crypto.c | 88 +--
> src/openvpn/crypto.h | 20 --
> src/openvpn/init.c| 30 -
> src/openvpn/options.c | 30 +
> src/openvpn/options.h | 2 -
> src/openvpn/ps.c | 5 +-
> src/openvpn/ssl.c | 1 -
> 9 files changed, 9 insertions(+), 190 deletions(-)
>
> diff --git a/Changes.rst b/Changes.rst
> index b08bff3d7..174e233c8 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -94,6 +94,11 @@ TLS 1.0 and 1.1 are deprecated
> Should backwards compatibility with older OpenVPN peers be
> required, please see the ``--compat-mode`` instead.
>
> +``--prng`` has beeen removed
> +OpenVPN used to implement its own PRNG based on a hash. However
> implementing
> +a PRNG is better left to a crypto library. So we use mbed TLS or OpenSSL
> +PRNG instead now.
That last sentence doesn't read well. Suggestion: "So we use the PRNG
from mbed TLS or OpenSSL now."
> void
> prng_bytes(uint8_t *output, int len)
> {
> -static size_t processed = 0;
> -
> -if (nonce_md)
> -{
> -const int md_size = md_kt_size(nonce_md);
> -while (len > 0)
> -{
> -const int blen = min_int(len, md_size);
> -md_full(nonce_md, nonce_data, md_size + nonce_secret_len,
> nonce_data);
> -memcpy(output, nonce_data, blen);
> -output += blen;
> -len -= blen;
> -
> -/* Ensure that random data is reset regularly */
> -processed += blen;
> -if (processed > PRNG_NONCE_RESET_BYTES)
> -{
> -prng_reset_nonce();
> -processed = 0;
> -}
> -}
> -}
> -else
> -{
> -ASSERT(rand_bytes(output, len));
> -}
> +ASSERT(rand_bytes(output, len));
> }
Hmm, this leaves just this tiny wrapper. Why not just remove that too,
and just use ASSERT(rand_bytes()) in the callers? (I can live with the
wrapper too, if you prefer to keep it.)
> diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
> index a61176172..a0f8a00e9 100644
> --- a/src/openvpn/ps.c
> +++ b/src/openvpn/ps.c
> @@ -912,10 +912,7 @@ port_share_open(const char *host,
>
> /* no blocking on control channel back to parent */
> set_nonblock(fd[1]);
> -
> - /* initialize prng */
> -prng_init(NULL, 0);
> -
> +
> /* execute the event loop */
Trailing whitespace inserted.
Other from these details, this looks good to me. As long as the typos
and whitespace is fixed before committing:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3 05/21] [OSSL 3.0] Use EVP_PKEY based API for loading DH keys
Hi,
Just a nit:
On 19-10-2021 20:31, Arne Schwabe wrote:
> +if (!SSL_CTX_set0_tmp_dh_pkey(ctx->ctx, dh))
> +{
> +crypto_msg(M_FATAL, "SSL_CTX_set_tmp_dh");
> +}
This error message looks incorrect and incomplete.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Update Fox e-mail address in copyright notices
Hi, On Fri, 2 Jul 2021 at 12:12, Gert Doering wrote: > On Thu, Jul 01, 2021 at 07:14:58PM +0200, Max Fillinger wrote: > > Replace open...@fox-it.com with open...@foxcrypto.com. > > > > Signed-off-by: Max Fillinger > > Generally speaking, this is fine, I just have a small issue with > "who can reasonably ACK this". Can you poke Steffan to approve this > (as "he introduced a significant number of these", so he's a natural > authority on "yes, this makes sense to change to the new corp mail")? Yes, this makes sense to change to the new corp mail. :-) Haven't looked in detail at the patch itself, but changing these to @foxcrypto.com is the right thing to do. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Change CTR DRBG update function call to new mbedtls 2.16.0 API
Hi,
On 06-04-2021 12:55, Maximilian Fillinger wrote:
>> Am 02.04.21 um 15:26 schrieb Max Fillinger:
>>> From: Uipko Berghuis
>>>
>>> In mbedtls 2.16.0 mbedtls_ctr_drbg_update() changed to
>>> mbedtls_ctr_drbg_update_ret(). Change the function name and handle the
>>> new return value error code.
>>> ---
>>> src/openvpn/ssl_mbedtls.c | 5 -
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
>>> index 5d7af351..56e9f045 100644
>>> --- a/src/openvpn/ssl_mbedtls.c
>>> +++ b/src/openvpn/ssl_mbedtls.c
>>> @@ -950,7 +950,10 @@ tls_ctx_personalise_random(struct tls_root_ctx
>>> *ctx)
>>>
>>> if (0 != memcmp(old_sha256_hash, sha256_hash,
>> sizeof(sha256_hash)))
>>> {
>>> -mbedtls_ctr_drbg_update(cd_ctx, sha256_hash, 32);
>>> +if (!mbed_ok(mbedtls_ctr_drbg_update_ret(cd_ctx,
>> sha256_hash, 32)))
>>> +{
>>> +msg(M_WARN, "WARNING: failed to personalise random,
>> could not update CTR_DRBG");
>>> +}
>>> memcpy(old_sha256_hash, sha256_hash,
>> sizeof(old_sha256_hash));
>>> }
>>> }
>>>
>>
>> This change will break compilation with anything that is < 2.16.0.
>
> This function is deprecated in 2.16. I don't mind keeping this change to
> OpenVPN-NL for now, but for future reference, what's the best solution
> when a new version of mbedtls removes the function?
I'd say add a compat-wrapper, like we have many for openssl. Possibly in
compat-mbedtls.h (mimicing the openssl code) or just in crypto_mbedtls.h
if we don't have many. Something like (untested/"pseudo"code):
#if MBEDTLS_VERSION < 2.16
static inline int mbedtls_ctr_drbg_update_ret(ctx, h, len)
{
mbedtls_ctr_drbg_update(ctx, h, len);
return 0;
}
#endif
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Remove --no-replay
Hi, On 07-04-2021 17:50, Antonio Quartulli wrote: > On 26/07/2020 15:31, Arne Schwabe wrote: >> Am 26.07.20 um 02:01 schrieb Arne Schwabe: >>> Am 17.07.20 um 19:10 schrieb David Sommerseth: The --no-replay feature is considered to be a security weakness, which was also highlighed during the OpenVPN 2.4 security audit [0]. This option was added to the DeprecatedOptions[1] list and has been reported as deprecated since OpenVPN 2.4. >>> >>> As a side note, removing this feature weakens the ability to use OpenVPN >>> is a pure tunnel without crypto (--auth none, --cipher none and >>> no-replay) since this removes the ability to disable replay proctection >>> when no authentication is enabled. (replay protection without auth is >>> silly as a attacker can just fake the replay id too.) >>> >>> Acked-By: Arne Schwabe >> >> I given that a bit of a thought. But we need to decide if we to support >> unencrypted transport only session or not in future. If we do not want >> to support them, then applying this patch is fine, otherwise we should >> restrict disabling no-replay to --auth none and also --auth none to >> --cipher none basically: >> >> --cipher != none => auth none and no-replay forbidden >> >> --cipher == none => allows auth none and also no-replay >> >> --cipher none and auth none, warn if no-replay is used that it does not >> prevent replay attacks. But do not fail since we would break a lot of >> setups. > > I work for the ministry of oversimplification and I think that removing > user knobs is simply a good thing. > > Following the logic provided by Arne, how about removing the --no-reply > knob and making this mechanism automatic? > > * if cipher != none -> replay prevention is always enabled; > * if cipher == none && auth == none -> replay prevention is disabled. > > > [allowing or disabling auth=none should be tackled saparately imho] Before getting into solutions, let me state a few facts: 1. Replay protection only really works when authentication is enabled. 2. Our AEAD modes all require replay protection. 3. Automatically disabling replay protection will cause a config backward compatibility break. Given 1, if we were to couple allowing --no-replay to some crypto settings, I would say it should be coupled to --auth none. Given 2, how clear is our timeline on sunsetting non-AEAD ciphers? That would automatically sunset --no-replay. (I've lost track a bit...) Point 3 should not really be a problem, because --no-replay has been deprecated since 2.4 (2017), and is an edge use-case anyway. But it should be a conscious decision. So, unless we're about to sunset non-AEAD ciphers soon (which would automatically solve the issue), I would suggest a small variation to Antonio's approach: Remove the option, and: * if auth != none -> replay prevention is always enabled; * if auth == none -> replay prevention is disabled. (Of course, considering that auth != none for AEAD cipher modes, even when --auth none is set.) -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: reliable: retransmit if 3 follow-up ACKs are received
Hi, On 02-04-2021 20:16, Gert Doering wrote: > Your patch has been applied to the master branch. > > I have never looked into this reliable stuff before, and do not have > a test environment with a) significant amounts of control plane traffic, > and b) "just the right amount" of packet loss. So I've just stared at > the code a bit (seems reasonable) and tested client side on Linux > (works). Thanks! > I do wonder a bit under which condition this could fire - with the > larger control channel packets we introduced a few years ago, I do > not see many scenarios where "more than 3 packets" could be in-flight > at the same time... inital TLS handshake with very large DH groups? This was written in the context of our post-quantum crypto experiments with OpenVPN. Post-quantum (pub)keys are typically larger than traditional keys. So yes, large keys / paramters is one example. Another one would be someone pushing large amounts of routes. I recall ValikSS had such a use case. I've also seen companies push a lot of routers to have high-volume traffic (e.g. video conferencing, cloud storage) bypass the VPN. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] make --persist-key always-on and remove "off" code path
Hi,
On 10-04-2021 01:42, Arne Schwabe wrote:
>
> Am 09.04.2021 um 18:28 schrieb Gert Doering:
>> Hi,
>>
>> there was a big discussion on the IRC channel today about interactions
>> between "--chroot" and "--persist-key" and how and when stuff is reloaded
>> or not.
>>
>> Now, we all seem to agree that OpenVPN has way too many obscure options,
>> so I propose to get rid of another one, namely --persist-key - and I
>> suggest to make it permanently-active ("load the keys at startup, and
>> then do not touch these files again").
>>
>> Unless someone explains to me in simple words what the benefit is of
>> reloading the keys on every new outbound connection... yes, you *could*
>> put in a new key/cert/CA set while OpenVPN is active, and then trigger
>> a SIGUSR1 restart, having it "seamlessly" move to new credentials...
>>
>> But...
>>
>> How many of you do that? Instead of just calling "service openvpn
>> restart"?
>>
>> I do not use --persist-key, but I still restart my services after
>> fiddling
>> with configs...
>
> I am also for removing persist-key option (and ignore it if still
> present) and just always have the same behaviour. I can also not come up
> with a valid scenario where setting/not setting this option is making a
> real desirable difference.
For what it's still worth: I too agree that persist-key should be
always-on and removed as on option.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Remove support for non ISO C99 vararg support
;
> diff --git a/src/openvpn/error.h b/src/openvpn/error.h
> index eaedf172c..1a5521654 100644
> --- a/src/openvpn/error.h
> +++ b/src/openvpn/error.h
> @@ -146,33 +146,12 @@ bool dont_mute(unsigned int flags);
> /* Macro to ensure (and teach static analysis tools) we exit on fatal errors
> */
> #define EXIT_FATAL(flags) do { if ((flags) & M_FATAL) {_exit(1);}} while
> (false)
>
> -#if defined(HAVE_CPP_VARARG_MACRO_ISO) && !defined(__LCLINT__)
> -#define HAVE_VARARG_MACROS
> #define msg(flags, ...) do { if (msg_test(flags)) {x_msg((flags),
> __VA_ARGS__);} EXIT_FATAL(flags); } while (false)
> #ifdef ENABLE_DEBUG
> #define dmsg(flags, ...) do { if (msg_test(flags)) {x_msg((flags),
> __VA_ARGS__);} EXIT_FATAL(flags); } while (false)
> #else
> #define dmsg(flags, ...)
> #endif
> -#elif defined(HAVE_CPP_VARARG_MACRO_GCC) && !defined(__LCLINT__)
> -#define HAVE_VARARG_MACROS
> -#define msg(flags, args ...) do { if (msg_test(flags)) {x_msg((flags),
> args);} EXIT_FATAL(flags); } while (false)
> -#ifdef ENABLE_DEBUG
> -#define dmsg(flags, args ...) do { if (msg_test(flags)) {x_msg((flags),
> args);} EXIT_FATAL(flags); } while (false)
> -#else
> -#define dmsg(flags, args ...)
> -#endif
> -#else /* if defined(HAVE_CPP_VARARG_MACRO_ISO) && !defined(__LCLINT__) */
> -#if !PEDANTIC
> -#ifdef _MSC_VER
> -#pragma message("this compiler appears to lack vararg macros which will
> cause a significant degradation in efficiency")
> -#else
> -#warning this compiler appears to lack vararg macros which will cause a
> significant degradation in efficiency (you can ignore this warning if you are
> using LCLINT)
> -#endif
> -#endif
> -#define msg x_msg
> -#define dmsg x_msg
> -#endif /* if defined(HAVE_CPP_VARARG_MACRO_ISO) && !defined(__LCLINT__) */
>
> void x_msg(const unsigned int flags, const char *format, ...)
> #ifdef __GNUC__
> diff --git a/src/tapctl/error.h b/src/tapctl/error.h
> index 924cbbe89..cea50ba52 100644
> --- a/src/tapctl/error.h
> +++ b/src/tapctl/error.h
> @@ -67,7 +67,6 @@ bool dont_mute(unsigned int flags);
> #endif
> #define EXIT_FATAL(flags) do { if ((flags) & M_FATAL) {_exit(1);}} while
> (false)
>
> -#define HAVE_VARARG_MACROS
> #define msg(flags, ...) do { if (msg_test(flags)) {x_msg((flags),
> __VA_ARGS__);} EXIT_FATAL(flags); } while (false)
> #ifdef ENABLE_DEBUG
> #define dmsg(flags, ...) do { if (msg_test(flags)) {x_msg((flags),
> __VA_ARGS__);} EXIT_FATAL(flags); } while (false)
>
Thanks for doing the research. I wrote a patch like this a while ago,
but ever submitted it because I "just had to double check"...
Patch looks good and passes a configure-make-check cycle.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Remove flexible array member autoconf check
Hi,
On 28-03-2021 14:22, Arne Schwabe wrote:
> This is configure macro that tries out how to declare a variable array
> at the end of struct. This has been standardised in C99, so there is
> no more need for non C99 magic. See also this stackoverflow discussion:
>
> https://stackoverflow.com/questions/14643406/whats-the-need-of-array-with-zero-elements
> Signed-off-by: Arne Schwabe
> ---
> config-msvc.h | 1 -
> m4/ax_emptyarray.m4 | 40
> src/openvpn/circ_list.h | 2 +-
> src/openvpn/syshead.h | 2 --
> 4 files changed, 1 insertion(+), 44 deletions(-)
> delete mode 100644 m4/ax_emptyarray.m4
>
> diff --git a/config-msvc.h b/config-msvc.h
> index e430ca96f..0260927ce 100644
> --- a/config-msvc.h
> +++ b/config-msvc.h
> @@ -119,7 +119,6 @@
> #define inline __inline
> #endif
>
> -#define EMPTY_ARRAY_SIZE 0
> #define TARGET_WIN32 1
> #define TARGET_ALIAS "Windows-MSVC"
>
> diff --git a/m4/ax_emptyarray.m4 b/m4/ax_emptyarray.m4
> deleted file mode 100644
> index c6781c179..0
> --- a/m4/ax_emptyarray.m4
> +++ /dev/null
> @@ -1,40 +0,0 @@
> -dnl @synopsis AX_EMPTY_ARRAY
> -dnl
> -dnl Define EMPTY_ARRAY_SIZE to be either "0"
> -dnl or "" depending on which syntax the compiler
> -dnl prefers for empty arrays in structs.
> -dnl
> -dnl @version
> -dnl @author James Yonan
> -AC_DEFUN([AX_EMPTY_ARRAY], [
> - AS_VAR_PUSHDEF([VAR],[ax_cv_c_empty_array])dnl
> - AC_CACHE_CHECK(
> - [for C compiler empty array size],
> - [VAR],
> - [AC_COMPILE_IFELSE(
> - [AC_LANG_PROGRAM(
> - ,
> - [[
> -struct { int foo; int bar[0]; } mystruct;
> - ]]
> - )],
> - [VAR=0],
> - [AC_COMPILE_IFELSE(
> - [AC_LANG_PROGRAM(
> - ,
> - [[
> -struct { int foo; int bar[]; } mystruct;
> - ]]
> - )],
> - [VAR=],
> - [AC_MSG_ERROR([C compiler is unable to creaty
> empty arrays])]
> - )]
> - )]
> - )dnl
> - AC_DEFINE_UNQUOTED(
> - [EMPTY_ARRAY_SIZE],
> - [$VAR],
> - [Dimension to use for empty array declaration]
> - )dnl
> - AS_VAR_POPDEF([VAR])dnl
> -])
> diff --git a/src/openvpn/circ_list.h b/src/openvpn/circ_list.h
> index 23b42d2ab..ba9115eab 100644
> --- a/src/openvpn/circ_list.h
> +++ b/src/openvpn/circ_list.h
> @@ -34,7 +34,7 @@
> int x_size; \
> int x_cap; \
> int x_sizeof; \
> -type x_list[EMPTY_ARRAY_SIZE]; \
> +type x_list[]; \
> }
>
> #define CIRC_LIST_PUSH(obj, item) \
> diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
> index 5ee9bf1e8..cf9714593 100644
> --- a/src/openvpn/syshead.h
> +++ b/src/openvpn/syshead.h
> @@ -392,8 +392,6 @@ typedef int MIB_TCP_STATE;
> #ifdef PEDANTIC
> #undef HAVE_CPP_VARARG_MACRO_GCC
> #undef HAVE_CPP_VARARG_MACRO_ISO
> -#undef EMPTY_ARRAY_SIZE
> -#define EMPTY_ARRAY_SIZE 1
> #undef inline
> #define inline
> #endif
>
Makes sense, and change looks good. This is just unneeded configure
delay. Ran a quick configure-make-check to double-check there's not a
trivial typo I'm overlooking.
I haven't tested compiling with MSVC.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Avoid generating unecessary mbed debug messages
Hi,
On 16-03-2021 13:44, Arne Schwabe wrote:
> The main motivation to make this change is to avoid a crash in mbed TLS
> 2.25 with --verb < 8.
>
> mbed TLS 2.25 has a nasty bug that the print function for Montgomery style
> EC curves (Curve25519 and Curve448) does segfault. See also the issue
> reported here: https://github.com/ARMmbed/mbedtls/issues/4208
>
> We request always debug level 3 from mbed TLS but filter out any debug
> output of level 3 unless verb 8 or higher is set. This commeit sets
> the debug level to 2 to avoid this problem by makeing mbed TLS not
> generatin the problematic debug output.
>
> For the affected version to still use --verb 8 with mbed TLS 2.25 is to
> restrict the EC groups to ones that do not crash the print function
> like with '--tls-groups secp521r1:secp384r1:secp256r1'.
>
> This patch has no patch on user-visible behaviour on unaffected mbed TLS
> versions.
>
> Signed-off-by: Arne Schwabe
>
> Patch V2: Replace magic constant with proper define. Highlight more this
> avoding generating unessary debug output than crash workaround.
> ---
> src/openvpn/options.c | 6 ++
> src/openvpn/ssl_common.h | 1 +
> src/openvpn/ssl_mbedtls.c | 13 -
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 0eb049d8..c4646d48 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5883,6 +5883,12 @@ add_option(struct options *options,
> {
> VERIFY_PERMISSION(OPT_P_MESSAGES);
> options->verbosity = positive_atoi(p[1]);
> +if (options->verbosity >= (D_TLS_DEBUG_MED & M_DEBUG_LEVEL))
> +{
> +/* We pass this flag to the SSL library to avoid a
> + * mbed TLS always generating debug level logging */
Typo: "a mbed TLS" -> "mbed TLS".
> +options->ssl_flags |= SSLF_TLS_DEBUG_ENABLED;
> +}
> #if !defined(ENABLE_DEBUG) && !defined(ENABLE_SMALL)
> /* Warn when a debug verbosity is supplied when built without debug
> support */
> if (options->verbosity >= 7)
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index b493cbb7..2e3c98db 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -352,6 +352,7 @@ struct tls_options
> #define SSLF_TLS_VERSION_MIN_MASK 0xF /* (uses bit positions 6 to 9) */
> #define SSLF_TLS_VERSION_MAX_SHIFT10
> #define SSLF_TLS_VERSION_MAX_MASK 0xF /* (uses bit positions 10 to 13)
> */
> +#define SSLF_TLS_DEBUG_ENABLED(1<<14)
> unsigned int ssl_flags;
>
> #ifdef ENABLE_MANAGEMENT
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index b30b6b9d..4626e983 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -1058,7 +1058,18 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
> mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint,
> MBEDTLS_SSL_TRANSPORT_STREAM,
> MBEDTLS_SSL_PRESET_DEFAULT);
> #ifdef MBEDTLS_DEBUG_C
> -mbedtls_debug_set_threshold(3);
> +/* We only want to have mbed TLS generate debug level logging when we
> would
> + * also display it.
> + * In fact mbed TLS 2.25.0 crashes generating debug log if Curve25591 is
> + * selected for DH (https://github.com/ARMmbed/mbedtls/issues/4208) */
> +if (session->opt->ssl_flags & SSLF_TLS_DEBUG_ENABLED)
> +{
> + mbedtls_debug_set_threshold(3);
> +}
> +else
> +{
> +mbedtls_debug_set_threshold(2);
> +}
> #endif
> mbedtls_ssl_conf_dbg(ks_ssl->ssl_config, my_debug, NULL);
> mbedtls_ssl_conf_rng(ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
>
Otherwise, this now looks good me.
Acked-by: Steffan Karger
Thanks,
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Avoid a crash in mbed TLS 2.25 with --verb < 8
Hi,
On 08-03-2021 15:21, Arne Schwabe wrote:
> mbed TLS 2.25 has a nasty bug that the print function for Montgomery style
> EC curves (Curve25519 and Curve448) does segfault. See also the issue
> reported here: https://github.com/ARMmbed/mbedtls/issues/4208
>
> We request always debug level 3 from mbed TLS but filter out any debug
> output of level 3 unless verb 8 or higher is set. This commeit sets
> the debug level to 2 to avoid this problem by makeing mbed TLS not
> generatin the problematic debug output.
Only setting level 3 when we actually need it is a better approach
anyway. So +1 that, even without this bug. Somewhat surprisingly, a
quick test shows that this does not matter for performance (perhaps that
is why I didn't do it like that back then, can't remember).
> For the affected version to still use --verb 8 with mbed TLS 2.25 is to
> restrict the EC groups to ones that do not crash the print function
> like with '--tls-groups secp521r1:secp384r1:secp256r1'.
>
> This patch has no patch on user-visible behaviour on unaffected mbed TLS
> versions.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/options.c | 6 ++
> src/openvpn/ssl_common.h | 1 +
> src/openvpn/ssl_mbedtls.c | 11 ++-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 0eb049d8..6d908e15 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5883,6 +5883,12 @@ add_option(struct options *options,
> {
> VERIFY_PERMISSION(OPT_P_MESSAGES);
> options->verbosity = positive_atoi(p[1]);
> +if (options->verbosity > 7)
Instead of the magic 7, can we use "D_TLS_DEBUG_MED & M_DEBUG_LEVEL"?
> +{
> +/* We pass this flag to the SSL library to avoid a bug
> + * in mbed TLS 2.5.0 with high log level */
(Repeating Antonio:) Should be 2.25.
> +options->ssl_flags |= SSLF_TLS_DEBUG_ENABLED;
> +}
> #if !defined(ENABLE_DEBUG) && !defined(ENABLE_SMALL)
> /* Warn when a debug verbosity is supplied when built without debug
> support */
> if (options->verbosity >= 7)
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index bbb8135d..f821c654 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -350,6 +350,7 @@ struct tls_options
> #define SSLF_TLS_VERSION_MIN_MASK 0xF /* (uses bit positions 6 to 9) */
> #define SSLF_TLS_VERSION_MAX_SHIFT10
> #define SSLF_TLS_VERSION_MAX_MASK 0xF /* (uses bit positions 10 to 13)
> */
> +#define SSLF_TLS_DEBUG_ENABLED(1<<14)
> unsigned int ssl_flags;
>
> #ifdef ENABLE_MANAGEMENT
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index b30b6b9d..a7b6c2c6 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -1058,7 +1058,16 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
> mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint,
> MBEDTLS_SSL_TRANSPORT_STREAM,
> MBEDTLS_SSL_PRESET_DEFAULT);
> #ifdef MBEDTLS_DEBUG_C
> -mbedtls_debug_set_threshold(3);
> +/* This works around a crash in mbed TLS 2.25 if Curve25591 is selected
> + * for DH (https://github.com/ARMmbed/mbedtls/issues/4208)*/
> +if (session->opt->ssl_flags & SSLF_TLS_DEBUG_ENABLED)
> +{
> +mbedtls_debug_set_threshold(3);
> +}
> +else
> +{
> +mbedtls_debug_set_threshold(2);
> +}
> #endif
> mbedtls_ssl_conf_dbg(ks_ssl->ssl_config, my_debug, NULL);
> mbedtls_ssl_conf_rng(ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
>
Otherwise, this looks good to me. Just stare-at-code, no real testing done.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] wanted: mechanism to send text messages to client
Hi, On 21-12-2020 21:25, Arne Schwabe wrote: > Am 21.12.20 um 20:11 schrieb Gert Doering: >> On Mon, Dec 21, 2020 at 06:24:36PM +, Greg Cox wrote: >>> If the software were >>> to contain a mechanism to make certain failure cases automatically more >>> prominent, particularly for 'simple' users who have GUI clients, it'll be a >>> big win for supportability on larger installs. >> >> This is indeed getting into philosophy... we do send different types of >> AUTH_FAILED today (like, for token expired). Maybe we could send an >> "AUTH_FAILED,cert expired" and have the client display this? >> >> (I admit that I'm neither an expert on AUTH_FAILED message, nor on >> "what is the client doing on variations of it", nor on "what *should* >> be the expected outcome?". Selva, Arne will know more). > > It is easy to add that message, [...] Uhm, I would say it's impossible to send that message. AUTH_FAILED messages are sent over the control channel, while in case of certificate errors the control channel will never be initialized. We could however do something that has the same effect: don't prevent TLS from sending it's "certificate_expired" alert. OpenVPN 2 (don't know about 3) currently just doesn't respond at all if it detects a TLS error. IIRC, this extra-paranoid behaviour has saved us from at least one of the timing-based attacks on TLS from the past, but I can't recall which one. At the same time, the TLS protocol and it's implementation have matured a lot since heartbleed. Possibly beyond the point where usability concerns now outweigh the security concerns. Before anyone suggests making this optional: no. no. no. I strongly believer we should carefully consider if we want to allow TLS to send alerts, or leave this as-is. David actually already brought this up in 2016, see this thread: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12892.html Note that any of this is separate from the initial discussion, where Gert proposes to send notifications *before* the certificate expires. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix tls-auth mismatch OCC message when tls-cryptv2 is used.
Hi,
On 11-12-2020 13:59, Arne Schwabe wrote:
> A server with tls-cryptv2 and tls-auth produces the warning:
>
> WARNING: 'tls-auth' is present in local config but missing in remote
> config, local='tls-auth'"
>
> The tls-auth option has no argument so the strpefix with the space
> included does not match it.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/options.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index d824cbad..d9d492b2 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -4138,7 +4138,7 @@ options_warning_safe_scan2(const int msglevel,
> if (strprefix(p1, "key-method ")
> || strprefix(p1, "keydir ")
> || strprefix(p1, "proto ")
> -|| strprefix(p1, "tls-auth ")
> +|| streq(p1, "tls-auth")
> || strprefix(p1, "tun-ipv6")
> || strprefix(p1, "cipher "))
> {
>
Thanks for fixing this.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/2 v2] tls-crypt-v2: fix server memory leak
tls-crypt-v2 was developed in parallel with the changes that allowed to
use tls-auth/tls-crypt in connection blocks. The tls-crypt-v2 patch set
was never updated to the new reality after commit 5817b49b, causing a
memory leak of about 600 bytes for each connecting client.
It would be nicer to not reload the tls-crypt-v2 server key for each
connecting client, but that requires more refactoring (and thus more time
to get right). So for now just plug the leak by free'ing the memory when
we close a client connection.
To test this easily, compile openvpn with -fsanity=address, run a server
with tls-crypt-v2, connect a client, stop the server.
Signed-off-by: Steffan Karger
---
v2: remove now-redundant free_key_ctx() from key_schedule_free()
src/openvpn/init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 27a4170d..c3493c42 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2561,7 +2561,6 @@ key_schedule_free(struct key_schedule *ks, bool
free_ssl_ctx)
if (tls_ctx_initialised(&ks->ssl_ctx) && free_ssl_ctx)
{
tls_ctx_free(&ks->ssl_ctx);
-free_key_ctx(&ks->tls_crypt_v2_server_key);
}
CLEAR(*ks);
}
@@ -3619,6 +3618,7 @@ do_close_free_key_schedule(struct context *c, bool
free_ssl_ctx)
* always free the tls_auth/crypt key. If persist_key is true, the key will
* be reloaded from memory (pre-cached)
*/
+free_key_ctx(&c->c1.ks.tls_crypt_v2_server_key);
free_key_ctx_bi(&c->c1.ks.tls_wrap_key);
CLEAR(c->c1.ks.tls_wrap_key);
buf_clear(&c->c1.ks.tls_crypt_v2_wkc);
--
2.25.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/2] tls-crypt-v2: also preload tls-crypt-v2 keys (if --persist-key)
This allows tls-crypt-v2 servers to drop privileges after reading the
keys. Without it, the server would try to read the key file for each
connecting client. (And clients for each reconnect.)
As with the previous patch, the pre-loading was developed in parallel
with tls-crypt-v2, and the tls-crypt-v2 patches were never amended to
implement the pre-loading.
Also as with the previous patch, it would be nicer if servers would not
reload the tls-crypt-v2 server key for each connecting client. But let's
first fix the issue, and see if we can improve later.
Signed-off-by: Steffan Karger
---
src/openvpn/options.c | 52 +--
1 file changed, 25 insertions(+), 27 deletions(-)
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 21f8d494..599f534c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1980,6 +1980,23 @@ connection_entry_load_re(struct connection_entry *ce,
const struct remote_entry
}
}
+static void
+connection_entry_preload_key(const char **key_file, bool *key_inline,
+ struct gc_arena *gc)
+{
+if (key_file && *key_file && !(*key_inline))
+{
+struct buffer in = buffer_read_from_file(*key_file, gc);
+if (!buf_valid(&in))
+{
+msg(M_FATAL, "Cannot pre-load keyfile (%s)", *key_file);
+}
+
+*key_file = (const char *) in.data;
+*key_inline = true;
+}
+}
+
static void
options_postprocess_verify_ce(const struct options *options,
const struct connection_entry *ce)
@@ -2931,36 +2948,17 @@ options_postprocess_mutate_ce(struct options *o, struct
connection_entry *ce)
ce->tls_crypt_v2_file_inline = o->tls_crypt_v2_file_inline;
}
-/* pre-cache tls-auth/crypt key file if persist-key was specified and keys
- * were not already embedded in the config file
+/* Pre-cache tls-auth/crypt(-v2) key file if persist-key was specified and
+ * keys were not already embedded in the config file.
*/
if (o->persist_key)
{
-if (ce->tls_auth_file && !ce->tls_auth_file_inline)
-{
-struct buffer in = buffer_read_from_file(ce->tls_auth_file,
&o->gc);
-if (!buf_valid(&in))
-{
-msg(M_FATAL, "Cannot pre-load tls-auth keyfile (%s)",
-ce->tls_auth_file);
-}
-
-ce->tls_auth_file = (char *)in.data;
-ce->tls_auth_file_inline = true;
-}
-
-if (ce->tls_crypt_file && !ce->tls_crypt_file_inline)
-{
-struct buffer in = buffer_read_from_file(ce->tls_crypt_file,
&o->gc);
-if (!buf_valid(&in))
-{
-msg(M_FATAL, "Cannot pre-load tls-crypt keyfile (%s)",
-ce->tls_crypt_file);
-}
-
-ce->tls_crypt_file = (char *)in.data;
-ce->tls_crypt_file_inline = true;
-}
+connection_entry_preload_key(&ce->tls_auth_file,
+ &ce->tls_auth_file_inline, &o->gc);
+connection_entry_preload_key(&ce->tls_crypt_file,
+ &ce->tls_crypt_file_inline, &o->gc);
+connection_entry_preload_key(&ce->tls_crypt_v2_file,
+ &ce->tls_crypt_v2_file_inline, &o->gc);
}
}
--
2.25.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/2] tls-crypt-v2: fix server memory leak
tls-crypt-v2 was developed in parallel with the changes that allowed to use tls-auth/tls-crypt in connection blocks. The tls-crypt-v2 patch set was never updated to the new reality after commit 5817b49b, causing a memory leak of about 600 bytes for each connecting client. It would be nicer to not reload the tls-crypt-v2 server key for each connecting client, but that requires more refactoring (and thus more time to get right). So for now just plug the leak by free'ing the memory when we close a client connection. To test this easily, compile openvpn with -fsanity=address, run a server with tls-crypt-v2, connect a client, stop the server. Signed-off-by: Steffan Karger --- src/openvpn/init.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 27a4170d..5cde8a4b 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -3619,6 +3619,7 @@ do_close_free_key_schedule(struct context *c, bool free_ssl_ctx) * always free the tls_auth/crypt key. If persist_key is true, the key will * be reloaded from memory (pre-cached) */ +free_key_ctx(&c->c1.ks.tls_crypt_v2_server_key); free_key_ctx_bi(&c->c1.ks.tls_wrap_key); CLEAR(c->c1.ks.tls_wrap_key); buf_clear(&c->c1.ks.tls_crypt_v2_wkc); -- 2.25.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Fix port-share option with TLS-Crypt v2
Hi,
On 30-11-2020 13:38, Arne Schwabe wrote:
> The port-share option assumed that all openvpn initial reset packets
> are between 14 and 255 bytes long. This is not true for tls-crypt-v2.
>
> Patch V2: use correct length for TLS-Crypt v2, use length variable
> non-tlscryptv2 test
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/ps.c | 34 +-
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
> index 2089e6b9..5d760782 100644
> --- a/src/openvpn/ps.c
> +++ b/src/openvpn/ps.c
> @@ -983,14 +983,38 @@ is_openvpn_protocol(const struct buffer *buf)
> const int len = BLEN(buf);
> if (len >= 3)
> {
> -return p[0] == 0
> - && p[1] >= 14
> - && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)
> - || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 <<
> P_OPCODE_SHIFT));
> +int plen = (p[0] << 8) | p[1];
> +
> +if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT))
> +{
> +/* WKc is at least 290 byte (not including metadata):
> + *
> + * 16 bit len + 256 bit HMAC + 2048 bit Kc = 2320 bit
> + *
> + * This is increased by the normal length of client handshake +
> + * tls-crypt overhead (32)
> + *
> + * For metadata tls-crypt-v2.txt does not explicitly specify
> + * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN
> + * as 1024 bytes. We err on the safe side with 255 extra overhead
> + *
> + * We don't do the 2 byte check for tls-crypt-v2 because it is
> very
> + * unrealistic to have only 2 bytes available.
> + */
> +return (plen >= 336 && plen < (1024 + 255));
> +}
> +else
> +{
> +/* For non tls-crypt2 we assume the packet length to valid
> between
> + * 14 and 255 */
> +return plen >= 14 && plen <= 255
> + && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 <<
> P_OPCODE_SHIFT));
> +}
> }
> else if (len >= 2)
> {
> -return p[0] == 0 && p[1] >= 14;
> +int plen = (p[0] << 8) | p[1];
> +return plen >= 14 && plen <= 255;
> }
> else
> {
>
Thanks. This looks good to me now.
Ran some tests to check that this correctly resolves the issue when
using --tls-crypt-v2 in combination with --port-share.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix port-share option with TLS-Crypt v2
Hi,
On 20-11-2020 17:04, Arne Schwabe wrote:
> The port-share option assumed that all openvpn initial reset packets
> are between 14 and 255 bytes long. This is not true for tls-crypt-v2.
Good catch, I totally missed this in the tls-crypt-v2 patch set.
> ---
> src/openvpn/ps.c | 34 ++
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
> index 2089e6b9..cd00bc23 100644
> --- a/src/openvpn/ps.c
> +++ b/src/openvpn/ps.c
> @@ -983,10 +983,36 @@ is_openvpn_protocol(const struct buffer *buf)
> const int len = BLEN(buf);
> if (len >= 3)
> {
> -return p[0] == 0
> - && p[1] >= 14
> - && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)
> - || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 <<
> P_OPCODE_SHIFT));
> +if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT))
> +{
> +/* WKc is at least 306 byte (not including metadata):
> + *
> + * 16 bit len + 256 bit HMAC + 128 bit IV + 2048 bit Kc = 2448
> bit
The IV is part of the HMAC, so does not need to be counted here. So this
this be 2320 bits / 290 bytes.
> + *
> + * This is increased by the normal length of client handshake +
> + * tls-crypt overhead (32)
> + *
> + * For metadata tls-crypt-v2.txt does not explicitly specify
> + * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN
> + * as 1024 bytes. We err on the safe side with 255 extra overhead
> + *
> + * We don't do the 2 byte check for tls-crypt-v2 because it is
> very
> + * unrealistic to have only 2 bytes available.
> + */
> +int plen = (p[0] << 8) | p[1];
> +
> +return (plen >= 352 && plen < (1024 + 255));
> +}
> +else
> +{
> +/* first two bytes are the size field. This is an openvpn packet
> + * between 14 bytes and 255 (otherwise p[1] would be 01 or
> larger.
> + * on the hard reset packet key_id is zero, so only the opcode
> + * part is non-null */
Instead of explaining the length field in the comment, why not just move
the "int plen = (p[0] << 8) | p[1];" outside the if to have the code
explain that by itself?
> +return p[0] == 0
> +&& p[1] >= 14
> +&& (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 <<
> P_OPCODE_SHIFT));
> +}
> }
> else if (len >= 2)
> {
>
Thanks,
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Fix compilation on pre-EKM mbedTLS libraries.
Hi,
On 10-10-2020 10:14, Gert Doering wrote:
> commit f0734e49956217 simplified key_state_export_keying_material(),
> changing the function prototype. For older mbedTLS versions, there
> is an "always fail" dummy function which was overlooked in that change.
>
> Fix prototype.
>
> v2: also adjust function return (NULL -> false)
>
> Signed-off-by: Gert Doering
> ---
> src/openvpn/ssl_mbedtls.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index bb5633b7..11fbeae4 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -252,14 +252,13 @@ key_state_export_keying_material(struct tls_session
> *session,
> }
> }
> #else
> -unsigned char*
> +bool
> key_state_export_keying_material(struct tls_session *session,
> const char* label, size_t label_size,
> - size_t ekm_size,
> - struct gc_arena *gc)
> + void *ekm, size_t ekm_size)
> {
> /* Dummy function to avoid ifdefs in the common code */
> -return NULL;
> +return false;
> }
> #endif /* HAVE_EXPORT_KEYING_MATERIAL */
>
>
Sorry, totally missed this. Fix looks good.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Simplify key material exporter backend API
Just pass pointer and length, instead of a gc and return (possibly)
allocated memory. Saves us some gc instantiations and memcpy()s. Exact
same functionality, 19 lines less code.
(Didn't want to delay the TLS EKM reviews for this, so submitted as a
patch afterwards.)
Signed-off-by: Steffan Karger
---
src/openvpn/ssl.c | 26 --
src/openvpn/ssl_backend.h | 14 +-
src/openvpn/ssl_mbedtls.c | 12 +---
src/openvpn/ssl_openssl.c | 11 ---
4 files changed, 22 insertions(+), 41 deletions(-)
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b572b7b8..87b51d96 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1786,23 +1786,14 @@ init_key_contexts(struct key_ctx_bi *key,
static bool
generate_key_expansion_tls_export(struct tls_session *session, struct key2
*key2)
{
-struct gc_arena gc = gc_new();
-unsigned char *key2data;
-
-key2data = key_state_export_keying_material(session,
-EXPORT_KEY_DATA_LABEL,
-strlen(EXPORT_KEY_DATA_LABEL),
-sizeof(key2->keys),
-&gc);
-if (!key2data)
+if (!key_state_export_keying_material(session, EXPORT_KEY_DATA_LABEL,
+ strlen(EXPORT_KEY_DATA_LABEL),
+ key2->keys, sizeof(key2->keys)))
{
return false;
}
-memcpy(key2->keys, key2data, sizeof(key2->keys));
-secure_memzero(key2data, sizeof(key2->keys));
key2->n = 2;
-gc_free(&gc);
return true;
}
@@ -2499,12 +2490,11 @@ export_user_keying_material(struct key_state_ssl *ssl,
unsigned int size = session->opt->ekm_size;
struct gc_arena gc = gc_new();
-unsigned char *ekm;
-if ((ekm = key_state_export_keying_material(session,
-session->opt->ekm_label,
-
session->opt->ekm_label_size,
-session->opt->ekm_size,
-&gc)))
+unsigned char *ekm = gc_malloc(session->opt->ekm_size, true, &gc);
+if (key_state_export_keying_material(session,
+ session->opt->ekm_label,
+ session->opt->ekm_label_size,
+ ekm, session->opt->ekm_size))
{
unsigned int len = (size * 2) + 2;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index 4bcb3181..c3d12e5b 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -398,18 +398,14 @@ void backend_tls_ctx_reload_crl(struct tls_root_ctx
*ssl_ctx,
* @param session The session associated with the given key_state
* @param labelThe label to use when exporting the key
* @param label_size The size of the label to use when exporting the key
- * @param ekm_size THe size of the exported/returned key material
- * @param gc gc_arena that might be used to allocate the string
- * returned
- * @returnsThe exported key material, the caller may zero the
- * string but should not free it
+ * @param ekm Buffer to return the exported key material in
+ * @param ekm_size The size of ekm, in bytes
+ * @returnstrue if exporting succeeded, false otherwise
*/
-
-unsigned char*
+bool
key_state_export_keying_material(struct tls_session *session,
const char* label, size_t label_size,
- size_t ekm_size,
- struct gc_arena *gc) __attribute__((nonnull));
+ void *ekm, size_t ekm_size);
/**/
/** @addtogroup control_tls
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index f375e957..bb5633b7 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -219,11 +219,10 @@ mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned
char *ms,
return true;
}
-unsigned char *
+bool
key_state_export_keying_material(struct tls_session *session,
const char* label, size_t label_size,
- size_t ekm_size,
- struct gc_arena *gc)
+ void *ekm, size_t ekm_size)
{
ASSERT(strlen(label) == label_size);
@@ -233,10 +232,9 @@ key_state_export_keying_material(struct tls_session
*session,
* there is no PRF, in both cases we cannot generate key ma
[Openvpn-devel] [PATCH] networking_iproute2: fix memory leak in net_iface_mtu_set()
ASAN yelled at me that someone forgot to call argv_free(). Fix that. Signed-off-by: Steffan Karger --- src/openvpn/networking_iproute2.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c index f3b9c614..3b460527 100644 --- a/src/openvpn/networking_iproute2.c +++ b/src/openvpn/networking_iproute2.c @@ -88,6 +88,8 @@ net_iface_mtu_set(openvpn_net_ctx_t *ctx, const char *iface, uint32_t mtu) argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set failed"); +argv_free(&argv); + return 0; } -- 2.25.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v5] Implement generating data channel keys via EKM/RFC 5705
strlen(EXPORT_KEY_DATA_LABEL),
> +sizeof(key2->keys),
> +&gc);
> +if (!key2data)
> +{
> +return false;
> +}
> +memcpy(key2->keys, key2data, sizeof(key2->keys));
> +secure_memzero(key2data, sizeof(key2->keys));
> +key2->n = 2;
> +
> +gc_free(&gc);
> +return true;
> +}
> +
> static struct key2
> generate_key_expansion_openvpn_prf(const struct tls_session *session)
> {
> @@ -1854,7 +1877,7 @@ generate_key_expansion_openvpn_prf(const struct
> tls_session *session)
> */
> static bool
> generate_key_expansion(struct key_ctx_bi *key,
> - const struct tls_session *session)
> + struct tls_session *session)
> {
> bool ret = false;
> struct key2 key2;
> @@ -1865,10 +1888,20 @@ generate_key_expansion(struct key_ctx_bi *key,
> goto exit;
> }
>
> -
> bool server = session->opt->server;
>
> -key2 = generate_key_expansion_openvpn_prf(session);
> +if (session->opt->crypto_flags & CO_USE_TLS_KEY_MATERIAL_EXPORT)
> +{
> +if (!generate_key_expansion_tls_export(session, &key2))
> +{
> +msg(D_TLS_ERRORS, "TLS Error: Keying material export failed");
> +goto exit;
> +}
> +}
> +else
> +{
> +key2 = generate_key_expansion_openvpn_prf(session);
> +}
>
> key2_print(&key2, &session->opt->key_type,
> "Master Encrypt", "Master Decrypt");
> @@ -1967,6 +2000,11 @@ tls_session_update_crypto_params(struct tls_session
> *session,
> return false;
> }
>
> +if (options->data_channel_use_ekm)
> +{
> +session->opt->crypto_flags |= CO_USE_TLS_KEY_MATERIAL_EXPORT;
> +}
> +
> if (strcmp(options->ciphername, session->opt->config_ciphername))
> {
> msg(D_HANDSHAKE, "Data Channel: using negotiated cipher '%s'",
> @@ -2251,13 +2289,11 @@ push_peer_info(struct buffer *buf, struct tls_session
> *session)
> * push request, also signal that the client wants
> * to get push-reply messages without without requiring a round
> * trip for a push request message*/
> -if(session->opt->pull)
> +if (session->opt->pull)
> {
> iv_proto |= IV_PROTO_REQUEST_PUSH;
> }
>
> -buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
> -
> /* support for Negotiable Crypto Parameters */
> if (session->opt->ncp_enabled
> && (session->opt->mode == MODE_SERVER || session->opt->pull))
> @@ -2269,8 +2305,14 @@ push_peer_info(struct buffer *buf, struct tls_session
> *session)
> buf_printf(&out, "IV_NCP=2\n");
> }
> buf_printf(&out, "IV_CIPHERS=%s\n",
> session->opt->config_ncp_ciphers);
> +
> +#ifdef HAVE_EXPORT_KEYING_MATERIAL
> +iv_proto |= IV_PROTO_TLS_KEY_EXPORT;
> +#endif
> }
>
> +buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
> +
> /* push compression status */
> #ifdef USE_COMP
> comp_generate_peer_info_string(&session->opt->comp_options, &out);
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 005628f6..f00f8abd 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -116,6 +116,8 @@
> * to wait for a push-request to send a push-reply */
> #define IV_PROTO_REQUEST_PUSH (1<<2)
>
> +/** Supports key derivation via TLS key material exporter [RFC5705] */
> +#define IV_PROTO_TLS_KEY_EXPORT (1<<3)
>
> /* Default field in X509 to be username */
> #define X509_USERNAME_FIELD_DEFAULT "CN"
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index cf9fba25..4bcb3181 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -389,6 +389,7 @@ void key_state_ssl_free(struct key_state_ssl *ks_ssl);
> void backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx,
> const char *crl_file, bool crl_inline);
>
> +#define EXPORT_KEY_DATA_LABEL "EXPORTER-OpenVPN-datakeys"
> /**
> * Keying Material Exporters [RFC 5705] allows additional keying material to
> be
> * derived from existing TLS channel. This exported keying material can then
> be
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 4ec355a9..f375e957 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -1168,11 +1168,8 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>
> #ifdef HAVE_EXPORT_KEYING_MATERIAL
> /* Initialize keying material exporter */
> -if (session->opt->ekm_size)
> -{
> -mbedtls_ssl_conf_export_keys_ext_cb(ks_ssl->ssl_config,
> -mbedtls_ssl_export_keys_cb,
> session);
> -}
> +mbedtls_ssl_conf_export_keys_ext_cb(ks_ssl->ssl_config,
> +mbedtls_ssl_export_keys_cb, session);
> #endif
>
> /* Initialise SSL context */
>
If the above is fixed:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 2/2] Implement generating data channel keys via EKM/RFC 5705
Hi, On 25-08-2020 09:36, Arne Schwabe wrote: > OpenVPN currently uses its own (based on TLS 1.0) key derivation > mechanism to generate the 256 bytes key data in key2 struct that > are then used used to generate encryption/hmac/iv vectors. While > this mechanism is still secure, it is not state of the art. > > Instead of modernising our own approach, this commit implements > key derivation using the Keying Material Exporters API introduced > by RFC 5705. Thanks! I'm glad to see you're picking up the work I haven't been able to find time for. > We also use an opportunistic approach of negotiating the use of > EKM (exported key material) through an IV_PROTO flag and prefer > EKM to our own PRF if both client and server support it. The > use of EKM is pushed to the client as part of NCP as > key-derivation tls-ekm. > > We still exchange the random data (112 bytes from client to server > and 64 byte from server to client) for the OpenVPN PRF but > do not use it. Removing that exchange would break the handshake > and make a key-method 3 or similar necessary. > > As a side effect, this makes a little bit easier to have a FIPS compatible > version of OpenVPN since we do not rely on calling MD5 anymore. > > Side note: this commit breaks the (not yet merged) WolfSSL support as it > claims to support EKM in the OpenSSL compat API but always returns an error > if you try to use it. > > Signed-off-by: Arne Schwabe Signed off two times. > Patch v2: rebase/change to V2 of EKM refactoring > > Patch v3: add Changes.rst > > Signed-off-by: Arne Schwabe > --- > Changes.rst | 11 +++ > doc/doxygen/doc_key_generation.h | 14 +++-- > src/openvpn/crypto.h | 4 +++ > src/openvpn/init.c | 1 + > src/openvpn/multi.c | 4 +++ > src/openvpn/options.c| 14 + > src/openvpn/options.h| 3 ++ > src/openvpn/push.c | 5 ++- > src/openvpn/ssl.c| 54 > src/openvpn/ssl.h| 2 ++ > src/openvpn/ssl_backend.h| 2 ++ > src/openvpn/ssl_mbedtls.c| 7 ++--- > 12 files changed, 107 insertions(+), 14 deletions(-) > > diff --git a/Changes.rst b/Changes.rst > index f67e1d76..2a2829e7 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -1,3 +1,14 @@ > +Overview of changes in 2.6 > +== > + > + > +New features > + > +Keying Material Exporters (RFC 5705) based key generation > +As part of the cipher negotiation OpenVPN will automatically prefer > +the RFC5705 based key material generation to the current custom > +OpenVPN PRF. This feature requires OpenSSL or mbed TLS 2.18+. > + > Overview of changes in 2.5 > == > > diff --git a/doc/doxygen/doc_key_generation.h > b/doc/doxygen/doc_key_generation.h > index 4bb9c708..cef773a9 100644 > --- a/doc/doxygen/doc_key_generation.h > +++ b/doc/doxygen/doc_key_generation.h > @@ -58,6 +58,12 @@ > * > * @subsection key_generation_method_2 Key method 2 > * > + * There are two methods for generating key data when using key method 2 > + * the first is OpenVPN's traditional approach that exchanges random > + * data and uses a PRF and the other is using the RFC5705 keying material > + * exporter to generate the key material. For both methods the random > + * data is exchange but only used in the traditional method. > + * > * -# The client generates random material in the following amounts: > *- Pre-master secret: 48 bytes > *- Client's PRF seed for master secret: 32 bytes > @@ -73,8 +79,12 @@ > *server's random material. > * > * %Key method 2 %key expansion is performed by the \c > - * generate_key_expansion() function. Please refer to its source code for > - * details of the %key expansion process. > + * generate_key_expansion_openvpn_prf() function. Please refer to its source > + * code for details of the %key expansion process. > + * > + * When the client sends the IV_PROTO_TLS_KEY_EXPORT flag and the server > replies > + * with `key-derivation tls-ekm` the RFC5705 key material exporter with the > + * label EXPORTER-OpenVPN-datakeys is used for the key data. Did you request this label with IANA? Also, a user could now use --keying-material-exporter to export the data channel keys to a plugin. I'm inclined to say we should prevent that, by rejecting our internal label as a user label. > * @subsection key_generation_random Source of random material > * > diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h > index 999f643e..ec935ca5 100644 > --- a/src/openvpn/crypto.h > +++ b/src/openvpn/crypto.h > @@ -254,6 +254,10 @@ struct crypto_options > #define CO_MUTE_REPLAY_WARNINGS (1<<2) > /**< Bit-flag indicating not to display > * replay warnings. */ > +#define CO_USE_TLS_KEY_MATERIAL_EXPORT (1<<3) > +/**< Bit-flag indicating that key derivation Sugestion: "data channel key deriv
Re: [Openvpn-devel] Introducing the OpenVPN Data Channel Offload Linux kernel module (ovpn-dco)
Hi Antonio, On 22-09-2020 16:17, Antonio Quartulli wrote: > Dear all, > > OpenVPN Inc.[1] and me are happy to announce that today we have released > to the public our work-in-progress project called "ovpn-dco". > > [...] > > The idea of implementing an OpenVPN kernel module has been around for > years; finally what were once words have become lines of code. > I hope more enthusiasts will join the project and help pushing the > development forward. Congrats! Very cool that the first version of the code is out. I'm very much looking forward to this progressing towards stable and feature-complete code. I can't make any promises on involvement right now, but you most certainly triggered my interest :) -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 1/2] Move openvpn specific key expansion into its own function
rror: key already initialized");
> +goto exit;
> +}
> +
> +
> +bool server = session->opt->server;
> +
> +key2 = generate_key_expansion_openvpn_prf(session);
> +
> +key2_print(&key2, &session->opt->key_type,
> + "Master Encrypt", "Master Decrypt");
>
> /* check for weak keys */
> for (int i = 0; i < 2; ++i)
> {
> -fixup_key(&key2.keys[i], key_type);
> -if (!check_key(&key2.keys[i], key_type))
> +if (!check_key(&key2.keys[i], &session->opt->key_type))
> {
> msg(D_TLS_ERRORS, "TLS Error: Bad dynamic key generated");
> goto exit;
> }
> }
> -
> -/* Initialize OpenSSL key contexts */
> -int key_direction = server ? KEY_DIRECTION_INVERSE :
> KEY_DIRECTION_NORMAL;
> -init_key_ctx_bi(key, &key2, key_direction, key_type, "Data Channel");
> -
> -/* Initialize implicit IVs */
> -key_ctx_update_implicit_iv(&key->encrypt, key2.keys[(int)server].hmac,
> - MAX_HMAC_KEY_LENGTH);
> -key_ctx_update_implicit_iv(&key->decrypt, key2.keys[1-(int)server].hmac,
> - MAX_HMAC_KEY_LENGTH);
> -
> +init_key_contexts(key, &session->opt->key_type, server, &key2);
> ret = true;
>
> exit:
> -secure_memzero(&master, sizeof(master));
> secure_memzero(&key2, sizeof(key2));
>
> return ret;
> }
>
> +
Stray newline.
> static void
> key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
> {
> @@ -1879,10 +1922,7 @@ tls_session_generate_data_channel_keys(struct
> tls_session *session)
> {
> bool ret = false;
> struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
> -const struct session_id *client_sid = session->opt->server ?
> - &ks->session_id_remote :
> &session->session_id;
> -const struct session_id *server_sid = !session->opt->server ?
> - &ks->session_id_remote :
> &session->session_id;
> +
>
> if (ks->authenticated == KS_AUTH_FALSE)
> {
> @@ -1891,9 +1931,8 @@ tls_session_generate_data_channel_keys(struct
> tls_session *session)
> }
>
> ks->crypto_options.flags = session->opt->crypto_flags;
> -if (!generate_key_expansion(&ks->crypto_options.key_ctx_bi,
> -&session->opt->key_type, ks->key_src,
> client_sid, server_sid,
> -session->opt->server))
> +
> +if (!generate_key_expansion(&ks->crypto_options.key_ctx_bi, session))
Since you're passing session now, shouldn't you remove the key_ctx_bi
argument too? That's also part of session: (struct key_state) *ks =
&session->key[KS_PRIMARY].
> {
> msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
> goto cleanup;
>
Otherwise this looks good. I can also live with leaving key_ctx_bi in
for now. The current version of the patch is a clear improvement (and
paving the way for 2/2, ofc.)
So, as long as at least the whitespace is fixed:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix client's poor man NCP fallback
Hi,
On 14-08-2020 10:06, Arne Schwabe wrote:
> OpenVPN 2.5 clients do not correctly do a fallback to the server server.
> This commit fixes that logic and also fixes --data-ciphers-fallback to
> be used in situations other than no OCC cipher.
>
> To reproduce the error use a client with only --data-ciphers set against
> a server without NCP.
>
> OPTIONS ERROR: failed to negotiate cipher with server.
> Add the server's cipher ('AES-256-CBC') to --data-ciphers
> (currently 'AES-256-CBC') if you want to connect to this server.
>
> Reported by: Richard Bonhomme
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/ssl_ncp.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index f522b8f0..c9ab85ce 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -296,13 +296,14 @@ check_pull_client_ncp(struct context *c, const int
> found)
> }
> /* If the server did not push a --cipher, we will switch to the
> * remote cipher if it is in our ncp-ciphers list */
> -bool useremotecipher = tls_poor_mans_ncp(&c->options,
> -
> c->c2.tls_multi->remote_ciphername);
> -
> +if(tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername))
> +{
> +return true;
> +}
>
> /* We could not figure out the peer's cipher but we have fallback
> * enabled */
> -if (!useremotecipher && c->options.enable_ncp_fallback)
> +if (!c->c2.tls_multi->remote_ciphername &&
> c->options.enable_ncp_fallback)
> {
> return true;
> }
>
This makes sense. Given that the commit message is fixed as suggested by
Richard:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 2/3] Move openvpn specific key expansion into its own function
Hi,
On 12-08-2020 16:01, Arne Schwabe wrote:
> This moves the OpenVPN specific PRF into its own function also
> simplifies the code a bit by passing tls_session directly instead of
> 5 of its fields.
Moves in the right direction. Some comments though:
> Signed-off-by: Arne Schwabe
>
> Patch V2: Rebase
> ---
> src/openvpn/ssl.c | 109 +-
> 1 file changed, 69 insertions(+), 40 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 3fcaa25f..06cc4c0b 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1765,27 +1765,38 @@ openvpn_PRF(const uint8_t *secret,
> VALGRIND_MAKE_READABLE((void *)output, output_len);
> }
>
> -/*
> - * Using source entropy from local and remote hosts, mix into
> - * master key.
> - */
> -static bool
> -generate_key_expansion(struct key_ctx_bi *key,
> - const struct key_type *key_type,
> - const struct key_source2 *key_src,
> - const struct session_id *client_sid,
> - const struct session_id *server_sid,
> - bool server)
> +static void
> +init_key_contexts(struct key_ctx_bi *key,
> + const struct key_type *key_type,
> + bool server,
> + struct key2 *key2)
> +{
> +/* Initialize OpenSSL key contexts */
Since you're touching this, can you remove "OpenSSL" from this comment.
It's a lie! Sometimes.
> +int key_direction = server ? KEY_DIRECTION_INVERSE :
> KEY_DIRECTION_NORMAL;
> +init_key_ctx_bi(key, key2, key_direction, key_type, "Data Channel");
> +
> +/* Initialize implicit IVs */
> +key_ctx_update_implicit_iv(&key->encrypt, (*key2).keys[(int)server].hmac,
> + MAX_HMAC_KEY_LENGTH);
> +key_ctx_update_implicit_iv(&key->decrypt,
> (*key2).keys[1-(int)server].hmac,
> + MAX_HMAC_KEY_LENGTH);
> +
> +}
> +
> +
> +static struct key2
> +generate_key_expansion_oepnvpn_prf(const struct tls_session *session)
What is this oepnvpn you're talking about?
> {
> +
Unneeded newline.
> uint8_t master[48] = { 0 };
> -struct key2 key2 = { 0 };
> -bool ret = false;
>
> -if (key->initialized)
> -{
> -msg(D_TLS_ERRORS, "TLS Error: key already initialized");
> -goto exit;
> -}
> +const struct key_state *ks = &session->key[KS_PRIMARY];
> +const struct key_source2 *key_src = ks->key_src;
> +
> +const struct session_id *client_sid = session->opt->server ?
> + &ks->session_id_remote :
> &session->session_id;
> +const struct session_id *server_sid = !session->opt->server ?
> + &ks->session_id_remote :
> &session->session_id;
>
> /* debugging print of source key material */
> key_source2_print(key_src);
> @@ -1803,6 +1814,7 @@ generate_key_expansion(struct key_ctx_bi *key,
> master,
> sizeof(master));
>
> +struct key2 key2;
> /* compute key expansion */
> openvpn_PRF(master,
> sizeof(master),
> @@ -1815,41 +1827,62 @@ generate_key_expansion(struct key_ctx_bi *key,
> server_sid,
> (uint8_t *)key2.keys,
> sizeof(key2.keys));
> +secure_memzero(&master, sizeof(master));
>
> +/* We use the DES fixup here so we can drop it once we
> + * drop DES support and non RFC5705 key derivation */
> +for (int i = 0; i < 2; ++i)
> +{
> +fixup_key(&key2.keys[i], &session->opt->key_type);
> +}
Very nice that we can finally get rid of this. But I'm wondering now:
should we forbid DES (or maybe even any small block cipher) when using EKM?
Even if we remove small block cipher support completely from 2.6, I'd
still really like to backport EKM to 2.5 ("long-term compat"). At that
will still allow persistent users to force it to use DES.
> key2.n = 2;
>
> -key2_print(&key2, key_type, "Master Encrypt", "Master Decrypt");
> +return key2;
> +}
> +
> +/*
> + * Using source entropy from local and remote hosts, mix into
> + * master key.
> + */
> +static bool
> +generate_key_expansion(struct key_ctx_bi *key,
> + const struct tls_session *session)
> +{
> +bool ret = false;
> +
> +if (key->initialized)
> +{
> +msg(D_TLS_ERRORS, "TLS Error: key already initialized");
> +goto exit;
> +}
> +
> +
> +bool server = session->opt->server;
> +
> +struct key2 key2 = generate_key_expansion_oepnvpn_prf(session);
Possible use-before-initialization/declaration here (undefined behavior).
key2 is used in the exit: label, so it should not be declared after the
"goto exit" above. This is type of mistake is the reason I don't like
late-declaration of any variable in functions that use a goto-error
flow. Next to not-too-smart static analyzers complain
Re: [Openvpn-devel] [PATCH v3 1/3] Refactor key_state_export_keying_material functions
truct external_context {
> void *sign_ctx;
> };
>
> +/** struct to cache TLS secrets for keying material exporter (RFC 5705).
> + * The constants (64 and 48) are inherent to TLS version and
> + * the whole keying material export will likely change when they change */
> +struct tls_key_cache {
> +unsigned char client_server_random[64];
> +mbedtls_tls_prf_types tls_prf_type;
> +unsigned char master_secret[48];
> +};
> +
> /**
> * Structure that wraps the TLS context. Contents differ depending on the
> * SSL library used.
> @@ -114,8 +123,7 @@ struct key_state_ssl {
> mbedtls_ssl_context *ctx; /**< mbedTLS connection context */
> bio_ctx *bio_ctx;
>
> -/** Keying material exporter cache (RFC 5705). */
> -uint8_t *exported_key_material;
> +struct tls_key_cache tls_key_cache;
>
> };
>
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 5ba74402..f52c7c39 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -158,35 +158,26 @@ tls_ctx_initialised(struct tls_root_ctx *ctx)
> return NULL != ctx->ctx;
> }
>
> -void
> -key_state_export_keying_material(struct key_state_ssl *ssl,
> - struct tls_session *session)
> -{
> -if (session->opt->ekm_size > 0)
> -{
> -unsigned int size = session->opt->ekm_size;
> -struct gc_arena gc = gc_new();
> -unsigned char *ekm = (unsigned char *) gc_malloc(size, true, &gc);
> +unsigned char*
> +key_state_export_keying_material(struct tls_session *session,
> + const char* label, size_t label_size,
> + size_t ekm_size,
> + struct gc_arena *gc)
>
> -if (SSL_export_keying_material(ssl->ssl, ekm, size,
> - session->opt->ekm_label,
> - session->opt->ekm_label_size,
> - NULL, 0, 0))
> -{
> -unsigned int len = (size * 2) + 2;
> +{
> +unsigned char *ekm = (unsigned char *) gc_malloc(ekm_size, true, gc);
>
> -const char *key = format_hex_ex(ekm, size, len, 0, NULL, &gc);
> -setenv_str(session->opt->es, "exported_keying_material", key);
> +SSL* ssl = session->key[KS_PRIMARY].ks_ssl.ssl;
>
> -dmsg(D_TLS_DEBUG_MED, "%s: exported keying material: %s",
> - __func__, key);
> -}
> -else
> -{
> -msg(M_WARN, "WARNING: Export keying material failed!");
> -setenv_del(session->opt->es, "exported_keying_material");
> -}
> -gc_free(&gc);
> +if (SSL_export_keying_material(ssl, ekm, ekm_size, label,
> + label_size, NULL, 0, 0) == 1)
> +{
> +return ekm;
> +}
> +else
> +{
> +secure_memzero(ekm, ekm_size);
> +return NULL;
> }
> }
>
>
Thanks, looks good to me now. Changes wrt v2 in comments and whitespace
only, so didn't re-test.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Refactor key_state_export_keying_material functions
Hi, On 12-08-2020 16:01, Arne Schwabe wrote: > This refactors the common code between mbed SSL and OpenSSL into > export_user_keying_material and also prepares the backend functions > to export more than one key. > > Also fix checking the return value of SSL_export_keying_material > only 1 is a sucess, -1 is also an error. > > Signed-off-by: Arne Schwabe > > Patch V2: Cache secrets for mbed TLS instead generating all ekms > in the call back function > > Signed-off-by: Arne Schwabe > --- > > [...] > > --- a/src/openvpn/ssl_backend.h > +++ b/src/openvpn/ssl_backend.h > @@ -394,13 +394,23 @@ void backend_tls_ctx_reload_crl(struct tls_root_ctx > *ssl_ctx, > * derived from existing TLS channel. This exported keying material can then > be > * used for a variety of purposes. > * > + * This extra newline seems unneeded. > * @param ks_ssl The SSL channel's state info > * @param session The session associated with the given key_state > + * @param labelThe label to use when exporting the key > + * @param label_size The size of the label to use when exporting the key > + * > + * @param gc gc_arena that might be used to allocate the string > + * returned > + * @returnsThe exported key material, the caller may zero the > + * string but should not free it > */ > > -void > -key_state_export_keying_material(struct key_state_ssl *ks_ssl, > - struct tls_session *session) > __attribute__((nonnull)); > +unsigned char* > +key_state_export_keying_material(struct tls_session *session, > + const char* label, size_t label_size, > + size_t ekm_size, > + struct gc_arena *gc) > __attribute__((nonnull)); (Quoting a former colleague:) comment is a lie! It does not match the actual function prototype. All the code itself looks good to me. I *think* it could be made slightly better even, but these changes make sense for the intended goal, and look correct. Tested both openssl and mbedtls builds (with -fsanitize=address), and verified that this indeed produces the same exported values for both builds and a build without this patch. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/3] Refactor key_state_export_keying_material functions
Hi,
Couldn't resist giving this a quick look.
Feature-ACK on the patch set, but some comments on the approach:
On 12-08-2020 10:55, Arne Schwabe wrote:
> This refactors the common code between mbed SSL and OpenSSL into
> export_user_keying_material and also prepares the backend functions
> to export more than one key.
>
> Also fix checking the return value of SSL_export_keying_material
> only 1 is a sucess, -1 is also an error.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/ssl.c | 33 -
> src/openvpn/ssl_backend.h | 18 --
> src/openvpn/ssl_mbedtls.c | 22 ++---
> src/openvpn/ssl_openssl.c | 51 +--
> 4 files changed, 83 insertions(+), 41 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index f16114c2..390114e1 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2412,6 +2412,37 @@ error:
> return false;
> }
>
> +static void
> +export_user_keying_material(struct key_state_ssl *ssl,
> +struct tls_session *session)
> +{
> +if (session->opt->ekm_size > 0)
> +{
> +unsigned int size = session->opt->ekm_size;
> +struct gc_arena gc = gc_new();
> +
> +unsigned char *ekm;
> +if ((ekm = key_state_export_keying_material(ssl, session,
> +EXPORT_KEY_USER, &gc)))
> +{
Hmm, I don't think these abstractions are right. I would argue that the
crypto backends should know as few about OpenVPN specifics as possible.
So things like "EXPORT_KEY_USER" should probably not be passed to the
backend, and instead be handled in ssl.c.
Why not give the backend a prototype like
key_state_export_keying_material(label, label_len, ekm, ekm_len)
I understand that this would mean an extra memcpy() for the mbedtls
code, but this is not performance-critical at all, right?
(In the follow-up patch, just cache the master secret and client/server
random, instead of the derived key material in the mbedtls backend, so
you can generate the export at will. For OpenSSL, this API should be
trivial.)
> +unsigned int len = (size * 2) + 2;
> +
> +const char *key = format_hex_ex(ekm, size, len, 0, NULL, &gc);
> +setenv_str(session->opt->es, "exported_keying_material", key);
> +
> +dmsg(D_TLS_DEBUG_MED, "%s: exported keying material: %s",
> + __func__, key);
> +secure_memzero(ekm, size);
> +}
> +else
> +{
> +msg(M_WARN, "WARNING: Export keying material failed!");
> +setenv_del(session->opt->es, "exported_keying_material");
> +}
> +gc_free(&gc);
> +}
> +}
> +
> /**
> * Handle reading key data, peer-info, username/password, OCC
> * from the TLS control channel (cleartext).
> @@ -2541,7 +2572,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi
> *multi, struct tls_sessio
> if ((ks->authenticated > KS_AUTH_FALSE)
> && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL))
> {
> -key_state_export_keying_material(&ks->ks_ssl, session);
> +export_user_keying_material(&ks->ks_ssl, session);
>
> if (plugin_call(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL,
> NULL, NULL, session->opt->es) != OPENVPN_PLUGIN_FUNC_SUCCESS)
> {
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index 7f52ab1e..8faaefd5 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -389,18 +389,32 @@ void key_state_ssl_free(struct key_state_ssl *ks_ssl);
> void backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx,
> const char *crl_file, bool crl_inline);
>
> +
> +/* defines the different RFC5705 that are used in OpenVPN */
Make this /** to have it turn up nicely in the doxygen.
> +enum export_key_identifier {
> +EXPORT_KEY_USER
> +};
> +
> /**
> * Keying Material Exporters [RFC 5705] allows additional keying material to
> be
> * derived from existing TLS channel. This exported keying material can then
> be
> * used for a variety of purposes.
> *
> + * Note
> + *
Note ... what?
> * @param ks_ssl The SSL channel's state info
> * @param session The session associated with the given key_state
> + * @param key The key to export.
> + * @param gc gc_arena that might be used to allocate a string
Where "a string" is "the keying material", right? Maybe just say that :)
> + * @returnsThe exported key material, the caller may zero the
> + * string but should not free it
> */
>
> -void
> +unsigned char*
> key_state_export_keying_material(struct key_state_ssl *ks_ssl,
> - struct tls_session *session)
> __attribute__((nonnull));
> + struct tls_session *session,
> +
Re: [Openvpn-devel] [PATCH applied] Re: Remove S_OP_NORMAL key state.
On 11-08-2020 10:45, Gert Doering wrote: > Acked-by: Gert Doering > > Server-side and client-side tested. > > Not sure if I understand all possible implications of S_NORMAL_OP, > but indeed it is not *used* anywere, except in ">= S_ACTIVE". > > The flow of "at which point in time we set must_negotiate = 0" > changes a bit - the old code would do it "when it expired AND > we're in >= S_ACTIVE", while the new code would do it "right > when setting S_ACTIVE" - which is the only place where S_ACTIVE > is set, so it would always catch said condition. This should be > totally fine. > > Your patch has been applied to the master branch. FWIW: I agree, I don't think we need S_NORMAL_OP. Less code, more better. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default
Hi,
No full review yet, but this version does seem to address my previous
comments. Some minor nits I noticed on my first run through v2:
On 29-07-2020 13:38, Arne Schwabe wrote:
> This reworks the NCP logic to be more strict about what is
> considered an acceptable result of an NCP negotiation. It also
> us to finally drop BF-CBC support by default.
>
> All new behaviour is currently limited to server/client
> mode with pull enabled. P2p mode without pull does not change.
>
> New Server behaviour:
> - when a client announces its supported ciphers through either
> OCC or IV_CIPHER/IV_NCP we reject the client with a
> AUTH_FAILED message if we have no common cipher.
>
> - When a client does not announce any cipher in either
> OCC or NCP we by reject it unless fallback-cipher is
> specified in either ccd or config.
>
> New client behaviour:
> - When no cipher is pushed (or a cipher we refused to support)
> and we also cannot support the server's server announced in
> OCC we fail the connection and log why
>
> - If fallback-cipher is specified we will in the case that
> cipher is missing from occ use the fallback cipher instead
> of failing the connection
>
> Both client and server behaviour:
> - We only announce --cipher xyz in occ if we are willing
> to support that cipher.
>
> It means that we only announce the fallback-cipher if
> it is also contained in --data-ciphers
>
> Compatiblity behaviour:
>
> In 2.5 both client and server will automatically set
> fallback-cipher xyz if --cipher xyz is in the config and
> also add append the cipher to the end of data-ciphers.
>
> We log a warn user about this and point to --data-ciphers and
> --fallback-cipher. This also happens if the configuration
> contains an explicit --cipher BF-CBC.
>
> If --cipher is not set, we only warn that previous versions
> allowed BF-CBC and point how to reenable BF-CBC. This might
> break very few config where someone connects a very old
> client to a 2.5 server but at some point we need to drop
> the BF-CBC and those affected use will already have a the
> scary SWEET32 warning in their logs.
>
> In short: If --cipher is explicitly set 2.6 will work the same as
> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
> we warn about it.
>
> Examples how breaking the default BF-CBC will be logged:
>
> Client side:
> - Client connecting to server that does not push cipher but
>has --cipher in OCC
>
> OPTIONS ERROR: failed to negotiate cipher with server. Add the server's
> cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if
> you want to connect to this server.
>
> - Client connecting to a server that does not support OCC:
>
>OPTIONS ERROR: failed to negotioate cipher with server. Configure
> --fallback-cipher if you want connect to this server.
>
> Server Side:
>
> - Server has a client only supporting BF-CBC connecting:
>
> styx/IP PUSH: No common cipher between server and client. Server
> data-ciphers:
> 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client
> supports cipher 'BF-CBC'.
>
> - Client without OCC:
>
>styx/IP PUSH:No NCP or OCC cipher data received from peer.
>styx/IP Use --fallback-cipher with the cipher the client is using if you
> want to allow the client to connect
>
> In all reject cases on the client:
>
>AUTH: Received control message: AUTH_FAILED,Data channel cipher
> negotiation failed (no shared cipher)
>
> Signed-off-by: Arne Schwabe
>
> Patch V2: rename fallback-cipher to data-ciphers-fallback
> add all corrections from Steffan
> Ignore occ cipher for clients sending IV_CIPHERS
> move client side ncp in its own function
> do not print INSECURE cipher warning if BF-CBC is not allowed
>
> Signed-off-by: Arne Schwabe
> ---
> doc/man-sections/protocol-options.rst | 22 -
> src/openvpn/crypto.c | 4 +-
> src/openvpn/init.c| 18 ++--
> src/openvpn/multi.c | 135 --
> src/openvpn/options.c | 117 +-
> src/openvpn/options.h | 2 +
> src/openvpn/ssl.c | 17 ++--
> src/openvpn/ssl_ncp.c | 82 +---
> src/openvpn/ssl_ncp.h | 18 ++--
> tests/unit_tests/openvpn/test_ncp.c | 26 +++--
> 10 files changed, 311 insertions(+), 130 deletions(-)
>
> diff --git a/doc/man-sections/protocol-options.rst
> b/doc/man-sections/protocol-options.rst
> index 051f1d32..69d3bc67 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -57,6 +57,9 @@ configured in a compatible way between both the local and
> remote side.
>http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
>
> --cipher alg
> + This option is deprecated for server-client mode and ``--data-ciphers``
> + or rarely `--data-ciphe
Re: [Openvpn-devel] [PATCH 9/9] Rework NCP compability logic and drop BF-CBC support by default
Hi,
This is awesome in many ways. Better behaviour, better code and a nice
way forward to really get rid of the BF-CBC default cipher.
It's also somewhat tricky, so here goes for a review purely based on
stare-at-code:
On 17-07-2020 15:47, Arne Schwabe wrote:
> This reworks the NCP logic to be more strict about what is
> considered an acceptable result of an NCP negotiation. It also
> us to finally drop BF-CBC support by default.
>
> All new behaviour is currently limited to server/client
> mode with pull enabled. P2p mode without pull does not change.
>
> New Server behaviour:
> - when a client announces its supported ciphers through either
> OCC or IV_CIPHER/IV_NCP we reject the client with a
> AUTH_FAILED message if we have no common cipher.
>
> - When a client does not announce any cipher in either
> OCC or NCP we by reject it unless fallback-cipher is
> specified in either ccd or config.
>
> New client behaviour:
> - When no cipher is pushed (or a cipher we refused to support)
> and we also cannot support the server's server announced in
> OCC we fail the connection and log why
>
> - If fallback-cipher is specified we will in the case that
> cipher is missing from occ use the fallback cipher instead
> of failing the connection
>
> Both client and server behaviour:
> - We only announce --cipher xyz in occ if we are willing
> to support that cipher.
>
> It means that we only announce the fallback-cipher if
> it is also contained in --data-ciphers
>
> Compatiblity behaviour:
>
> In 2.5 both client and server will automatically set
> fallback-cipher xyz if --cipher xyz is in the config and
> also add append the cipher to the end of data-ciphers.
>
> We log a warn user about this and point to --data-ciphers and
> --fallback-cipher. This also happens if the configuration
> contains an explicit --cipher BF-CBC.
>
> If --cipher is not set, we only warn that previous versions
> allowed BF-CBC and point how to reenable BF-CBC. This might
> break very few config where someone connects a very old
> client to a 2.5 server but at some point we need to drop
> the BF-CBC and those affected use will already have a the
> scary SWEET32 warning in their logs.
>
> In short: If --cipher is explicitly set 2.6 will work the same as
> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
> we warn about it.
>
> Examples how breaking the default BF-CBC will be logged:
>
> Client side:
> - Client connecting to server that does not push cipher but
>has --cipher in OCC
>
> OPTIONS ERROR: failed to negotiate cipher with server. Add the server's
> cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if
> you want to connect to this server.
>
> - Client connecting to a server that does not support OCC:
>
>OPTIONS ERROR: failed to negotioate cipher with server. Configure
> --fallback-cipher if you want connect to this server.
>
> Server Side:
>
> - Server has a client only supporting BF-CBC connecting:> diff --git
> a/doc/man-sections/protocol-options.rst
b/doc/man-sections/protocol-options.rst
[ Used the output of "git diff -w" to review, so pasting that here is
"original" too. ]
> diff --git a/doc/man-sections/protocol-options.rst
> b/doc/man-sections/protocol-options.rst
> index 051f1d32..ab22b415 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -57,6 +57,9 @@ configured in a compatible way between both the local and
> remote side.
>http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
>
> --cipher alg
> + This option is deprecated for server-client mode and ``--data-ciphers``
> + or rarely `--data-ciphers-fallback`` should be used instead.
> +
>Encrypt data channel packets with cipher algorithm ``alg``.
>
>The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
> @@ -183,8 +186,9 @@ configured in a compatible way between both the local and
> remote side.
>``--server`` ), or if ``--pull`` is specified (client-side, implied by
>setting --client).
>
> - If both peers support and do not disable NCP, the negotiated cipher will
> - override the cipher specified by ``--cipher``.
> + If no common cipher is found is found during cipher negotiation, the
Duplicate "is found".
> + connection is terminated. To support old clients/server that do not
> + provide any cipher support see ``data-ciphers-fallback``.
>
>Additionally, to allow for more smooth transition, if NCP is enabled,
>OpenVPN will inherit the cipher of the peer if that cipher is different
> @@ -201,8 +205,18 @@ configured in a compatible way between both the local
> and remote side.
>This list is restricted to be 127 chars long after conversion to OpenVPN
>ciphers.
>
> - This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> - to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> + This option was called ``--ncp
[Openvpn-devel] [PATCH] Gently push users towards --data-ciphers in --show-ciphers output
Also:
* fix a typo in the openssl output ("may be use*d*")
* mention GCM before CBC (we prefer AEAD modes)
Signed-off-by: Steffan Karger
---
src/openvpn/crypto_mbedtls.c | 5 +++--
src/openvpn/crypto_openssl.c | 10 +-
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 19a87eb4..fbb1f120 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -149,8 +149,9 @@ show_available_ciphers(void)
#ifndef ENABLE_SMALL
printf("The following ciphers and cipher modes are available for use\n"
"with " PACKAGE_NAME ". Each cipher shown below may be used as a\n"
- "parameter to the --cipher option. Using a CBC or GCM mode is\n"
- "recommended. In static key mode only CBC mode is allowed.\n\n");
+ "parameter to the --data-ciphers (or --cipher) option. Using a\n"
+ "GCM or CBC mode is recommended. In static key mode only CBC\n"
+ "mode is allowed.\n\n");
#endif
while (*ciphers != 0)
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index c47c2f3c..c60d4a54 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -287,11 +287,11 @@ show_available_ciphers(void)
size_t num_ciphers = 0;
#ifndef ENABLE_SMALL
printf("The following ciphers and cipher modes are available for use\n"
- "with " PACKAGE_NAME ". Each cipher shown below may be use as a\n"
- "parameter to the --cipher option. The default key size is\n"
- "shown as well as whether or not it can be changed with the\n"
- "--keysize directive. Using a CBC or GCM mode is recommended.\n"
- "In static key mode only CBC mode is allowed.\n\n");
+ "with " PACKAGE_NAME ". Each cipher shown below may be used as a\n"
+ "parameter to the --data-ciphers (or --cipher) option. The\n"
+ "default key size is shown as well as whether or not it can be\n"
+ "changed with the --keysize directive. Using a GCM or CBC mode\n"
+ "is recommended. In static key mode only CBC mode is
allowed.\n\n");
#endif
for (nid = 0; nid < 1; ++nid)
--
2.25.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers
n.
> diff --git a/doc/man-sections/server-options.rst
> b/doc/man-sections/server-options.rst
> index c24aec0b..74ad5e18 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -473,8 +473,8 @@ fast hardware. SSL/TLS authentication must be used in
> this mode.
> *AES-GCM-128* and *AES-GCM-256*.
>
>:code:`IV_CIPHERS=`
> -The client pushes the list of configured ciphers with the
> -``--ciphers`` option to the server.
> +The client announces the list of supported ciphers configured with
> the
> +``--data-ciphers`` option to the server.
>
>:code:`IV_GUI_VER= `
> The UI version of a UI if one is running, for example
> diff --git a/sample/sample-config-files/client.conf
> b/sample/sample-config-files/client.conf
> index 7f2f30a3..47ca4099 100644
> --- a/sample/sample-config-files/client.conf
> +++ b/sample/sample-config-files/client.conf
> @@ -112,7 +112,7 @@ tls-auth ta.key 1
> # then you must also specify it here.
> # Note that v2.4 client/server will automatically
> # negotiate AES-256-GCM in TLS mode.
> -# See also the ncp-cipher option in the manpage
> +# See also the data-ciphers option in the manpage
> cipher AES-256-CBC
>
> # Enable compression on the VPN link.
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 88ba9db2..d2549bca 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1827,7 +1827,7 @@ multi_client_set_protocol_options(struct context *c)
> else
> {
> /*
> - * Push the first cipher from --ncp-ciphers to the client that
> + * Push the first cipher from --data-ciphers to the client that
> * the client announces to be supporting.
> */
> char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers,
> o->ciphername,
> @@ -1847,7 +1847,7 @@ multi_client_set_protocol_options(struct context *c)
> {
> msg(M_INFO, "PUSH: No common cipher between server and "
> "client. Expect this connection not to work. Server "
> -"ncp-ciphers: '%s', client supported ciphers '%s'",
> +"data-ciphers: '%s', client supported ciphers '%s'",
> o->ncp_ciphers, peer_ciphers);
> }
> else
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 31e33ae3..896abcde 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -536,7 +536,7 @@ static const char usage_message[] =
> "--cipher alg: Encrypt packets with cipher algorithm alg\n"
> " (default=%s).\n"
> " Set alg=none to disable encryption.\n"
> -"--ncp-ciphers list : List of ciphers that are allowed to be
> negotiated.\n"
> +"--data-ciphers list : List of ciphers that are allowed to be
> negotiated.\n"
> "--ncp-disable : (DEPRECATED) Disable cipher negotiation.\n"
> "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
> " nonce_secret_len=nsl. Set alg=none to disable
> PRNG.\n"
> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
> VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
> options->ciphername = p[1];
> }
> -else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
> +else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
> +&& p[1] && !p[2])
> {
> VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
> options->ncp_ciphers = p[1];
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index e057a40b..6760884e 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -111,7 +111,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena
> *gc)
> const cipher_kt_t *ktc = cipher_kt_get(token);
> if (!ktc)
> {
> -msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
> +msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
> error_found = true;
> }
> else
> @@ -130,7 +130,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena
> *gc)
> if (!(buf_forward_capacity(&new_list) >
>strlen(ovpn_cipher_name) + 2))
> {
> -msg(M_WARN, "Length of --ncp-ciphers is over the "
> +msg(M_WARN, "Length of --data-ciphers is over the "
> "limit of 127 chars");
> error_found = true;
> }
>
Thanks. Patch looks good, and passes manual tests.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers
Hi, On 23-07-2020 18:09, David Sommerseth wrote: >> This was a deliberate decision. We really want to people to move towards >> ncp and putting another hurdle with having an option that works better >> on but gives a warning and a option that does not work on 2.4 does not >> help here. If we decide that really aliases are a no-go in OpenVPN then >> I would rather drop data-ciphers and stay with ncp-ciphers forever for >> this reason. > > Lets take a few steps back try to see a broader picture. > > [..snip..] > > We really need a proper and sane processes to allow the development of OpenVPN > to have a chance to move on and leave things behind when appropriate, to be > able to evolve and grow with the future - without being strangled by what > existed in the far past (meaning: no longer community supported releases). > Otherwise I do fear for the future of OpenVPN 2.x. > > By having a clear strategy and adhering to a process of feature/option > management in OpenVPN, we give clearly defined time-window for stability and > functionality for our users. This predictability is, in my experience, much > more important to users than if a specifically named option is supported or > not. Yes, you've made these points clear earlier on IRC. I (and with me Arne and Gert) just don't agree with you on some of the details, resulting in a different verdict on this patch. None of us has trouble with deprecating options. We appreciate the work you've put into the DeprecatedOptions page, and all of us have sent and/or acked patched to remove dangerous or obsolete options. This difference is in how we weigh the pros and cons per option. So I'll leave the broader picture for now, and summarize why I'm going to ACK Arne's patch exactly because is *doesn't* print a warning when the old name is used. Option name aliases add negligible code complexity and are trivial to maintain. (Just look at --udp-mtu.) Keeping them in allows users to write configs that work well and do not produce any warnings on both older and newer versions. (Printing warnings for harmless things reduces the value of the other warnings we print.) Let's focus our time and effort on reducing actual complexity. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Require AEAD support in the crypto library
gt; cipher_ctx_get_tag(EVP_CIPHER_CTX *ctx, uint8_t *tag_buf, int tag_size)
> {
> -#ifdef HAVE_AEAD_CIPHER_MODES
> return EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, tag_size, tag_buf);
> -#else
> -ASSERT(0);
> -#endif
> }
>
> int
> @@ -841,16 +827,12 @@ cipher_ctx_reset(EVP_CIPHER_CTX *ctx, const uint8_t
> *iv_buf)
> int
> cipher_ctx_update_ad(EVP_CIPHER_CTX *ctx, const uint8_t *src, int src_len)
> {
> -#ifdef HAVE_AEAD_CIPHER_MODES
> int len;
> if (!EVP_CipherUpdate(ctx, NULL, &len, src, src_len))
> {
> crypto_msg(M_FATAL, "%s: EVP_CipherUpdate() failed", __func__);
> }
> return 1;
> -#else /* ifdef HAVE_AEAD_CIPHER_MODES */
> -ASSERT(0);
> -#endif
> }
>
> int
> @@ -874,7 +856,6 @@ int
> cipher_ctx_final_check_tag(EVP_CIPHER_CTX *ctx, uint8_t *dst, int *dst_len,
> uint8_t *tag, size_t tag_len)
> {
> -#ifdef HAVE_AEAD_CIPHER_MODES
> ASSERT(tag_len < SIZE_MAX);
> if (!EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_TAG, tag_len, tag))
> {
> @@ -882,9 +863,6 @@ cipher_ctx_final_check_tag(EVP_CIPHER_CTX *ctx, uint8_t
> *dst, int *dst_len,
> }
>
> return cipher_ctx_final(ctx, dst, dst_len);
> -#else /* ifdef HAVE_AEAD_CIPHER_MODES */
> -ASSERT(0);
> -#endif
> }
>
> void
> diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h
> index 4694ee08..e6f8f537 100644
> --- a/src/openvpn/crypto_openssl.h
> +++ b/src/openvpn/crypto_openssl.h
> @@ -61,13 +61,9 @@ typedef HMAC_CTX hmac_ctx_t;
> /** Cipher is in CFB mode */
> #define OPENVPN_MODE_CFBEVP_CIPH_CFB_MODE
>
> -#ifdef HAVE_AEAD_CIPHER_MODES
> -
> /** Cipher is in GCM mode */
> #define OPENVPN_MODE_GCMEVP_CIPH_GCM_MODE
>
> -#endif /* HAVE_AEAD_CIPHER_MODES */
> -
> /** Cipher should encrypt */
> #define OPENVPN_OP_ENCRYPT 1
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 04518bf5..94308a8e 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -104,9 +104,7 @@ const char title_string[] =
> " [MH/RECVDA]"
> #endif
> #endif
> -#ifdef HAVE_AEAD_CIPHER_MODES
> " [AEAD]"
> -#endif
> " built on " __DATE__
> ;
>
> @@ -871,11 +869,7 @@ init_options(struct options *o, const bool init_gc)
> o->scheduled_exit_interval = 5;
> #endif
> o->ciphername = "BF-CBC";
> -#ifdef HAVE_AEAD_CIPHER_MODES /* IV_NCP=2 requires GCM support */
> o->ncp_enabled = true;
> -#else
> -o->ncp_enabled = false;
> -#endif
> o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
> o->authname = "SHA1";
> o->prng_hash = "SHA1";
>
Thanks. This looks good to me now, and passes travis and basic local tests.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/9] Require AEAD support in the crypto library
Hi, On 17-07-2020 15:47, Arne Schwabe wrote: > All supported crypto libraries have AEAD support and with our > ncp/de facto default cipher AES-256-GCM we do not want to support > the obscure corner case of a library with disabled AEAD. Again: feature-ACK, but some comments. config-msvc.h still has a #define HAVE_AEAD_CIPHER_MODES 1 and ssl_openssl.c has two occurances of #ifdef EVP_CIPH_FLAG_AEAD_CIPHER which I think can go too now. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 2/9] Drop support for OpenSSL 1.0.1
e = session->opt->ekm_size;
> struct gc_arena gc = gc_new();
> unsigned char *ekm = (unsigned char *) gc_malloc(size, true, &gc);
> @@ -188,7 +187,6 @@ key_state_export_keying_material(struct key_state_ssl
> *ssl,
> setenv_del(session->opt->es, "exported_keying_material");
> }
> gc_free(&gc);
> -#endif /* if (OPENSSL_VERSION_NUMBER >= 0x10001000) */
> }
> }
>
> @@ -559,7 +557,7 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const
> char *profile)
> #else /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
> if (profile)
> {
> -msg(M_WARN, "WARNING: OpenSSL 1.0.1 does not support
> --tls-cert-profile"
> +msg(M_WARN, "WARNING: OpenSSL 1.0.2 does not support
> --tls-cert-profile"
> ", ignoring user-set profile: '%s'", profile);
> }
> #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
> @@ -573,19 +571,11 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>
> ASSERT(ctx);
>
> -#if (OPENSSL_VERSION_NUMBER >= 0x10002000L &&
> !defined(LIBRESSL_VERSION_NUMBER)) \
> -|| LIBRESSL_VERSION_NUMBER >= 0x207fL
> -/* OpenSSL 1.0.2 and up */
> cert = SSL_CTX_get0_certificate(ctx->ctx);
> -#else
> -/* OpenSSL 1.0.1 and earlier need an SSL object to get at the
> certificate */
> -SSL *ssl = SSL_new(ctx->ctx);
> -cert = SSL_get_certificate(ssl);
> -#endif
>
> if (cert == NULL)
> {
> -goto cleanup; /* Nothing to check if there is no certificate */
> +return; /* Nothing to check if there is no certificate */
> }
>
> ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
> @@ -607,13 +597,6 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
> {
> msg(M_WARN, "WARNING: Your certificate has expired!");
> }
> -
> -cleanup:
> -#if OPENSSL_VERSION_NUMBER < 0x10002000L \
> -|| (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <
> 0x207fL)
> -SSL_free(ssl);
> -#endif
> -return;
> }
>
> void
> @@ -680,7 +663,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const
> char *curve_name
> }
> else
> {
> -#if OPENSSL_VERSION_NUMBER >= 0x10002000L
> #if (OPENSSL_VERSION_NUMBER < 0x1010L &&
> !defined(LIBRESSL_VERSION_NUMBER))
>
> /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
> @@ -691,29 +673,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const
> char *curve_name
> * so do nothing */
> #endif
> return;
> -#else /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */
> -/* For older OpenSSL we have to extract the curve from key on our
> own */
> -EC_KEY *eckey = NULL;
> -const EC_GROUP *ecgrp = NULL;
> -EVP_PKEY *pkey = NULL;
> -
> -/* Little hack to get private key ref from SSL_CTX, yay OpenSSL... */
> -SSL *ssl = SSL_new(ctx->ctx);
> -if (!ssl)
> -{
> -crypto_msg(M_FATAL, "SSL_new failed");
> -}
> -pkey = SSL_get_privatekey(ssl);
> -SSL_free(ssl);
> -
> -msg(D_TLS_DEBUG, "Extracting ECDH curve from private key");
> -
> -if (pkey != NULL && (eckey = EVP_PKEY_get1_EC_KEY(pkey)) != NULL
> -&& (ecgrp = EC_KEY_get0_group(eckey)) != NULL)
> -{
> -nid = EC_GROUP_get_curve_name(ecgrp);
> -}
> -#endif /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */
> }
>
> /* Translate NID back to name , just for kicks */
> @@ -1462,15 +1421,7 @@ tls_ctx_use_management_external_key(struct
> tls_root_ctx *ctx)
>
> ASSERT(NULL != ctx);
>
> -#if (OPENSSL_VERSION_NUMBER >= 0x10002000L &&
> !defined(LIBRESSL_VERSION_NUMBER)) \
> -|| LIBRESSL_VERSION_NUMBER >= 0x207fL
> -/* OpenSSL 1.0.2 and up */
> X509 *cert = SSL_CTX_get0_certificate(ctx->ctx);
> -#else
> -/* OpenSSL 1.0.1 and earlier need an SSL object to get at the
> certificate */
> -SSL *ssl = SSL_new(ctx->ctx);
> -X509 *cert = SSL_get_certificate(ssl);
> -#endif
>
> ASSERT(NULL != cert);
>
> @@ -1510,13 +1461,6 @@ tls_ctx_use_management_external_key(struct
> tls_root_ctx *ctx)
>
> ret = 0;
> cleanup:
> -#if OPENSSL_VERSION_NUMBER < 0x10002000L \
> -|| (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <
> 0x207fL)
> -if (ssl)
> -{
> -SSL_free(ssl);
> -}
> -#endif
> if (ret)
> {
> crypto_msg(M_FATAL, "Cannot enable SSL external private key
> capability");
>
Otherwise this now looks good to me. So if the whitespace can fixed when
committing:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/9] Indicate that a client is in pull mode in IV_PROTO
Hi,
On 17-07-2020 15:47, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
Feature-ACK, clever use of existing infra. Some comments though:
This commit message could use a bit more information. In particular, it
would be good to explain how the various OpenVPN versions currently
interpret IV_PROTO values, and why the change from a version-like
interpretation to a bitfield will not cause issues.
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/multi.c | 12 ++--
> src/openvpn/ssl.c | 15 +--
> src/openvpn/ssl.h | 7 +++
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 0095c871..88ba9db2 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c)
> {
> int proto = 0;
> int r = sscanf(optstr, "IV_PROTO=%d", &proto);
> -if ((r == 1) && (proto >= 2))
> +if (r == 1)
> {
> -tls_multi->use_peer_id = true;
> +if (proto >= 2)
> +{
> +tls_multi->use_peer_id = true;
> +}
Wouldn't checking for proto & IV_PROTO_REQUEST_PUSH suffice? Any 2.3/2.4
client will only ever send "2", and newer clients will know this is a
bitfield now.
This works fine, but - in particular without an explaining comment -
first checking for a value, then continue as a bitfield is somewhat
surprising.
> +if (proto & IV_PROTO_REQUEST_PUSH)
> +{
> +c->c2.push_request_received = true;
> +}
> }
> +
> }
>
> /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session
> *session)
> buf_printf(&out, "IV_PLAT=win\n");
> #endif
>
> -/* support for P_DATA_V2 */
> -buf_printf(&out, "IV_PROTO=2\n");
> +/* support for P_DATA_V2*/
> +int iv_proto = IV_PROTO_DATA_V2;
> +
> +/* support for receiving push_reply before sending
> + * push request, also signal that the client wants
> + * to get push-reply messages without without requiring a round
> + * trip for a push request message*/
> +if(session->opt->pull)
This needs a space behind the if.
> +{
> +iv_proto |= IV_PROTO_REQUEST_PUSH;
> +}
> +
> +buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
>
> /* support for Negotiable Crypto Parameters */
> if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..86fb6853 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,13 @@
> /* Maximum length of OCC options string passed as part of auth handshake */
> #define TLS_OPTIONS_LEN 512
>
> +/* Definitions of the bits in the IV_PROTO bitfield */
> +#define IV_PROTO_DATA_V2(1<<1) /**< Support P_DATA_V2 */
> +#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push
> + * request and server does not need
> + * to wait for a push-request to
> send
> + * a push-reply */
> +
Style nit: would this not be more readable if you place the (rather
long) comment above the value, instead of behind?
Also: any particular reason to no use 1<<0 ? If we should not use that
for some reason, let's mark it as reserved. Otherwise: maybe just use it?
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Remove key-method 1
Hi
On 13-07-2020 11:46, Arne Schwabe wrote:
> @@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct
> link_socket_info *lsi, boo
> floated, &ad_start))
> {
> /* Restore pre-NCP frame parameters */
> -if (is_hard_reset(opcode, c->options.key_method))
> +if (is_hard_reset(opcode, KEY_METHOD_2))
> {
Can't we just remove the key_method parameter from is_hard_reset()?
> @@ -3817,10 +3803,9 @@ options_string(const struct options *o,
> * tls-auth/tls-crypt does not match. Removing tls-auth here
> would
> * break stuff, so leaving that in place. */
>
> -if (o->key_method > 1)
> -{
> -buf_printf(&out, ",key-method %d", o->key_method);
> -}
> +
> +
> +buf_printf(&out, ",key-method %d", 2);
This could do with one less newline.
> @@ -2404,7 +2348,7 @@ key_method_2_write(struct buffer *buf, struct
> tls_session *session)
> }
>
> /* write key_method + flags */
> -if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
> +if (!buf_write_u8(buf, (KEY_METHOD_2 & KEY_METHOD_MASK)))
> {
> goto error;
> }
The masking looks a bit silly now. Maybe replace with a static_assert()
if you want to be sure that no "wrong" bits are set?
> static bool
> key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct
> tls_session *session)
> {
> struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
>
> -int key_method_flags;
> bool username_status, password_status;
>
> struct gc_arena gc = gc_new();
> @@ -2582,8 +2464,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi
> *multi, struct tls_sessio
> /* allocate temporary objects */
> ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
>
> -ASSERT(session->opt->key_method == 2);
> -
> /* discard leading uint32 */
> if (!buf_advance(buf, 4))
> {
> @@ -2593,7 +2473,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi
> *multi, struct tls_sessio
> }
>
> /* get key method */
> -key_method_flags = buf_read_u8(buf);
> +int key_method_flags = buf_read_u8(buf);
This variable declaration is now *after* a "goto error" jump. Currently
not harmful, because the variable isn't used after the error label, but
static analyzers might complain about "jump past initialization".
Otherwise this looks good based on stare-at-code. Didn't have time to
test yet.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/3] Drop support for OpenSSL 1.0.1
Hi,
Feature-ACK for sure. Some comments below.
On 13-07-2020 11:46, Arne Schwabe wrote:
> OpenSSL 1.0.1 was supported until 2016-12-31. Rhel6/Centos6 still
> use this version but considering that RHEL7 and RHEL8 are already
> out, these versions can also stay with OpenVPN 2.4.
>
> All the supported Debian based distributions also come with at
> least 1.0.2
>
> This also allows the tls groups commit to be applied without
> adding ifdefs to disable that functionality on OpenSSL 1.0.1
>
> Signed-off-by: Arne Schwabe
> ---
> .travis.yml | 8
> Changes.rst | 5 -
> configure.ac | 3 +--
> src/openvpn/crypto.c | 7 ---
> src/openvpn/openssl_compat.h | 14 --
> src/openvpn/ssl_openssl.c| 32 +---
> 6 files changed, 6 insertions(+), 63 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 925d09ea..101ff096 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -35,10 +35,6 @@ jobs:
>env: SSLLIB="openssl" RUN_COVERITY="1"
>os: linux
>compiler: gcc
> -- name: gcc | openssl-1.0.1u
> - env: SSLLIB="openssl" OPENSSL_VERSION="1.0.1u"
> - os: linux
> - compiler: gcc
> - name: gcc | openssl-1.1.1d
>env: SSLLIB="openssl" OPENSSL_VERSION="1.1.1d"
>os: linux
> @@ -87,10 +83,6 @@ jobs:
>env: SSLLIB="mbedtls"
>os: osx
>compiler: clang
> -- name: mingw64 | openssl-1.0.1u
> - env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32 OPENSSL_VERSION="1.0.1u"
> - os: linux
> - compiler: ": Win64 build only"
> - name: mingw64 | openssl-1.1.1d
>env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32 OPENSSL_VERSION="1.1.1d"
>os: linux
> diff --git a/Changes.rst b/Changes.rst
> index 42f0d190..d45dc900 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -31,7 +31,10 @@
> https://community.openvpn.net/openvpn/wiki/DeprecatedOptions
> With the improved and matured data channel cipher negotiation, the use
> of ``ncp-disable`` should not be necessary anymore.
>
> -
> +- Support for building with OpenSSL 1.0.1 has been removed. The minimum
> + supported OpenSSL version is now 1.0.2.
> +
> +
This adds some trailing whitespace.
> Overview of changes in 2.4
> ==
>
> diff --git a/configure.ac b/configure.ac
> index 53b7a967..8194d8c2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -839,7 +839,7 @@ if test "${with_crypto_library}" = "openssl"; then
> # if the user did not explicitly specify flags, try to
> autodetect
> PKG_CHECK_MODULES(
> [OPENSSL],
> - [openssl >= 1.0.1],
> + [openssl >= 1.0.2],
> [have_openssl="yes"],
> [] # If this fails, we will do another test next
> )
There's a similar check for the no-pkgconfig fallback case. That should
be updated too:
# If pkgconfig check failed or OPENSSL_CFLAGS/OPENSSL_LIBS env vars
# are used, check the version directly in the OpenSSL include file
if test "${have_openssl}" != "yes"; then
AC_MSG_CHECKING([additionally if OpenSSL is available and
version >= 1.0.1])
AC_COMPILE_IFELSE(
[AC_LANG_PROGRAM(
[[
#include
]],
[[
/* Version encoding: MNNFFPPS - see opensslv.h for details */
#if OPENSSL_VERSION_NUMBER < 0x10001000L
#error OpenSSL too old
#endif
]]
)],
[AC_MSG_RESULT([ok])],
[AC_MSG_ERROR([OpenSSL version too old])]
)
fi
AC_CHECK_FUNCS([SSL_CTX_new EVP_CIPHER_CTX_set_key_length],
,
[AC_MSG_ERROR([openssl check failed])]
)
> @@ -931,7 +931,6 @@ if test "${with_crypto_library}" = "openssl"; then
> X509_STORE_get0_objects \
> X509_OBJECT_free \
> X509_OBJECT_get_type \
> - EVP_PKEY_id \
> EVP_PKEY_get0_RSA \
> EVP_PKEY_get0_DSA \
> EVP_PKEY_get0_EC_KEY \
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 1ce98184..bbf47ef7 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -428,13 +428,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer
> work,
> tag_ptr = BPTR(buf);
> ASSERT(buf_advance(buf, tag_size));
> dmsg(D_PACKET_CONTENT, "DECRYPT MAC: %s", format_hex(tag_ptr, tag_size,
> 0, &gc));
> -#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER < 0x10001040L
> -/* OpenSSL <= 1.0.1c bug requires set tag before processing ciphertext */
> -if (!EVP_CIPHER_CTX_ctrl(ctx->cipher, EVP_CTRL_GCM_SET_TAG, tag_size,
> tag_ptr))
> -{
> -CRYPT_ERROR("setting tag failed");
> -}
> -#endif
>
> if (buf->len < 1)
>
Re: [Openvpn-devel] [PATCH 2/3] Cleanup: Remove unused code of old poor man's NCP.
Hi,
As discusses in #openvpn-devel on IRC, this patch breaks interop with
clients that don't pull, but that will be restored in a follow-up
refactoring (before 2.5 rc1). I can live with that, but I think this
should be mentioned in the commit message.
On 07-07-2020 14:16, Arne Schwabe wrote:
> Ever since the NCPv2 the ncp_get_best_cipher uses the global
> options->ncp_enabled option and ignore the tls_session->ncp_enabled
> option.
>
> The server side's poor man's NCP is implemented as seeing the list
> of supported ciphers from the peer as just one cipher so this special
> handling for poor man's NCP of the older NCP here is not needed anymore.
>
> Theoretically we can now get rid of tls_session->ncp_enabled but doing
> so requires more refactoring since options is not available in the
> methods that still use it. And when we remove ncp-disable the variable
> will be removed anyway.
>
> Also document the remaining usage of tls_poor_mans_ncp better.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/init.c | 2 ++
> src/openvpn/ssl.c | 15 +--
> 2 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 91b919d5..e9c01629 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2376,6 +2376,8 @@ do_deferred_options(struct context *c, const unsigned
> int found)
> }
> else if (c->options.ncp_enabled)
> {
> +/* If the server did not push a --cipher, we will switch to the
> + * remote cipher if it is in our ncp-ciphers list */
> tls_poor_mans_ncp(&c->options,
> c->c2.tls_multi->remote_ciphername);
> }
> struct frame *frame_fragment = NULL;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 9df7552d..71565dd3 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2463,8 +2463,7 @@ key_method_2_write(struct buffer *buf, struct
> tls_session *session)
> * generation is postponed until after the pull/push, so we can process
> pushed
> * cipher directives.
> */
> -if (session->opt->server && !(session->opt->ncp_enabled
> - && session->opt->mode == MODE_SERVER &&
> ks->key_id <= 0))
> +if (session->opt->server && !(session->opt->mode == MODE_SERVER &&
> ks->key_id <= 0))
> {
> if (ks->authenticated != KS_AUTH_FALSE)
> {
> @@ -2616,18 +2615,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi
> *multi, struct tls_sessio
> multi->remote_ciphername =
> options_string_extract_option(options, "cipher", NULL);
>
> -if (!tls_peer_supports_ncp(multi->peer_info))
> -{
> -/* Peer does not support NCP, but leave NCP enabled if the local and
> - * remote cipher do not match to attempt 'poor-man's NCP'.
> - */
> -if (multi->remote_ciphername == NULL
> -|| 0 == strcmp(multi->remote_ciphername,
> multi->opt.config_ciphername))
> -{
> -session->opt->ncp_enabled = false;
> -}
> -}
> -
This removes the last user of tls_peer_supports_ncp(), should that be
removed too?
Other than this I think this looks good and moves in the right
direction. I haven't tested this thoroughly. Since much of the logic
will be changing soon anyway, I think it's okay to move forward
nevertheless. But we do need some aggressive testing when all the
changes are in.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Make key_state->authenticated more state machine like
Hi,
On 07-07-2020 18:20, Antonio Quartulli wrote:
> On 07/07/2020 14:16, Arne Schwabe wrote:
>> This order the states from unauthenticated to authenticated and also
>> changes the comparison for KS_AUTH_FALSE from != to >
>>
>> Also remove a now obsolete comment and two obsolete ifdefs. While
>> keeping the ifdef in ssl_verify would save a few bytes of code,
>> this is too minor to justify keeping the ifdef
>>
>> Signed-off-by: Arne Schwabe
>> ---
>> src/openvpn/ssl.c| 6 +++---
>> src/openvpn/ssl_common.h | 7 ++-
>> src/openvpn/ssl_verify.c | 15 ---
>> 3 files changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>> index 71565dd3..c73b51c3 100644
>> --- a/src/openvpn/ssl.c
>> +++ b/src/openvpn/ssl.c
>> @@ -2465,7 +2465,7 @@ key_method_2_write(struct buffer *buf, struct
>> tls_session *session)
>> */
>> if (session->opt->server && !(session->opt->mode == MODE_SERVER &&
>> ks->key_id <= 0))
>> {
>> -if (ks->authenticated != KS_AUTH_FALSE)
>> +if (ks->authenticated > KS_AUTH_FALSE)
>> {
>> if (!tls_session_generate_data_channel_keys(session))
>> {
>> @@ -2646,7 +2646,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi
>> *multi, struct tls_sessio
>> secure_memzero(up, sizeof(*up));
>>
>> /* Perform final authentication checks */
>> -if (ks->authenticated != KS_AUTH_FALSE)
>> +if (ks->authenticated > KS_AUTH_FALSE)
>> {
>> verify_final_auth_checks(multi, session);
>> }
>> @@ -2671,7 +2671,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi
>> *multi, struct tls_sessio
>> * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final
>> * veto opportunity over authentication decision.
>> */
>> -if ((ks->authenticated != KS_AUTH_FALSE)
>> +if ((ks->authenticated > KS_AUTH_FALSE)
>> && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL))
>> {
>> key_state_export_keying_material(&ks->ks_ssl, session);
>> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
>> index fdf589b5..7d841ffb 100644
>> --- a/src/openvpn/ssl_common.h
>> +++ b/src/openvpn/ssl_common.h
>> @@ -129,8 +129,8 @@ struct key_source2 {
>>
>> enum ks_auth_state {
>>KS_AUTH_FALSE,
>
> I suggest to add comments here describing how states work and how new
> states should be added.
>
> I.e. something like: "any state before or equal to KS_AUTH_FALSE is
> considered unauthorized". Not sure the terminology is right, but
> something like this would help introducing new states, like Steffan
> reported before.
+1
Don't care much about the terminology. As long as the intention from the
quote from Antonio is immediately obvious from the code.
> Maybe he has additional comments too.
Nope, otherwise I think this looks good.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Make openvpn --version exit with exit code 0
For some reason, openvpn --version has since the beginning of time returned exit code 1. A quick sample among common unix utilities confirms that the rest of the world agrees with me that 0 makes more sense. Let's make openvpn --version exit with exit code 0 too. Signed-off-by: Steffan Karger --- src/openvpn/options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 2073b4a6..a72b677a 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4326,7 +4326,7 @@ usage_version(void) msg(M_INFO|M_NOPREFIX, "special build: %s", CONFIGURE_SPECIAL_BUILD); #endif #endif -openvpn_exit(OPENVPN_EXIT_STATUS_USAGE); /* exit point */ +openvpn_exit(OPENVPN_EXIT_STATUS_GOOD); } void -- 2.17.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] merge key_state->authenticated and key_state->auth_deferred
Hi,
Didn't find time to fully review, but I think this is moving into the
right direction. I did notice something I'd like you to consider:
On 06-07-2020 18:35, Arne Schwabe wrote:
> @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct
> tls_session *session)
> if (session->opt->server && !(session->opt->ncp_enabled
>&& session->opt->mode == MODE_SERVER &&
> ks->key_id <= 0))
> {
> -if (ks->authenticated)
> +if (ks->authenticated != KS_AUTH_FALSE)
> {
Statements like these can be risky. I'm not sure how likely enum
ks_auth_state is to grow another state, but I could imagine someone
adding an explicit KS_AUTH_FAILED or so.
I think this patch should at least document the enum better, and clearly
state assumptions like "KS_AUTH_FALSE is the only state that represents
auth failure".
Or perhaps we can make is less fragile by modeling the enum like a state
machine and documenting allowed state transitions. That would allow you
to write things like "if (ks->authenticated < KS_AUTH_TRUE)" to express
"any state before authentication succeeded".
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] systemd: Change the default cipher to AES-256-GCM for server configs
On 22-06-2020 19:59, David Sommerseth wrote: > On 22/06/2020 14:43, Steffan Karger wrote: >> On 22-06-2020 14:29, David Sommerseth wrote: >>> On 22/06/2020 14:21, Arne Schwabe wrote: >>>> >>>>> PrivateTmp=true >>>>> WorkingDirectory=/etc/openvpn/server >>>>> -ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log >>>>> --status-version 2 --suppress-timestamps --config %i.conf >>>>> +ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log >>>>> --status-version 2 --suppress-timestamps --cipher AES-256-GCM >>>>> --ncp-ciphers AES-256-GCM:AES-128-GCM:AES-256-CBC:AES-128-CBC:BF-CBC >>>>> --config %i.conf >>>>> CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE >>>>> CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE >>>>> CAP_AUDIT_WRITE >>>>> LimitNPROC=10 >>>>> DeviceAllow=/dev/null rw >>>>> >>>> >>>> NACK. >>>> >>>> Setting ncp-cipher to include BF-CBC by default allows BF-CBC in configs >>>> that did not allow it before. Basically any config that had something >>>> other than cipher BF-CBC and no ncp-ciphers in it will now allow clients >>>> with BF-CBC to connect. I don't want force users to set ncp-cipher to a >>>> sane value since the systemd unit file doesn't. >>> >>> That will break existing configs on the next upgrade. Do we want do do >>> that? >>> >>> I'm fine with removing BF-CBC, but it is scheduled for removal in OpenVPN >>> 2.6. >> >> I think Arne has a very good point that it's kinda weird to "degrade" >> the NCP defaults. >> >> Making AES-256-GCM the default cipher for TLS-based connections (GCM >> won't work with static key configs) does not imply *removing* BF-CBC >> support. Maybe these should be the steps: >> >> 2.4: Use to AES-256-GCM when available (basically what NCP did) >> 2.5: Switch to AES-256-GCM as the default cipher (but allow overriding) >> 2.6: Remove support for small block ciphers all together >> >> Yes, this will probably break some (less secure) setups and make some >> people angry. But at some point people need to move on. We've been >> throwing warnings at them for a while now. > > Yes, I agree that Arne got a point. But I'm not completely convinced breaking > configs in OpenVPN 2.5 without a prior note that it will stop working. Our > warning only screams "you shouldn't use ciphers with block sizes < 128 bits", > it doesn't say "this will stop working in a future release". > > OpenVPN 2.4.9 gives this warning: > > WARNING: INSECURE cipher with block size less than 128 bit (64 bit). This > allows attacks like SWEET32. Mitigate by using a --cipher with a larger > block size (e.g. AES-256-CBC). > > The community has been informed it will stop working in 2.6, not before this > release. > > This must be documented properly and log warnings updated to indicate > short-term workarounds. > > I could be willing to consider breaking configs for ciphers in a later 2.5.x > release as long as users are properly warned *when* it will stop working - and > that users gets a real chance to fix configs before we do break their configs > - where a workaround approach could be considered and available from v2.5.0 > (on the other hand, if they change their configs, they should swap ciphers > instead of applying a workaround for a dying cipher - but for some it might be > a bit more complicated to do such a swap). > > We need to find a good middle-way for OpenVPN 2.5, where we clearly signal > "weak ciphers will be removed" and when we will do that. While also moving > forward and break as few configs as possible and not make configs weaker than > before. I agree the warning we log should be made even more scary. The DeprecatedOptions wiki however already says: Removal of insecure ciphers: Ciphers with cipher block-size less than 128 bits (most commonly BF, DES, CAST5, IDEA and RC2) Deprecated in: OpenVPN v2.4 To be removed in: OpenVPN v2.6 What I'm proposing is a step in between for something that is long overdue: update the *defaults* to no longer use insecure ciphers. But to cater for those that are unprepared, allow them to explicitly re-enable BF-CBC during a transition period. They've been ignoring our advice to migrate for a long time now, and even the wiki page says that these ciphers are already deprecated in 2.4. -Steffan signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] systemd: Change the default cipher to AES-256-GCM for server configs
Hi,
On 22-06-2020 16:01, Arne Schwabe wrote:
> Am 22.06.20 um 14:43 schrieb Steffan Karger:
>> Maybe these should be the steps:
>>
>> 2.4: Use to AES-256-GCM when available (basically what NCP did)
>> 2.5: Switch to AES-256-GCM as the default cipher (but allow overriding)
>> 2.6: Remove support for small block ciphers all together
>>
>> Yes, this will probably break some (less secure) setups and make some
>> people angry. But at some point people need to move on. We've been
>> throwing warnings at them for a while now.
>
> I had a different suggestion in the channel:
>
> - Deprecate ncp-disable. Reason: Was a good debug switch when introduced
> should not be necessary anymore.
Deprecate in 2.5, remove in 2.6 makes sense to me.
Should we do the same for --cipher in P2MP configs?
> - Introduce ncp-fallback-cipher for compatibility with ncp-disable/old
> versions
> - needs to be a cipher from ncp-ciphers list
> - Eventually default the first cipher from ncp-ciphers list
> - in 2.5 default to --cipher if --cipher is set and automatically
> add cipher to ncp-ciphers and set ncp-fallback-cipher.
>
> - If cipher is not set
> a) warn about that cipher will be ignored in p2mp mode and if BF-CBC is
> still needed (e.g. peer 2.3 or older) that ncp-fallback-cipher should be set
> b) same a) but do it automatically for the user+warn
>
> This allows us to eventually get rid of --cipher while providing still a
> smooth transaction.
That does sounds more friendly than my proposal.
But do we really need an extra option? Does is really differ so much
from changing the default cipher to AES-256-CBC? Both proposals require
config changes to keep old clients working with 2.5+ if --cipher wasn't
set explicitly.
We already have a lot of things in place to cater for a smooth transition:
- 2.4+ does NCP by default.
- Any 2.4+ client always sends OCC strings, which makes "poor man's
NCP" work for --ncp-disable clients as long as their cipher is in the
server's --ncp-ciphers list.
- pre-2.4 clients almost always send OCC strings too, except for
ENABLE_SMALL builds.
- If NCP and poor-man's NCP fail, 2.5 can (if --cipher is explicitly
set) fall back to whatever is in --cipher and print a big fat warning
that this connection will break in 2.6.
- If someone *really* wants to have 2.3 ENABLE_SMALL clients connect to
a 2.6 server, they can run a separate server for those clients with only
the cipher of their choice in --ncp-ciphers ("Fall back to the first
cipher", as you proposed).
Basically: if we can agree to no longer guarantee interoperability
between 2.6+ and 2.3- ("mostly just works, but special cases - in
particular ENABLE_SMALL builds - might break"), I don't think we need to
add yet another option.
-Steffan
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] systemd: Change the default cipher to AES-256-GCM for server configs
Hi, On 22-06-2020 14:29, David Sommerseth wrote: > On 22/06/2020 14:21, Arne Schwabe wrote: >> >>> PrivateTmp=true >>> WorkingDirectory=/etc/openvpn/server >>> -ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log >>> --status-version 2 --suppress-timestamps --config %i.conf >>> +ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log >>> --status-version 2 --suppress-timestamps --cipher AES-256-GCM --ncp-ciphers >>> AES-256-GCM:AES-128-GCM:AES-256-CBC:AES-128-CBC:BF-CBC --config %i.conf >>> CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE >>> CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE >>> CAP_AUDIT_WRITE >>> LimitNPROC=10 >>> DeviceAllow=/dev/null rw >>> >> >> NACK. >> >> Setting ncp-cipher to include BF-CBC by default allows BF-CBC in configs >> that did not allow it before. Basically any config that had something >> other than cipher BF-CBC and no ncp-ciphers in it will now allow clients >> with BF-CBC to connect. I don't want force users to set ncp-cipher to a >> sane value since the systemd unit file doesn't. > > That will break existing configs on the next upgrade. Do we want do do that? > > I'm fine with removing BF-CBC, but it is scheduled for removal in OpenVPN 2.6. I think Arne has a very good point that it's kinda weird to "degrade" the NCP defaults. Making AES-256-GCM the default cipher for TLS-based connections (GCM won't work with static key configs) does not imply *removing* BF-CBC support. Maybe these should be the steps: 2.4: Use to AES-256-GCM when available (basically what NCP did) 2.5: Switch to AES-256-GCM as the default cipher (but allow overriding) 2.6: Remove support for small block ciphers all together Yes, this will probably break some (less secure) setups and make some people angry. But at some point people need to move on. We've been throwing warnings at them for a while now. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/3] Make cipher_kt_name always return normalised cipher name
d_secret_file)
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 9ed6ff5f..042b0ce0 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -116,8 +116,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena
> *gc)
> }
> else
> {
> -const char *ovpn_cipher_name =
> -translate_cipher_name_to_openvpn(cipher_kt_name(ktc));
> +const char *ovpn_cipher_name = cipher_kt_name(ktc);
>
> if (buf_len(&new_list)> 0)
> {
>
Makes sense, changes look good and passes my tests.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/3] Make cipher_kt_get also accept OpenVPN config cipher name
Hi,
On 05-06-2020 13:25, Arne Schwabe wrote:
> Basically calls to cipher_kt_get were calling
> translate_cipher_name_from_openvpn. The only two exception were the
> (broken) unit test and tls-crypt that uses cipher_kt_get("AES-256-CTR")
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto.c | 2 +-
> src/openvpn/crypto_backend.h | 3 ++-
> src/openvpn/crypto_mbedtls.c | 1 +
> src/openvpn/crypto_openssl.c | 1 +
> src/openvpn/ssl_ncp.c| 8
> 5 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index ba1fc095..1ce98184 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -763,7 +763,7 @@ init_key_type(struct key_type *kt, const char *ciphername,
> CLEAR(*kt);
> if (strcmp(ciphername, "none") != 0)
> {
> -kt->cipher =
> cipher_kt_get(translate_cipher_name_from_openvpn(ciphername));
> +kt->cipher = cipher_kt_get(ciphername);
> if (!kt->cipher)
> {
> msg(M_FATAL, "Cipher %s not supported", ciphername);
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index d46cb63f..85cb084a 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -227,7 +227,8 @@ void cipher_des_encrypt_ecb(const unsigned char
> key[DES_KEY_LENGTH],
> * initialise encryption/decryption.
> *
> * @param ciphernameName of the cipher to retrieve parameters for (e.g.
> - * \c AES-128-CBC).
> + * \c AES-128-CBC). Will be translated to the library
> name
> + * from the openvpn config name if needed.
> *
> * @return A statically allocated structure containing
> parameters
> * for the given cipher, or NULL if no matching
> parameters
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index c8dcddc8..bb752557 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -465,6 +465,7 @@ cipher_kt_get(const char *ciphername)
>
> ASSERT(ciphername);
>
> +ciphername = translate_cipher_name_from_openvpn(ciphername);
> cipher = mbedtls_cipher_info_from_string(ciphername);
>
> if (NULL == cipher)
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 13ab4859..bbaa5742 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -580,6 +580,7 @@ cipher_kt_get(const char *ciphername)
>
> ASSERT(ciphername);
>
> +ciphername = translate_cipher_name_from_openvpn(ciphername);
> cipher = EVP_get_cipherbyname(ciphername);
>
> if (NULL == cipher)
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 042b0ce0..ea1dc960 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -103,12 +103,12 @@ mutate_ncp_cipher_list(const char *list, struct
> gc_arena *gc)
> while (token)
> {
> /*
> - * Going through a roundtrip by using
> translate_cipher_name_from_openvpn
> - * and translate_cipher_name_to_openvpn also normalises the cipher
> name,
> + * Going through a roundtrip by using cipher_kt_get/cipher_kt_name
> + * (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
> */
> -const char *cipher_name = translate_cipher_name_from_openvpn(token);
> -const cipher_kt_t *ktc = cipher_kt_get(cipher_name);
> + const cipher_kt_t *ktc = cipher_kt_get(token);
> if (!ktc)
> {
> msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
>
This makes the cipher_kt_name and cipher_kt_get abstractions nicely
consistent. Code looks good, and passes my tests.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [BUG] test_ncp.c failing
On 29-05-2020 01:46, James Bottomley wrote: > The problem seems to be openssl uses a mixed case name for the cipher > and EVP_CIPHER_name() is case sensitive. Applying the patch below > fixes this for openssl and gets make check to pass all tests, but I > rather wonder why this isn't part of cipher_kt_name() to prevent this > type of problem? This was my first thought when looking at the patch too. Probably because I wrote the cipher name translation code as one of my very first OpenVPN contributions, and didn't get the abstractions right at the time. Would be good to see if this can be refactored. Slightly annoying to test if that won't break corner cases though, because I think this part is not sufficiently covered by automated tests. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Switch assertion failure to returning false
Hi,
Thanks for the clear report, patch and follow-up.
On 20-05-2020 23:31, Jeremy Evans wrote:
> On 05/20 09:33, Gert Doering wrote:
>> On Wed, May 20, 2020 at 11:34:04AM -0700, Jeremy Evans wrote:
>>> To give some background, we hit this assertion failure, with the
>>> following log output:
>>
>> This should not happen, asserting out in "normal server use" is bad.
>>
>> (Neither should it ever reach that point without ks->authenticated being
>> true)
Agreed. I think it makes sense to first fix the urgent part (ie, not
kill the server), then figure out how de code ends up at this ASSERT.
Jeremy, can you determine from your logs whether this always
happenedwhen --auth-user-pass-verify returns zero or non-zero? Ie,
should the connections that trigger this succeed or fail?
>>> @@ -1930,7 +1930,10 @@ tls_session_generate_data_channel_keys(struct
>>> tls_session *session)
>>>&ks->session_id_remote :
>>> &session->session_id;
>>>
>>> -ASSERT(ks->authenticated);
>>> +if (!ks->authenticated) {
>>> +msg(D_TLS_ERRORS, "TLS Error: key_state not authenticated");
>>> +goto cleanup;
>>> +}
>>>
>>> ks->crypto_options.flags = session->opt->crypto_flags;
>>> if (!generate_key_expansion(&ks->crypto_options.key_ctx_bi,
>>
>> I'm not sure if that code is correct, though - it will erase key
>> material (in cleanup) without actually having generated a session
>> key. So "bad things might happen later".
>
> The same behavior would happen if generate_key_expansion fails a few
> lines below, so my assumption was it was safe to do so. However, that's
> just an assumption and not even an educated guess.
This change correctly turns "kill the server" into "reject the
connection", which I agree is a good thing.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2
6 +288,7 @@ crypto_pem_decode(const char *name, struct buffer *dst,
> }
>
> mbedtls_pem_free(&ctx);
> +gc_free(&gc);
> return ret;
> }
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index a81dcfd8..4fa65ba8 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -400,9 +400,8 @@ crypto_pem_encode(const char *name, struct buffer *dst,
> BUF_MEM *bptr;
> BIO_get_mem_ptr(bio, &bptr);
>
> -*dst = alloc_buf_gc(bptr->length + 1, gc);
> +*dst = alloc_buf_gc(bptr->length, gc);
> ASSERT(buf_write(dst, bptr->data, bptr->length));
> -buf_null_terminate(dst);
>
> ret = true;
> cleanup:
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index e9f9cc2a..3018c18e 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -702,7 +702,7 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>
> if (!filename || streq(filename, ""))
> {
> -printf("%s\n", BPTR(&client_key_pem));
> +printf("%.*s\n", BLEN(&client_key_pem), BPTR(&client_key_pem));
> client_filename = INLINE_FILE_TAG;
> client_inline = (const char *)BPTR(&client_key_pem);
> }
> diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c
> b/tests/unit_tests/openvpn/test_tls_crypt.c
> index 8406d89d..28bebb6e 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -548,7 +548,8 @@ test_tls_crypt_v2_write_server_key_file(void **state)
> const char *filename = "testfilename.key";
>
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_server_key);
> +expect_memory(__wrap_buffer_write_file, pem, test_server_key,
> + strlen(test_server_key));
> will_return(__wrap_buffer_write_file, true);
>
> tls_crypt_v2_write_server_key_file(filename);
> @@ -561,7 +562,8 @@ test_tls_crypt_v2_write_client_key_file(void **state)
>
> /* Test writing the client key */
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_client_key);
> +expect_memory(__wrap_buffer_write_file, pem, test_client_key,
> + strlen(test_client_key));
> will_return(__wrap_buffer_write_file, true);
>
> /* Key generation re-reads the created file as a sanity check */
> @@ -580,7 +582,8 @@ test_tls_crypt_v2_write_client_key_file_metadata(void
> **state)
>
> /* Test writing the client key */
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_client_key_metadata);
> +expect_memory(__wrap_buffer_write_file, pem, test_client_key_metadata,
> +strlen(test_client_key_metadata));
> will_return(__wrap_buffer_write_file, true);
>
> /* Key generation re-reads the created file as a sanity check */
>
Thanks, this looks good to me now.
The patch has a small merge conflict with the inline-refactoring-fixups,
but that should be easy enough to resolve when applying.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2
Hi,
On 27-04-2020 03:26, Arne Schwabe wrote:
> Change crypto_pem_encode to not put a nul-terminated terminated
> string into the buffer. This was useful for printf but should
> not be written into the file.
>
> Instead do not assume that the buffer is null terminated and
> print only the number of bytes in the buffer. Also fix a
> similar case in printing static key where the 0 byte was
> never added to the buffer
>
> Patch V2: make pem_encode behave more like other similar functions in OpenVPN
> and do not null terminate.
>
> Patch V3: also make the mbed TLS variant of pem_decode behave like other
> similar functions in OpeNVPN and accept a not null-terminated
> buffer.
Thanks, this looks better. Two more things:
> @@ -273,9 +270,25 @@ crypto_pem_decode(const char *name, struct buffer *dst,
> return false;
> }
>
> +/* mbed TLS requires the src to be null-terminated */
> +struct gc_arena gc = gc_new();
> +struct buffer input;
> +
> +if (*(BLAST(src)) == '\0')
> +{
> +input = *src;
> +}
> +else
> +{
> +/* allocate a new buffer to avoid modifying the src buffer */
> +input = alloc_buf_gc(BLEN(src) + 1, &gc);
> +buf_copy(&input, src);
> +buf_null_terminate(&input);
> +}
> +
> size_t use_len = 0;
> mbedtls_pem_context ctx = { 0 };
> -bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer,
> BPTR(src),
> +bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer,
> BPTR(&input),
> NULL, 0, &use_len));
> if (ret && !buf_write(dst, ctx.buf, ctx.buflen))
> {
Maybe a matter of taste, but why not always alloc the buffer? That
simplifies the code, and performance nor memory footprint is relevant
here. Just a suggestion. I'm also fine with keeping it like this.
> diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c
> b/tests/unit_tests/openvpn/test_tls_crypt.c
> index 366b48d5..f4b1f860 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -530,7 +530,8 @@ test_tls_crypt_v2_write_server_key_file(void **state) {
> const char *filename = "testfilename.key";
>
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_server_key);
> +expect_memory(__wrap_buffer_write_file, pem, test_server_key,
> + strlen(test_server_key));
> will_return(__wrap_buffer_write_file, true);
>
> tls_crypt_v2_write_server_key_file(filename);
> @@ -542,7 +543,8 @@ test_tls_crypt_v2_write_client_key_file(void **state) {
>
> /* Test writing the client key */
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_client_key);
> +expect_memory(__wrap_buffer_write_file, pem, test_client_key,
> + strlen(test_client_key));
> will_return(__wrap_buffer_write_file, true);
>
> /* Key generation re-reads the created file as a sanity check */
>
ASAN tells me you're missing another one:
==26863==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x618023d6 at pc 0x004310c8 bp 0x7fffbae53b90 sp 0x7fffbae53338
READ of size 847 at 0x618023d6 thread T0
#0 0x4310c7 in strcmp
(tests/unit_tests/openvpn/tls_crypt_testdriver+0x4310c7)
#1 0x7f9234541c6e (/lib/x86_64-linux-gnu/libcmocka.so.0+0x2c6e)
#2 0x7f92345446f9 in _check_expected
(/lib/x86_64-linux-gnu/libcmocka.so.0+0x56f9)
#3 0x4cd414 in __wrap_buffer_write_file
tests/unit_tests/openvpn/../../.././../openvpn/tests/unit_tests/openvpn/test_tls_crypt.c:108:5
#4 0x4cc23b in tls_crypt_v2_write_client_key_file
tests/unit_tests/openvpn/../../.././../openvpn/src/openvpn/tls_crypt.c:709:15
#5 0x4d0f15 in test_tls_crypt_v2_write_client_key_file_metadata
tests/unit_tests/openvpn/../../.././../openvpn/tests/unit_tests/openvpn/test_tls_crypt.c:592:5
#6 0x7f92345444e0 (/lib/x86_64-linux-gnu/libcmocka.so.0+0x54e0)
#7 0x7f9234544ec4 in _cmocka_run_group_tests
(/lib/x86_64-linux-gnu/libcmocka.so.0+0x5ec4)
#8 0x4cd75a in main
tests/unit_tests/openvpn/../../.././../openvpn/tests/unit_tests/openvpn/test_tls_crypt.c:645:15
#9 0x7f92341fd0b2 in __libc_start_main
/build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
#10 0x41d92d in _start
(tests/unit_tests/openvpn/tls_crypt_testdriver+0x41d92d)
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Uncrustify the tests/unit_tests/ part of our tree.
Hi,
On 26-04-2020 11:54, Gert Doering wrote:
> Apply uncrustify 0.70.1 (FreeBSD port) with our rules to that part
> of the tree, which followed a more compact coding style so far.
> ---
>
> @@ -155,20 +157,21 @@ test_packet_id_write_long_wrap(void **state)
> }
>
> int
> -main(void) {
> +main(void)
> +{
> const struct CMUnitTest tests[] = {
> -cmocka_unit_test_setup_teardown(test_packet_id_write_short,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -cmocka_unit_test_setup_teardown(test_packet_id_write_long,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -
> cmocka_unit_test_setup_teardown(test_packet_id_write_short_prepend,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -
> cmocka_unit_test_setup_teardown(test_packet_id_write_long_prepend,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -cmocka_unit_test_setup_teardown(test_packet_id_write_short_wrap,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -cmocka_unit_test_setup_teardown(test_packet_id_write_long_wrap,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_short,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_long,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_short_prepend,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_long_prepend,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_short_wrap,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_long_wrap,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> };
This now exceeds 80 chars by a fair amount. Perhaps rewrap?
Otherwise this looks good to me. I think having consistent style over
the code base, including tests, would be good.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
Hi, On 26-04-2020 11:34, Gert Doering wrote: > On Sun, Apr 26, 2020 at 11:25:49AM +0200, Steffan Karger wrote: >>>> well, sometimes to adhere to the codestyle, you have to re-arrange code :) >>> >>> "rearrange" and "rewrite in a not easy to understand way" (which looks >>> a bit overthought to me, TBH - unlike "secure memzero" I cannot see an >>> obvious reason why all that volatile would be relevant). >> >> This secure memcmp is relevant to avoid timing side channels in e.g. >> authentication tag compare. Think about the HMAC in our tls-auth/crypt >> and the HMAC of (non-AEAD) data channel packets. > > I do understand why it has to be constant *time*, in regards to "do the > compared buffers differ or not". > > I do not see how all this "volatile" and "copy from pointer to variables > to other stuff" handwaving is going to make any difference wrt constant > time comparison. > > And it hurts my eyes. Pretty it is not. But "let's not try to outsmart the crypto library folks" makes sense to me. The copying stuff is probably just about making some annoying compiler happy. We could probably leave that out, but that makes it harder to see that we follow the mbed internal implementation. When inlining this without volatile keywords, a compiler might at some point become smart enough to realize "hey, this caller only cares about zero/nonzero, not the actual value. And this code will only return zero if *all* bytes are equal, so I can stop comparing as soon as soon as I've found a difference". Up to now, it seems compilers have not gotten this smart. But it's not like we're keeping a close eye on that. So I would love to get rid of the responsibility :-) -Steffan signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [Openvpn-users] new openssl = new OpenVPN release ?
On 22-04-2020 10:27, Jan Just Keijser wrote: > On 22/04/20 10:13, Arne Schwabe wrote: SSL_check_chain() function". Which we don't, I just grepped through our source tree. So, unless I misunderstand something about OpenSSL intricacies, I think we're safe - no new installers needed, and OpenVPN is not in risk. >>> the advisory applies only to application that use the SSL_check_chain() >>> function as part of a TLS 1.3 handshake. AFAIK, iIn OpenVPN 2.4 we don't >>> do anything with TLS 1.3 just yet, so this security advisory does not >>> apply to OpenVPN. Also note that this bug appears only in OpenSSL 1.1.1 >>> [d-f] , so anything older is fine as well. >> Hu? OpenVPN 2.4 supports TLS 1.3 just fine. We have support for it in >> tls-version-min and also tls-ciphersuites which is TLS 1.3 specific. >> >> > what I meant was that OpenVPN 2.4 does not do any *specific* with any of > the new features of TLS 1.3, like the new psk callbacks etc. If the > control session is negotiated using TLS 1.3 then sure, OpenVPN will use > that, but other that OpenVPN does not make use of any of the new > features or crypto algorithms that come with OpenSSL 1.1.1 or TLS 1.3 > (chacha20 anyone ;) ? ) Arne has been working on some of the TLS 1.3-specific features, such as using CHACHA20-POLY1305 for the control channel: $ openvpn --show-tls Available TLS Ciphers, listed in order of preference: For TLS 1.3 and newer (--tls-ciphersuites): TLS_AES_256_GCM_SHA384 TLS_CHACHA20_POLY1305_SHA256 TLS_AES_128_GCM_SHA256 For TLS 1.2 and older (--tls-cipher): TLS-ECDHE-ECDSA-WITH-AES-256-GCM-SHA384 (This is with OpenVPN 2.4.7 from the Ubuntu 20.04 package.) -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
Hi,
On 17-04-2020 17:36, Gert Doering wrote:
> On Fri, Apr 17, 2020 at 03:42:49PM +0200, Antonio Quartulli wrote:
-static inline int
-memcmp_constant_time(const void *a, const void *b, size_t size)
-{
>>>
>>> Not sure I understand the motivation for this change. "Just so uncrustify
>>> stops trying to change this" is not really strong.
>>
>> well, sometimes to adhere to the codestyle, you have to re-arrange code :)
>
> "rearrange" and "rewrite in a not easy to understand way" (which looks
> a bit overthought to me, TBH - unlike "secure memzero" I cannot see an
> obvious reason why all that volatile would be relevant).
This secure memcmp is relevant to avoid timing side channels in e.g.
authentication tag compare. Think about the HMAC in our tls-auth/crypt
and the HMAC of (non-AEAD) data channel packets.
>> On top of that, this is basically allowing us to re-use an existing
>> openssl API when possible.
>
> True, but if that turns out to be a code complication and reduces
> efficiency, I'm not convinced it's the right way to spend our cycles.
>
>>> Also, keeping the "inline" part would be good... this is in the per-packet
>>> path.
>>
>> I am not sure it would work in this case, because the function is
>> defined in a .c file now - it's not inlineable anymore outside of the
>> mbedtls code.
>
> This is why I'm not exactly happy with the change.
>
> We could do it "openvpn style" all in header files, or we could just
> leave the function alone.
This kind of code is a tricky balance between "prevent the compiler from
optimizing it to a not-constant-time implementation" and "as much
performance as we can get". Moving this responsibility to the crypto
library seems like a good idea to me.
And because our recommended data channel ciphers are AEAD ciphers for
which the auth tag compare is handled internally by the crypto library,
I don't care so much for the performance aspect. Want best security? Use
AEAD! Want best performance? Use AEAD!
You get the point. Use AEAD ;-)
-Steffan
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Add tls-crypt-v2 test writing metadata
Hi,
On 20-04-2020 12:44, Arne Schwabe wrote:
> ---
> tests/unit_tests/openvpn/test_tls_crypt.c | 44 +--
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c
> b/tests/unit_tests/openvpn/test_tls_crypt.c
> index b9e3a7a6..91a4d209 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -72,6 +72,24 @@ static const char *test_client_key = \
> "/Z5wtPCAZ0tOzj4ItTI77fBOYRTfEayzHgEr\n"
> "-END OpenVPN tls-crypt-v2 client key-\n";
>
> +
> +/* Has custom metadata of AABBCCDD (base64) */
> +static const char *test_client_key_metadata= \
> +"-BEGIN OpenVPN tls-crypt-v2 client key-\n"
> +"AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4v\n"
> +"MDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5f\n"
> +"YGFiY2RlZmdoaWprbG1ub3BxcnN0dXZ3eHl6e3x9fn+AgYKDhIWGh4iJiouMjY6P\n"
> +"kJGSk5SVlpeYmZqbnJ2en6ChoqOkpaanqKmqq6ytrq+wsbKztLW2t7i5uru8vb6/\n"
> +"wMHCw8TFxsfIycrLzM3Oz9DR0tPU1dbX2Nna29zd3t/g4eLj5OXm5+jp6uvs7e7v\n"
> +"8PHy8/T19vf4+fr7/P3+/2ntp1WCqhcLjJQY/igkjNt3Yb6i0neqFkfrOp2UCDcz\n"
> +"6RSJtPLZbvOOKUHk2qwxPYUsFCnz/IWV6/ZiLRrabzUpS8oSN1HS6P7qqAdrHKgf\n"
> +"hVTHasdSf2UdMTPC7HBgnP9Ll0FhKN0h7vSzbbt7QM7wH9mr1ecc/Mt0SYW2lpwA\n"
> +"aJObYGTyk6hTgWm0g/MLrworLrezTqUHBZzVsu+LDyqLWK1lzJNd66MuNOsGA4YF\n"
> +"fbCsDh8n3H+Cw1k5YNBZDYYJOtVUgBWXheO6vgoOmqDdI0dAQ3hVo9DE+SkCFjgf\n"
> +"l4FY2yLEh9ZVZZrl1eD1Owh/X178CkHrBJYl9LNQSyQEKlDGWwBLQ/pY3qtjctr3\n"
> +"pV62MPQdBo+1lcsjDCJVQA6XUyltas4BKQ==\n"
> +"-END OpenVPN tls-crypt-v2 client key-\n";
> +
> int
> __wrap_parse_line(const char *line, char **p, const int n, const char *file,
>const int line_num, int msglevel, struct gc_arena *gc)
> @@ -520,21 +538,40 @@ test_tls_crypt_v2_write_server_key_file(void **state) {
>
> static void
> test_tls_crypt_v2_write_client_key_file(void **state) {
> + const char *filename = "testfilename.key";
> +
> + /* Test writing the client key */
> + expect_string(__wrap_buffer_write_file, filename, filename);
> + expect_string(__wrap_buffer_write_file, pem, test_client_key);
> + will_return(__wrap_buffer_write_file, true);
> +
> + /* Key generation re-reads the created file as a sanity check */
> + expect_string(__wrap_buffer_read_from_file, filename, filename);
> + will_return(__wrap_buffer_read_from_file, test_client_key);
> +
> + tls_crypt_v2_write_client_key_file(filename, NULL, INLINE_FILE_TAG,
> + test_server_key);
> +}
> +
The indenting of this block is wrong: 2 instead of 4 spaces. This also
makes the changes in this patch harder to see.
> +static void
> +test_tls_crypt_v2_write_client_key_file_metadata(void **state) {
> const char *filename = "testfilename.key";
> +const char *b64metadata = "AABBCCDD";
>
> /* Test writing the client key */
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_client_key);
> +expect_string(__wrap_buffer_write_file, pem, test_client_key_metadata);
> will_return(__wrap_buffer_write_file, true);
>
> /* Key generation re-reads the created file as a sanity check */
> expect_string(__wrap_buffer_read_from_file, filename, filename);
> -will_return(__wrap_buffer_read_from_file, test_client_key);
> +will_return(__wrap_buffer_read_from_file, test_client_key_metadata);
>
> -tls_crypt_v2_write_client_key_file(filename, NULL, INLINE_FILE_TAG,
> +tls_crypt_v2_write_client_key_file(filename, b64metadata,
> INLINE_FILE_TAG,
> test_server_key);
> }
>
> +
> int
> main(void) {
> const struct CMUnitTest tests[] = {
> @@ -576,6 +613,7 @@ main(void) {
> test_tls_crypt_v2_teardown),
> cmocka_unit_test(test_tls_crypt_v2_write_server_key_file),
> cmocka_unit_test(test_tls_crypt_v2_write_client_key_file),
> +cmocka_unit_test(test_tls_crypt_v2_write_client_key_file_metadata),
> };
>
> #if defined(ENABLE_CRYPTO_OPENSSL)
>
Otherwise this looks good. So ACK-if-whitespace-is-fixed :)
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2
Hi,
On 20-04-2020 14:06, Arne Schwabe wrote:
> Change crypto_pem_encode to not put a nul-terminated terminated
> string into the buffer. This was useful for printf but should
> not be written into the file.
>
> Instead do not assume that the buffer is null terminated and
> print only the number of bytes in the buffer. Also fix a
> similar case in printing static key where the 0 byte was
> never added to the buffer
>
> Patch V2: make pem_encode behave more like other similar functions in OpenVPN
> and do not null terminate.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto.c | 4 ++--
> src/openvpn/crypto_mbedtls.c | 4 +++-
> src/openvpn/crypto_openssl.c | 3 +--
> src/openvpn/tls_crypt.c | 2 +-
> tests/unit_tests/openvpn/test_tls_crypt.c | 6 --
> 5 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 1678cba8..b05262e1 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1478,7 +1478,7 @@ write_key_file(const int nkeys, const char *filename)
> /* write key file to stdout if no filename given */
> if (!filename || strcmp(filename, "")==0)
> {
> -printf("%s\n", BPTR(&out));
> +printf("%.*s\n", BLEN(&out), BPTR(&out));
> }
> /* write key file, now formatted in out, to file */
> else if (!buffer_write_file(filename, &out))
> @@ -1887,7 +1887,7 @@ write_pem_key_file(const char *filename, const char
> *pem_name)
>
> if (!filename || strcmp(filename, "")==0)
> {
> -printf("%s\n", BPTR(&server_key_pem));
> +printf("%.*s", BLEN(&server_key_pem), BPTR(&server_key_pem));
> }
> else if (!buffer_write_file(filename, &server_key_pem))
> {
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index 3e77fa9e..14a528af 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -239,10 +239,12 @@ crypto_pem_encode(const char *name, struct buffer *dst,
> return false;
> }
>
> +/* We set the size buf to out_len-1 to NOT include the 0 byte that
> + * mbedtls_pem_write_buffer in its length calculation */
> *dst = alloc_buf_gc(out_len, gc);
> if (!mbed_ok(mbedtls_pem_write_buffer(header, footer, BPTR(src),
> BLEN(src),
>BPTR(dst), BCAP(dst), &out_len))
> -|| !buf_inc_len(dst, out_len))
> +|| !buf_inc_len(dst, out_len-1))
> {
> CLEAR(*dst);
> return false;
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index a81dcfd8..4fa65ba8 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -400,9 +400,8 @@ crypto_pem_encode(const char *name, struct buffer *dst,
> BUF_MEM *bptr;
> BIO_get_mem_ptr(bio, &bptr);
>
> -*dst = alloc_buf_gc(bptr->length + 1, gc);
> +*dst = alloc_buf_gc(bptr->length, gc);
> ASSERT(buf_write(dst, bptr->data, bptr->length));
> -buf_null_terminate(dst);
>
> ret = true;
> cleanup:
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index e9f9cc2a..3018c18e 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -702,7 +702,7 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>
> if (!filename || streq(filename, ""))
> {
> -printf("%s\n", BPTR(&client_key_pem));
> +printf("%.*s\n", BLEN(&client_key_pem), BPTR(&client_key_pem));
> client_filename = INLINE_FILE_TAG;
> client_inline = (const char *)BPTR(&client_key_pem);
> }
> diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c
> b/tests/unit_tests/openvpn/test_tls_crypt.c
> index b9e3a7a6..54fc917d 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -512,7 +512,8 @@ test_tls_crypt_v2_write_server_key_file(void **state) {
> const char *filename = "testfilename.key";
>
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_server_key);
> +expect_memory(__wrap_buffer_write_file, pem, test_server_key,
> + strlen(test_server_key));
> will_return(__wrap_buffer_write_file, true);
>
> tls_crypt_v2_write_server_key_file(filename);
> @@ -524,7 +525,8 @@ test_tls_crypt_v2_write_client_key_file(void **state) {
>
> /* Test writing the client key */
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_client_key);
> +expect_memory(__wrap_buffer_write_file, pem, test_client_key,
> + strlen(test_client_key));
> will_return(__wrap_buffer_write_file, true);
>
> /* Key generation re-reads the created file as a sanity check */
>
Didn't review in detail yet, but this patch now fails "make
Re: [Openvpn-devel] [PATCH] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2
On 06-04-2020 18:00, Arne Schwabe wrote:
> Am 06.04.20 um 17:44 schrieb Steffan Karger:
>> Hi,
>>
>> On 06-04-2020 15:00, Arne Schwabe wrote:
>>> crypto_pem_encode put a nul-terminated terminated string into the
>>> buffer. This is useful for printf but should not be written into
>>> the file.
>>>
>>> Also for static keys, we were missing the nul termination when priting
>>> it to stadout but since the buffer was cleared before, there was always
>>> a NULL byte in the right place. Make it explicit instead.
>>>
>>> Signed-off-by: Arne Schwabe
>>> ---
>>> src/openvpn/crypto.c| 11 +--
>>> src/openvpn/tls_crypt.c | 10 --
>>> 2 files changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>>> index 453cb20a..7af48df0 100644
>>> --- a/src/openvpn/crypto.c
>>> +++ b/src/openvpn/crypto.c
>>> @@ -1477,6 +1477,7 @@ write_key_file(const int nkeys, const char *filename)
>>> /* write key file to stdout if no filename given */
>>> if (!filename || strcmp(filename, "")==0)
>>> {
>>> +buf_null_terminate(&out);
>>> printf("%s\n", BPTR(&out));
>>> }
>>> /* write key file, now formatted in out, to file */
>>> @@ -1888,9 +1889,15 @@ write_pem_key_file(const char *filename, const char
>>> *pem_name)
>>> {
>>> printf("%s\n", BPTR(&server_key_pem));
>>> }
>>> -else if (!buffer_write_file(filename, &server_key_pem))
>>> +else
>>> {
>>> -msg(M_ERR, "ERROR: could not write key file");
>>> +/* crypto_pem_encode null terminates the buffer, do
>>> + * not write this to the file */
>>> +buf_inc_len(&server_key_pem, -1);
>>> +if (!buffer_write_file(filename, &server_key_pem))
>>> +{
>>> +msg(M_ERR, "ERROR: could not write key file");
>>> +}
>>> goto cleanup;
>>> }
>>>
>>> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
>>> index e9f9cc2a..85f2862b 100644
>>> --- a/src/openvpn/tls_crypt.c
>>> +++ b/src/openvpn/tls_crypt.c
>>> @@ -706,9 +706,15 @@ tls_crypt_v2_write_client_key_file(const char
>>> *filename,
>>> client_filename = INLINE_FILE_TAG;
>>> client_inline = (const char *)BPTR(&client_key_pem);
>>> }
>>> -else if (!buffer_write_file(filename, &client_key_pem))
>>> +else
>>> {
>>> -msg(M_FATAL, "ERROR: could not write client key file");
>>> +/* crypto_pem_encode null terminates the buffer, do
>>> + * not write this to the file */
>>> +buf_inc_len(&client_key_pem, -1);
>>> +if (!buffer_write_file(filename, &client_key_pem))
>>> +{
>>> +msg(M_FATAL, "ERROR: could not write client key file");
>>> +}
>>> goto cleanup;
>>> }
>>>
>>>
>>
>> Fix is correct, but I'm not too happy with changing the buffer from
>> being null-terminated to not-null-terminated. This change itself is
>> correct, but it feels like a recipe for mistakes in the future.
>>
>> What would you think about adding a length parameter to
>> buffer_write_file() instead, so that the API there changes to "write len
>> bytes of data to file"?
>
> So there are three alternatives how to things:
>
> a) make pem_encode behave more like other similar functions in OepnVPN
> and do not null terminate. Then we would need to call buf_null_terminate
> like in the static key path
>
> b) do what this patch does
>
> c) have buffer_write_file take a length argument.
>
> I considered c) but it felt wrong because the function currently just
> does a very simple job.
I felt that too, but slightly less so than having buffers that are
sometimes null-terminated and sometimes not. Slight different preference
I guess.
> I did not do a) because that would make printing the buffer etc more
> complicated since either pem_encode needs to allocate a bigger buffer
> than it needs to or we need to copy the buffer into one that allows an
> extra null byte.
I originally discarded this idea, but thinking a bit more about it, I
think it makes sense. You don't have to allocate extra bytes, I think
the "%.*s" format specifier should work just fine:
printf("%.*s", BLEN(buf), BPTR(buf));
> That being said I don't feel stronlgy about this and can send an updated
> patch that is a) or c)
Same here. Hence the fuzzy wording. Does my suggestion make you like c)
more? If you still like b) most, I'm okay with sticking to it. Neither
option excels in beauty :(
-Steffan
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2
Hi,
On 06-04-2020 15:00, Arne Schwabe wrote:
> crypto_pem_encode put a nul-terminated terminated string into the
> buffer. This is useful for printf but should not be written into
> the file.
>
> Also for static keys, we were missing the nul termination when priting
> it to stadout but since the buffer was cleared before, there was always
> a NULL byte in the right place. Make it explicit instead.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto.c| 11 +--
> src/openvpn/tls_crypt.c | 10 --
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 453cb20a..7af48df0 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1477,6 +1477,7 @@ write_key_file(const int nkeys, const char *filename)
> /* write key file to stdout if no filename given */
> if (!filename || strcmp(filename, "")==0)
> {
> +buf_null_terminate(&out);
> printf("%s\n", BPTR(&out));
> }
> /* write key file, now formatted in out, to file */
> @@ -1888,9 +1889,15 @@ write_pem_key_file(const char *filename, const char
> *pem_name)
> {
> printf("%s\n", BPTR(&server_key_pem));
> }
> -else if (!buffer_write_file(filename, &server_key_pem))
> +else
> {
> -msg(M_ERR, "ERROR: could not write key file");
> +/* crypto_pem_encode null terminates the buffer, do
> + * not write this to the file */
> +buf_inc_len(&server_key_pem, -1);
> +if (!buffer_write_file(filename, &server_key_pem))
> +{
> +msg(M_ERR, "ERROR: could not write key file");
> +}
> goto cleanup;
> }
>
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index e9f9cc2a..85f2862b 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -706,9 +706,15 @@ tls_crypt_v2_write_client_key_file(const char *filename,
> client_filename = INLINE_FILE_TAG;
> client_inline = (const char *)BPTR(&client_key_pem);
> }
> -else if (!buffer_write_file(filename, &client_key_pem))
> +else
> {
> -msg(M_FATAL, "ERROR: could not write client key file");
> +/* crypto_pem_encode null terminates the buffer, do
> + * not write this to the file */
> +buf_inc_len(&client_key_pem, -1);
> +if (!buffer_write_file(filename, &client_key_pem))
> +{
> +msg(M_FATAL, "ERROR: could not write client key file");
> +}
> goto cleanup;
> }
>
>
Fix is correct, but I'm not too happy with changing the buffer from
being null-terminated to not-null-terminated. This change itself is
correct, but it feels like a recipe for mistakes in the future.
What would you think about adding a length parameter to
buffer_write_file() instead, so that the API there changes to "write len
bytes of data to file"?
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix off-by-one in tls-crypt-v2 client wrapping with custom metadata
Hi,
On 03-04-2020 11:09, Arne Schwabe wrote:
> Instead of writing at the end of the metadata buffer, the decoded
> base64 data overwrites the opcode as BPTR points to the beginning
> of the buffer and not the current position. Replace with BEND to
> fix this off-by-one
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/tls_crypt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index 37df2ce7..e9f9cc2a 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -664,7 +664,7 @@ tls_crypt_v2_write_client_key_file(const char *filename,
> (int)strlen(b64_metadata),
> TLS_CRYPT_V2_MAX_B64_METADATA_LEN);
> }
> ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_USER, 1));
> -int decoded_len = openvpn_base64_decode(b64_metadata,
> BPTR(&metadata),
> +int decoded_len = openvpn_base64_decode(b64_metadata,
> BEND(&metadata),
> BCAP(&metadata));
> if (decoded_len < 0)
> {
>
Good catch. And apologies for the silly bug. Patch looks good, but it
would have been nice to add a regression (unit) test. Are you willing to
write one? Otherwise I will.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix OpenSSL private key passphrase notices
Hi,
On 21-10-2019 13:35, Santtu Lakkala wrote:
> Clear error stack on successful certificate loading in
> tls_ctx_load_cert_file_and_copy() and handle errors also for
> PEM_read_bio_PrivateKey() call in tls_ctx_load_priv_file().
>
> Due to certificate loading possibly leaking non-fatal errors on OpenSSL
> error stack, and some slight oversights in error handling, the
>
>> PASSWORD:Verification Failed: 'Private Key'
>
> line was never produced on the management channel for PEM formatted keys.
>
> Signed-off-by: Santtu Lakkala
> ---
> src/openvpn/ssl_openssl.c | 11 +--
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 07916c3c..74c8fa65 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -921,6 +921,10 @@ end:
> crypto_msg(M_FATAL, "Cannot load certificate file %s",
> cert_file);
> }
> }
> +else
> +{
> +crypto_print_openssl_errors(M_DEBUG);
> +}
>
> if (in != NULL)
> {
> @@ -963,12 +967,7 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const
> char *priv_key_file,
> pkey = PEM_read_bio_PrivateKey(in, NULL,
> SSL_CTX_get_default_passwd_cb(ctx->ctx),
>
> SSL_CTX_get_default_passwd_cb_userdata(ctx->ctx));
> -if (!pkey)
> -{
> -goto end;
> -}
> -
> -if (!SSL_CTX_use_PrivateKey(ssl_ctx, pkey))
> +if (!pkey || !SSL_CTX_use_PrivateKey(ssl_ctx, pkey))
> {
> #ifdef ENABLE_MANAGEMENT
> if (management && (ERR_GET_REASON(ERR_peek_error()) ==
> EVP_R_BAD_DECRYPT))
>
Thanks, and apologies for the late repsonse. This patch does was it
says, and looks good to me. I think this one should go into both master
and release/2.4.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Warn about insecure ciphers also in init_key_type
On 19-02-2020 12:21, Arne Schwabe wrote:
> With modern Clients and server initialising the crypto cipher later
> and not when reading in the config, most users never the warning when
> having selected BF-CBC in the configuration.
>
> This patch adds the logic to print out warning to init_key_type.
>
> Main reason for this patch is a personal experience with someone who was
> strictly against putting 'cipher' into a config file because he did not
> like hardcoding a cipher and "OpenVPN will do AES-GCM anyway" and thinks
> that it is better to not have it in configuration even after told by me
> that 15 year defaults might not be good anymore.
>
> Patch V2: rebase on master, fix minor style issues
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto.c | 26 ++
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 65e789ed..453cb20a 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -736,6 +736,17 @@ crypto_max_overhead(void)
> +max_int(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
> }
>
> +static void warn_insecure_key_type(const char* ciphername, const cipher_kt_t
> *cipher)
> +{
> +if (cipher_kt_insecure(cipher))
> +{
> +msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than
> 128"
> +" bit (%d bit). This allows attacks like SWEET32.
> Mitigate by "
> +"using a --cipher with a larger block size (e.g.
> AES-256-CBC).",
> +ciphername, cipher_kt_block_size(cipher)*8);
> +}
> +}
> +
> /*
> * Build a struct key_type.
> */
> @@ -779,6 +790,10 @@ init_key_type(struct key_type *kt, const char
> *ciphername,
> {
> msg(M_FATAL, "Cipher '%s' not allowed: block size too big.",
> ciphername);
> }
> +if (warn)
> +{
> +warn_insecure_key_type(ciphername, kt->cipher);
> +}
> }
> else
> {
> @@ -831,9 +846,10 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
> cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
> kt->cipher, enc);
>
> +const char* ciphername =
> translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher));
> msg(D_HANDSHAKE, "%s: Cipher '%s' initialized with %d bit key",
> prefix,
> -translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher)),
> +ciphername,
> kt->cipher_length *8);
>
> dmsg(D_SHOW_KEYS, "%s: CIPHER KEY: %s", prefix,
> @@ -841,13 +857,7 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
> dmsg(D_CRYPTO_DEBUG, "%s: CIPHER block_size=%d iv_size=%d",
> prefix, cipher_kt_block_size(kt->cipher),
> cipher_kt_iv_size(kt->cipher));
> -if (cipher_kt_insecure(kt->cipher))
> -{
> -msg(M_WARN, "WARNING: INSECURE cipher with block size less than
> 128"
> - " bit (%d bit). This allows attacks like SWEET32. Mitigate
> by "
> -"using a --cipher with a larger block size (e.g.
> AES-256-CBC).",
> -cipher_kt_block_size(kt->cipher)*8);
> -}
> +warn_insecure_key_type(ciphername, kt->cipher);
> }
> if (kt->digest && kt->hmac_length > 0)
> {
>
Tested, works as advertised.
Acked-by: Steffan Karger
-Steffan
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Warn about insecure ciphers also in init_key_type
Hi,
On 29-03-2019 13:27, Arne Schwabe wrote:
> With modern Clients and server initialising the crypto cipher later
> and not when reading in the config, most users never the warning when
> having selected BF-CBC in the configuration.
>
> This patch adds the logic to print out warning to init_key_type.
>
> Main reason for this patch is a personal experience with someone who was
> strictly against putting 'cipher' into a config file because he did not
> like hardcoding a cipher and "OpenVPN will do AES-GCM anyway" and thinks
> that it is better to not have it in configuration even after told by me
> that 15 year defaults might not be good anymore.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto.c | 27 +++
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index ff9dbfdc..8a92a8c1 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -736,6 +736,17 @@ crypto_max_overhead(void)
> +max_int(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
> }
>
> +static void warn_insecure_key_type(const char* ciphername, const cipher_kt_t
> *cipher)
> +{
> +if (cipher_kt_insecure(cipher))
> +{
> +msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than
> 128"
> +" bit (%d bit). This allows attacks like SWEET32.
> Mitigate by "
> +"using a --cipher with a larger block size (e.g.
> AES-256-CBC).",
> +ciphername, cipher_kt_block_size(cipher)*8);
> +}
> +}
> +
> /*
> * Build a struct key_type.
> */
> @@ -763,6 +774,7 @@ init_key_type(struct key_type *kt, const char *ciphername,
> kt->cipher_length = keysize;
> }
>
> +
Spurious newline, should be removed from patch.
> /* check legal cipher mode */
> aead_cipher = cipher_kt_mode_aead(kt->cipher);
> if (!(cipher_kt_mode_cbc(kt->cipher)
> @@ -779,6 +791,10 @@ init_key_type(struct key_type *kt, const char
> *ciphername,
> {
> msg(M_FATAL, "Cipher '%s' not allowed: block size too big.",
> ciphername);
> }
> +if(warn)
Space after if missing: if (warn)
> +{
> +warn_insecure_key_type(ciphername, kt->cipher);
> +}
> }
> else
> {
> @@ -831,9 +847,10 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
> cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
> kt->cipher, enc);
>
> +const char* ciphername =
> translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher));
> msg(D_HANDSHAKE, "%s: Cipher '%s' initialized with %d bit key",
> prefix,
> -translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher)),
> +ciphername,
> kt->cipher_length *8);
>
> dmsg(D_SHOW_KEYS, "%s: CIPHER KEY: %s", prefix,
> @@ -841,13 +858,7 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
> dmsg(D_CRYPTO_DEBUG, "%s: CIPHER block_size=%d iv_size=%d",
> prefix, cipher_kt_block_size(kt->cipher),
> cipher_kt_iv_size(kt->cipher));
> -if (cipher_kt_insecure(kt->cipher))
> -{
> -msg(M_WARN, "WARNING: INSECURE cipher with block size less than
> 128"
> -" bit (%d bit). This allows attacks like SWEET32. Mitigate
> by "
> -"using a --cipher with a larger block size (e.g.
> AES-256-CBC).",
> -cipher_kt_block_size(kt->cipher)*8);
> -}
> +warn_insecure_key_type(ciphername, kt->cipher);
> }
> if (kt->digest && kt->hmac_length > 0)
> {
>
Otherwise this looks good. Maybe send a rebased version with the above
nits resolved?
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] travis-ci: add arm64, s390x builds.
On 03-02-2020 09:04, Илья Шипицин wrote: > also, ARM64 builds are flaky. maybe we should add them as allow_failures. What is flaky about the ARM64 builds? Is it our build? Is it the travis infra? -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] linux arm64 tests fail
Hi, On 01-02-2020 13:43, Илья Шипицин wrote: > https://travis-ci.org/chipitsine/openvpn/jobs/644745481?utm_medium=notification&utm_source=github_status > > it indicates "ERROR" when running tests, however tests are ok after all. > > [ RUN ] tls_crypt_v2_wrap_too_long_metadata > > ERROR: could not crypt: insufficient space in dst > > [ OK ] tls_crypt_v2_wrap_too_long_metadata > > [ RUN ] tls_crypt_v2_wrap_unwrap_wrong_key > > [ OK ] tls_crypt_v2_wrap_unwrap_wrong_key > > [ RUN ] tls_crypt_v2_wrap_unwrap_dst_too_small > > [ OK ] tls_crypt_v2_wrap_unwrap_dst_too_small > > [ RUN ] test_tls_crypt_v2_write_server_key_file > > [ OK ] test_tls_crypt_v2_write_server_key_file > > [ RUN ] test_tls_crypt_v2_write_client_key_file > > [ OK ] test_tls_crypt_v2_write_client_key_file > > [==] 14 test(s) run. > > [ PASSED ] 14 test(s). > > PASS: tls_crypt_testdriver This is expected behaviour. And unrelated to arm64. Just check the amd64 build, exact same message. We test the "too much input data" case here, which prints an error. Looks odd, I admit. But printing warnings/errors even in tests helps speed up diagnosis. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Move keying material exporter check from syshead.h to configure.ac
On 20-01-2020 13:42, Lev Stipakov wrote: > + have_export_keying_material="yes" > + AC_CHECK_FUNCS( > + [SSL_export_keying_material], > + , > + [have_export_keying_material="no"; break] > + ) > > > Just wondering why not AC_CHECK_FUNC? In this case we could get rid of > "break". > > _FUNCS also defines HAVE_function macro, which we don't use in this > particular case. > > Noticed that we have similar code above: > > have_crypto_aead_modes="yes" > AC_CHECK_FUNCS( > [EVP_aes_256_gcm], > , > [have_crypto_aead_modes="no"; break] > ) > > so maybe that's why. This. It's too long ago that I fully remember, but I recall we changed to FUNCS for some reason in the past. Autoconf is full of surprises. So if we have something that works, and I want something similar, let's do it in the exact same way. -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Move keying material exporter check from syshead.h to configure.ac
Commit ab27c9f7 added a compile-time check for availablitity of
keying-material-export functionality to syshead.h. It turns out that
openvpnserv also includes syshead.h, and has ENABLE_CRYPTO_* defined in
it's config.h, but doesn't have the necessary CFLAGS / LIBS to actually
compile and link against the crypto libraries. That of course breaks
openvpnserv builds.
To fix this, change the compile-time check in syshead.h into a
configure-time check in configure.ac. That's more consistent with how we
do other feature checks anyway.
Signed-off-by: Steffan Karger
---
configure.ac | 20
src/openvpn/init.c| 4 ++--
src/openvpn/options.c | 4 ++--
src/openvpn/options.h | 2 +-
src/openvpn/ssl_mbedtls.c | 6 +++---
src/openvpn/syshead.h | 13 -
6 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/configure.ac b/configure.ac
index a47e0a06..98fd39ce 100644
--- a/configure.ac
+++ b/configure.ac
@@ -912,6 +912,13 @@ if test "${with_crypto_library}" = "openssl"; then
[have_crypto_aead_modes="no"; break]
)
+ have_export_keying_material="yes"
+ AC_CHECK_FUNCS(
+ [SSL_export_keying_material],
+ ,
+ [have_export_keying_material="no"; break]
+ )
+
AC_CHECK_FUNCS(
[ \
HMAC_CTX_new \
@@ -1010,6 +1017,13 @@ elif test "${with_crypto_library}" = "mbedtls"; then
[have_crypto_aead_modes="no"; break]
)
+ have_export_keying_material="yes"
+ AC_CHECK_FUNCS(
+ [mbedtls_ssl_conf_export_keys_ext_cb],
+ ,
+ [have_export_keying_material="no"; break]
+ )
+
CFLAGS="${saved_CFLAGS}"
LIBS="${saved_LIBS}"
AC_DEFINE([ENABLE_CRYPTO_MBEDTLS], [1], [Use mbed TLS library])
@@ -1217,6 +1231,12 @@ test "${enable_strict_options}" = "yes" &&
AC_DEFINE([ENABLE_STRICT_OPTIONS_CHEC
test "${enable_crypto_ofb_cfb}" = "yes" && AC_DEFINE([ENABLE_OFB_CFB_MODE],
[1], [Enable OFB and CFB cipher modes])
test "${have_crypto_aead_modes}" = "yes" &&
AC_DEFINE([HAVE_AEAD_CIPHER_MODES], [1], [Use crypto library])
+if test "${have_export_keying_material}" = "yes"; then
+ AC_DEFINE(
+ [HAVE_EXPORT_KEYING_MATERIAL], [1],
+ [Crypto library supports keying material exporter]
+ )
+fi
OPTIONAL_CRYPTO_CFLAGS="${OPTIONAL_CRYPTO_CFLAGS} ${CRYPTO_CFLAGS}"
OPTIONAL_CRYPTO_LIBS="${OPTIONAL_CRYPTO_LIBS} ${CRYPTO_LIBS}"
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index ce417df0..04207b61 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2931,7 +2931,7 @@ do_init_crypto_tls(struct context *c, const unsigned int
flags)
to.comp_options = options->comp;
#endif
-#ifdef HAVE_EKM
+#ifdef HAVE_EXPORT_KEYING_MATERIAL
if (options->keying_material_exporter_label)
{
to.ekm_size = options->keying_material_exporter_length;
@@ -2947,7 +2947,7 @@ do_init_crypto_tls(struct context *c, const unsigned int
flags)
{
to.ekm_size = 0;
}
-#endif /* HAVE_EKM */
+#endif /* HAVE_EXPORT_KEYING_MATERIAL */
/* TLS handshake authentication (--tls-auth) */
if (options->ce.tls_auth_file)
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 173a1eea..c459b260 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -662,7 +662,7 @@ static const char usage_message[] =
" an explicit nsCertType designation t = 'client' |
'server'.\n"
"--x509-track x : Save peer X509 attribute x in environment for use by\n"
" plugins and management interface.\n"
-#ifdef HAVE_EKM
+#ifdef HAVE_EXPORT_KEYING_MATERIAL
"--keying-material-exporter label len : Save Exported Keying Material
(RFC5705)\n"
" of len bytes (min. 16 bytes) using label in environment
for use by plugins.\n"
#endif
@@ -8506,7 +8506,7 @@ add_option(struct options *options,
options->use_peer_id = true;
options->peer_id = atoi(p[1]);
}
-#ifdef HAVE_EKM
+#ifdef HAVE_EXPORT_KEYING_MATERIAL
else if (streq(p[0], "keying-material-exporter") && p[1] && p[2])
{
int ekm_length = positive_atoi(p[2]);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 3c6b1965..2f1f6faf 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -640,7 +640,7 @@ struct options
bool use_peer_id;
uint32_t peer_id;
-#ifdef HAVE_EKM
+#ifdef HAVE_EXPORT_KEYING_MATERIAL
/* Keying Material Exporters [RF
Re: [Openvpn-devel] [PATCH 1/4] Only announce IV_NCP=2 when we are willing to support these ciphers
Hi,
Finally found some time to start looking at this patch set.
On 17-11-2019 19:12, Arne Schwabe wrote:
> We currently always announce IV_NCP=2 when we support these ciphers even
> when we do not accept them. This lead to a server pushing a AES-GCM-128
> cipher to clients and the client then rejecting it.
>
> Signed-off-by: Arne Schwabe
> ---
> doc/openvpn.8| 2 ++
> src/openvpn/init.c | 4
> src/openvpn/openvpn.h| 1 +
> src/openvpn/ssl.c| 4 +++-
> src/openvpn/ssl_common.h | 1 +
> 5 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index 457c2667..ae24b6c0 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -4392,6 +4392,8 @@ NCP server (v2.4+) with "\-\-cipher BF\-CBC" and
> "\-\-ncp\-ciphers
> AES\-256\-GCM:AES\-256\-CBC" set can either specify "\-\-cipher BF\-CBC" or
> "\-\-cipher AES\-256\-CBC" and both will work.
>
> +Note, for using NCP with a OpenVPN 2.4 server this list must include
> +the AES\-256\-GCM and AES\-128\-GCM ciphers.
> .\"*
> .TP
> .B \-\-ncp\-disable
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 0bdb0a9c..8f142311 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -623,6 +623,7 @@ save_ncp_options(struct context *c)
> c->c1.ciphername = c->options.ciphername;
> c->c1.authname = c->options.authname;
> c->c1.keysize = c->options.keysize;
> +c->c1.ncp_ciphers = c->options.ncp_ciphers;
> }
>
> /* Restores NCP-negotiable options to original values */
> @@ -632,6 +633,7 @@ restore_ncp_options(struct context *c)
> c->options.ciphername = c->c1.ciphername;
> c->options.authname = c->c1.authname;
> c->options.keysize = c->c1.keysize;
> +c->options.ncp_ciphers = c->c1.ncp_ciphers;
> }
options->ncp_ciphers itself is not negotiable, and thus does not need
restoring, I would think.
> void
> @@ -2827,6 +2829,7 @@ do_init_crypto_tls(struct context *c, const unsigned
> int flags)
> to.tcp_mode = link_socket_proto_connection_oriented(options->ce.proto);
> to.config_ciphername = c->c1.ciphername;
> to.config_authname = c->c1.authname;
> +to.config_ncp_ciphers = c->c1.ncp_ciphers;
> to.ncp_enabled = options->ncp_enabled;
> to.transition_window = options->transition_window;
> to.handshake_window = options->handshake_window;
> @@ -4532,6 +4535,7 @@ inherit_context_child(struct context *dest,
> dest->c1.ciphername = src->c1.ciphername;
> dest->c1.authname = src->c1.authname;
> dest->c1.keysize = src->c1.keysize;
> +dest->c1.ncp_ciphers = src->c1.ncp_ciphers;
>
> /* inherit auth-token */
> dest->c1.ks.auth_token_key = src->c1.ks.auth_token_key;
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index 900db7e1..9e46ad6c 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -209,6 +209,7 @@ struct context_1
>
> const char *ciphername; /**< Data channel cipher from config file */
> const char *authname; /**< Data channel auth from config file */
> +const char *ncp_ciphers;/**< NCP Ciphers */
> int keysize;/**< Data channel keysize from config file */
> #endif
> };
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 4455ebb8..6ca5c79a 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2322,7 +2322,9 @@ push_peer_info(struct buffer *buf, struct tls_session
> *session)
>
> /* support for Negotiable Crypto Parameters */
> if (session->opt->ncp_enabled
> -&& (session->opt->mode == MODE_SERVER || session->opt->pull))
> +&& (session->opt->mode == MODE_SERVER || session->opt->pull)
> +&& tls_item_in_cipher_list("AES-128-GCM",
> session->opt->config_ncp_ciphers)
> +&& tls_item_in_cipher_list("AES-256-GCM",
> session->opt->config_ncp_ciphers))
> {
> buf_printf(&out, "IV_NCP=2\n");
> }
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 8dd08862..fb82f610 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -290,6 +290,7 @@ struct tls_options
>
> const char *config_ciphername;
> const char *config_authname;
> +const char *config_ncp_ciphers;
> bool ncp_enabled;
>
> bool tls_crypt_v2;
>
Otherwise this makes sense and looks good.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi Lev,
Thanks for the update. Even with the latest added fixes and sanity
checks, the resulting code is now 30 lines shorter *and* more readable.
Some last questions / comments inline:
On 13-12-2019 20:19, Lev Stipakov wrote:
> @@ -2099,6 +2115,13 @@ io_wait_dowork(struct context *c, const unsigned int
> flags)
> tuntap |= EVENT_READ;
> }
>
> +#ifdef _WIN32
> +if (tuntap_is_wintun(c->c1.tuntap))
> +{
> +tuntap = EVENT_READ;
> +}
> +#endif
Now that the code is refactored, it is clear that for the tuntap case
the code will ignore any of the earlier set flags. Can you maybe add a
comment explaining why the other flags are being ignored?
> +/* there is a data in wintun ring buffer, read it immediately */
Grammar nit: "there is data" (remove 'a').
Otherwise, as far as stare-at-code goes, I'm fine with this patch now. I
haven't tested it, nor have I reviewed Windows-specific interaction. So
this needs at least one other review from someone able to review those
parts.
Regarding the integration into OpenVPN:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] fix clang warning about missing braces
On 28-11-2019 09:06, Lev Stipakov wrote:
> A struct with subobjects should be initialized
> with double braces.
This is not true. {0} is a valid initializer for structs in C. Both
clang and gcc used to have a bug where they incorrectly warned about
this. GCC fixed this a while ago[0]. I thought clang had fixed it too
recently, but apparently not?
-Steffan
[0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] mbedtls: add RFC 5705 keying material exporter support
Since mbed TLS 2.18, mbed TLS can also implement RFC 5705. As a first
step towards using the keying material exporter as a method to generate
key material for the data channel, implement the
--keying-material-exporter function we already have for OpenSSL also for
mbed TLS builds.
Implementing RFC 5705 for mbed TLS is a bit more cumbersome, because the
library itself only provides a callback that is called during connection
setup, which enables us to implement RFC 5705 ourselves. To protect
ourselves against mistakes, we immediately perform the required key
derivation to generate the exporterd keying material, and only cache the
derived key material until we can actually export it to the environment
(similar to the OpenSSL builds).
To test this, I found it easiest to temporarily move the call to
key_state_export_keying_material outside the if statement, and use a
script that runs after connection setup (e.g. --ipchange) that prints
the environment. E.g.
#!/bin/sh
env | sort
This should show the same value for the exported_keying_material env
variable for both mbed TLS and OpenSSL builds. Of course you can also
use the code as-is, and write a plugin to verify the same thing.
Signed-off-by: Steffan Karger
---
src/openvpn/init.c| 4 +--
src/openvpn/options.c | 4 +--
src/openvpn/options.h | 2 +-
src/openvpn/ssl_mbedtls.c | 64 ++-
src/openvpn/ssl_mbedtls.h | 5 +++
src/openvpn/ssl_openssl.c | 4 ++-
src/openvpn/syshead.h | 14 -
7 files changed, 89 insertions(+), 8 deletions(-)
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 0bdb0a9c..1ed6ae11 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2931,7 +2931,7 @@ do_init_crypto_tls(struct context *c, const unsigned int
flags)
to.comp_options = options->comp;
#endif
-#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000
+#ifdef HAVE_EKM
if (options->keying_material_exporter_label)
{
to.ekm_size = options->keying_material_exporter_length;
@@ -2947,7 +2947,7 @@ do_init_crypto_tls(struct context *c, const unsigned int
flags)
{
to.ekm_size = 0;
}
-#endif
+#endif /* HAVE_EKM */
/* TLS handshake authentication (--tls-auth) */
if (options->ce.tls_auth_file)
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c282b582..98d94b86 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -661,7 +661,7 @@ static const char usage_message[] =
" an explicit nsCertType designation t = 'client' |
'server'.\n"
"--x509-track x : Save peer X509 attribute x in environment for use by\n"
" plugins and management interface.\n"
-#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000
+#ifdef HAVE_EKM
"--keying-material-exporter label len : Save Exported Keying Material
(RFC5705)\n"
" of len bytes (min. 16 bytes) using label in environment
for use by plugins.\n"
#endif
@@ -8468,7 +8468,7 @@ add_option(struct options *options,
options->use_peer_id = true;
options->peer_id = atoi(p[1]);
}
-#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000
+#ifdef HAVE_EKM
else if (streq(p[0], "keying-material-exporter") && p[1] && p[2])
{
int ekm_length = positive_atoi(p[2]);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 7fd2c00f..7f3b3b20 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -640,7 +640,7 @@ struct options
bool use_peer_id;
uint32_t peer_id;
-#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000
+#ifdef HAVE_EKM
/* Keying Material Exporters [RFC 5705] */
const char *keying_material_exporter_label;
int keying_material_exporter_length;
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index a4197cba..54f692e0 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -190,12 +190,62 @@ tls_ctx_initialised(struct tls_root_ctx *ctx)
return ctx->initialised;
}
+#ifdef HAVE_EKM
+int mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
+ const unsigned char *kb, size_t maclen,
+ size_t keylen, size_t ivlen,
+ const unsigned char client_random[32],
+ const unsigned char server_random[32],
+ mbedtls_tls_prf_types tls_prf_type)
+{
+struct tls_session *session = p_expkey;
+struct key_state_ssl *ks_ssl = &session->key[KS_PRIMARY].ks_ssl;
+unsigned char client_server_random[64];
+
+ks_ssl->exported_key_material = gc_malloc(session->opt->ekm_size,
+
[Openvpn-devel] [PATCH] Update sample configs to use modern cipher, remove static key examples
Since these are examples, people might use them as a basis for their own configs. In the non-push/pull configs, we should specify a decent cipher. Further, I don't think we should recommend anyone to still use a static key configuration, so remove the static key config examples. Signed-off-by: Steffan Karger --- sample/sample-config-files/loopback-client| 1 + sample/sample-config-files/loopback-server| 1 + sample/sample-config-files/static-home.conf | 75 --- sample/sample-config-files/static-office.conf | 72 -- sample/sample-config-files/tls-home.conf | 12 +-- sample/sample-config-files/tls-office.conf| 3 + 6 files changed, 11 insertions(+), 153 deletions(-) delete mode 100644 sample/sample-config-files/static-home.conf delete mode 100644 sample/sample-config-files/static-office.conf diff --git a/sample/sample-config-files/loopback-client b/sample/sample-config-files/loopback-client index 7117307d..1734aa8b 100644 --- a/sample/sample-config-files/loopback-client +++ b/sample/sample-config-files/loopback-client @@ -22,5 +22,6 @@ ca sample-keys/ca.crt key sample-keys/client.key cert sample-keys/client.crt tls-auth sample-keys/ta.key 1 +cipher AES-256-GCM ping 1 inactive 120 1000 diff --git a/sample/sample-config-files/loopback-server b/sample/sample-config-files/loopback-server index 8e1f39cd..58daeb56 100644 --- a/sample/sample-config-files/loopback-server +++ b/sample/sample-config-files/loopback-server @@ -22,5 +22,6 @@ ca sample-keys/ca.crt key sample-keys/server.key cert sample-keys/server.crt tls-auth sample-keys/ta.key 0 +cipher AES-256-GCM ping 1 inactive 120 1000 diff --git a/sample/sample-config-files/static-home.conf b/sample/sample-config-files/static-home.conf deleted file mode 100644 index ed0c6726.. --- a/sample/sample-config-files/static-home.conf +++ /dev/null @@ -1,75 +0,0 @@ -# -# Sample OpenVPN configuration file for -# home using a pre-shared static key. -# -# '#' or ';' may be used to delimit comments. - -# Use a dynamic tun device. -# For Linux 2.2 or non-Linux OSes, -# you may want to use an explicit -# unit number such as "tun1". -# OpenVPN also supports virtual -# ethernet "tap" devices. -dev tun - -# Our OpenVPN peer is the office gateway. -remote 1.2.3.4 - -# 10.1.0.2 is our local VPN endpoint (home). -# 10.1.0.1 is our remote VPN endpoint (office). -ifconfig 10.1.0.2 10.1.0.1 - -# Our up script will establish routes -# once the VPN is alive. -up ./home.up - -# Our pre-shared static key -secret static.key - -# Cipher to use -cipher AES-256-CBC - -# OpenVPN 2.0 uses UDP port 1194 by default -# (official port assignment by iana.org 11/04). -# OpenVPN 1.x uses UDP port 5000 by default. -# Each OpenVPN tunnel must use -# a different port number. -# lport or rport can be used -# to denote different ports -# for local and remote. -; port 1194 - -# Downgrade UID and GID to -# "nobody" after initialization -# for extra security. -; user nobody -; group nobody - -# If you built OpenVPN with -# LZO compression, uncomment -# out the following line. -; comp-lzo - -# Send a UDP ping to remote once -# every 15 seconds to keep -# stateful firewall connection -# alive. Uncomment this -# out if you are using a stateful -# firewall. -; ping 15 - -# Uncomment this section for a more reliable detection when a system -# loses its connection. For example, dial-ups or laptops that -# travel to other locations. -; ping 15 -; ping-restart 45 -; ping-timer-rem -; persist-tun -; persist-key - -# Verbosity level. -# 0 -- quiet except for fatal errors. -# 1 -- mostly quiet, but display non-fatal network errors. -# 3 -- medium output, good for normal operation. -# 9 -- verbose, good for troubleshooting -verb 3 diff --git a/sample/sample-config-files/static-office.conf b/sample/sample-config-files/static-office.conf deleted file mode 100644 index 609ddd02.. --- a/sample/sample-config-files/static-office.conf +++ /dev/null @@ -1,72 +0,0 @@ -# -# Sample OpenVPN configuration file for -# office using a pre-shared static key. -# -# '#' or ';' may be used to delimit comments. - -# Use a dynamic tun device. -# For Linux 2.2 or non-Linux OSes, -# you may want to use an explicit -# unit number such as "tun1". -# OpenVPN also supports virtual -# ethernet "tap" devices. -dev tun - -# 10.1.0.1 is our local VPN endpoint (office). -# 10.1.0.2 is our remote VPN endpoint (home). -ifconfig 10.1.0.1 10.1.0.2 - -# Our up script will establish routes -# once the VPN is alive. -up ./office.up - -# Our pre-shared static key -secret static.key - -# Cipher to use -cipher AES-256-CBC - -# OpenVPN 2.0 uses UDP port 1194 by default -# (official port assignment by iana.org 11/04). -# OpenVPN 1.x uses UDP port 5000 by default. -# Each OpenVPN tunnel must use -# a different port number. -# lport or rport can be used -# to denote differ
Re: [Openvpn-devel] [PATCH v3] Make compression asymmetric by default and add warnings
Hi,
Feature-ack, and overall looks good. But some nits to tackle.
On 24-10-2018 12:06, Arne Schwabe wrote:
> This commit introduces the allow-compression option that allow
> changing the new default to the previous default or to a stricter
> version.
>
> Warning are not generated in the post option check
> (options_postprocess_mutate) since these warnings should also be shown
> on pushed options.
>
> Patch V2: fix spelling and grammer (thanks tincantech), also fix
>uncompressiable to incompressible in three other instances in the
>source code
>
> Patch V3: fix overlong lines. Do not allow compression to be pushed
> ---
> doc/openvpn.8 | 44 ++
> src/openvpn/comp-lz4.c | 3 +-
> src/openvpn/comp.c | 2 +-
> src/openvpn/comp.h | 16 +--
> src/openvpn/lzo.c | 2 +-
> src/openvpn/mtu.h | 2 +-
> src/openvpn/options.c | 101 -
> 7 files changed, 143 insertions(+), 27 deletions(-)
>
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index d40dcf43..ac923f51 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -2545,26 +2545,54 @@ Enable a compression algorithm.
>
> The
> .B algorithm
> -parameter may be "lzo", "lz4", or empty. LZO and LZ4
> -are different compression algorithms, with LZ4 generally
> +parameter may be "lzo", "lz4", "lz4\-v2", "stub", "stub\-v2" or empty.
> +LZO and LZ4 are different compression algorithms, with LZ4 generally
> offering the best performance with least CPU usage.
> For backwards compatibility with OpenVPN versions before v2.4, use "lzo"
> (which is identical to the older option "\-\-comp\-lzo yes").
>
> +The "lz4\-v2" and "stub\-v2" variants implement a better framing that does
> not add
> +overhead when packets cannot be compressed. All other variants always add
> one extra
> +framing byte compared to no compression framing.
> +
> If the
> .B algorithm
> -parameter is empty, compression will be turned off, but the packet
> -framing for compression will still be enabled, allowing a different
> -setting to be pushed later.
> +parameter is "stub", "stub\-v2", or empty, compression will be turned off,
> but
> +the packet framing for compression will still be enabled, allowing a
> different
> +setting to be pushed later. Additionally, "stub" and "stub\-v2" will disable
> +announcing lzo and lz4 compression support via "IV_" variables to the server.
> +
>
> .B Security Considerations
>
> Compression and encryption is a tricky combination. If an attacker knows or
> is
> able to control (parts of) the plaintext of packets that contain secrets, the
> attacker might be able to extract the secret if compression is enabled. See
> -e.g. the CRIME and BREACH attacks on TLS which also leverage compression to
> -break encryption. If you are not entirely sure that the above does not apply
> -to your traffic, you are advised to *not* enable compression.
> +e.g. the CRIME and BREACH attacks on TLS and VORACLE on VPNs which also
> leverage
> +compression to break encryption. If you are not entirely sure that the
> above does
> +not apply to your traffic, you are advised to *not* enable compression.
> +
> +.\"*
> +.TP
> +.B \-\-allow\-compression [mode]
> +As described in
> +\.B \-\-compress
> +option, compression is potentially dangerous option. This option allows
> +controlling the behaviour of OpenVPN when compression is used and allowed.
> +.B mode
> +may be "yes", "no", or "asym" (default).
> +
> +With allow\-compression set to "no", OpenVPN will refuse any non stub
> +compression. With "yes" OpenVPN will send and receive compressed packets.
> +With "asym", the default, 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.
> +
> +The default of asym has been chosen to maximise compatibility with existing
> +configuration while at the same time phasing out compression in existing
> +deployment by disabling compression on the uplink, effectively completely
> disabling
> +compression if both client and server are upgraded.
>
> .\"*
> .TP
> diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
> index f52fdbfb..fa23e5a5 100644
> --- a/src/openvpn/comp-lz4.c
> +++ b/src/openvpn/comp-lz4.c
> @@ -70,8 +70,9 @@ do_lz4_compress(struct buffer *buf,
> {
> /*
> * In order to attempt compression, length must be at least
> COMPRESS_THRESHOLD.
> + * and asymmetric compression must be disabled
> */
> -if (buf->len >= COMPRESS_THRESHOLD)
> +if (buf->len >= COMPRESS_THRESHOLD && (compctx->flags & COMP_F_NO_ASYM))
> {
> const size_t ps = PAYLOAD_SIZE(frame);
> int zlen_max = ps + COMP_EXTRA_BU
Re: [Openvpn-devel] [PATCH v6 1/2] Make tls_version_max return the actual maximum version
Hi,
On 09-11-2019 13:03, Arne Schwabe wrote:
> Before OpenSSL 1.1.1 there could be no mismatch between
> compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
> runtime detection to detect the actual best TLS version supported.
>
> Allowing this runtime detection also allows removing some of the
> TLS 1.3/OpenSSL 1.1.1 #ifdefs
>
> Without this patch tls-min-version 1.3 or-highest will actually
> downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against
> 1.1.1" scenario.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/ssl.c | 11 +--
> src/openvpn/ssl_openssl.c | 31 ---
> 2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 4455ebb8..e708fc93 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -4194,12 +4194,11 @@ show_available_tls_ciphers(const char *cipher_list,
> {
> printf("Available TLS Ciphers, listed in order of preference:\n");
>
> -#if (ENABLE_CRYPTO_OPENSSL && OPENSSL_VERSION_NUMBER >= 0x1010100fL)
> -printf("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n");
> -show_available_tls_ciphers_list(cipher_list_tls13, tls_cert_profile,
> true);
> -#else
> -(void) cipher_list_tls13; /* Avoid unused warning */
> -#endif
> +if (tls_version_max() >= TLS_VER_1_3)
> +{
> +printf("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n");
> +show_available_tls_ciphers_list(cipher_list_tls13, tls_cert_profile,
> true);
> +}
>
> printf("\nFor TLS 1.2 and older (--tls-cipher):\n\n");
> show_available_tls_ciphers_list(cipher_list, tls_cert_profile, false);
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 07916c3c..e07d6e74 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -215,7 +215,23 @@ int
> tls_version_max(void)
> {
> #if defined(TLS1_3_VERSION)
> +/* If this is defined we can safely assume TLS 1.3 support */
> return TLS_VER_1_3;
This clause is no longer needed, right?
> +#elif OPENSSL_VERSION_NUMBER >= 0x1010L
> +/*
> + * The library we are *linked* against is OpenSSL 1.1.1
s/is/might be/ ?
> + * and therefore supports TLS 1.3. This needs to be checked at runtime
> + * since we can be compiled against 1.1.0 and then the library can be
> + * upgraded to 1.1.1
> + */
> +if (OpenSSL_version_num() >= 0x1010100fL)
> +{
> +return TLS_VER_1_3;
> +}
> +else
> +{
> +return TLS_VER_1_2;
> +}
> #elif defined(TLS1_2_VERSION) || defined(SSL_OP_NO_TLSv1_2)
> return TLS_VER_1_2;
> #elif defined(TLS1_1_VERSION) || defined(SSL_OP_NO_TLSv1_1)
> @@ -241,12 +257,20 @@ openssl_tls_version(int ver)
> {
> return TLS1_2_VERSION;
> }
> -#if defined(TLS1_3_VERSION)
> else if (ver == TLS_VER_1_3)
> {
> +/*
> + * Supporting the library upgraded to TLS1.3 without recompile
> + * is enough to support here with a simple constant that the same
> + * as in the TLS 1.3, so spec it is very unlikely that OpenSSL
> + * will change this constant
> + */
> +#ifndef TLS1_3_VERSION
> +return 0x0304;
> +#else
Why not do this outside the function as
#ifndef TLS1_3_VERSION
#define TLS1_3_VERSION 0x0304
#endif
> return TLS1_3_VERSION;
> -}
> #endif
> +}
> return 0;
> }
>
> @@ -2015,7 +2039,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
> #if defined(TLS1_3_VERSION)
> if (tls13)
> {
> -SSL_CTX_set_min_proto_version(tls_ctx.ctx, TLS1_3_VERSION);
> +SSL_CTX_set_min_proto_version(tls_ctx.ctx,
> + openssl_tls_version(TLS_VER_1_3));
> tls_ctx_restrict_ciphers_tls13(&tls_ctx, cipher_list);
> }
> else
>
Otherwise looks good.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 4/7] wintun: ring buffers based I/O
Hi,
Some first-round review comments. I still need to fully grasp the event
mechanism intricacies for a real in-depth review.
As a general remark: could you try to stick to the 80 char line length
limit?
On 07-11-2019 18:45, Lev Stipakov wrote:
> From: Lev Stipakov
>
> Implemented according to Wintun documentation
> and reference client code.
>
> Wintun uses ring buffers to communicate between
> kernel driver and user process. Client allocates
> send and receive ring buffers, creates events
> and passes it to kernel driver under LocalSystem
> privileges.
>
> When data is available for read, wintun modifies
> "tail" pointer of send ring and signals via event.
> User process reads data from "head" to "tail" and
> updates "head" pointer.
>
> When user process is ready to write, it writes
> to receive ring, updates "tail" pointer and signals
> to kernel via event.
>
> In openvpn code we add send ring's event to event loop.
> Before performing io wait, we compare "head" and "tail"
> pointers of send ring and if they're different, we skip
> io wait and perform read.
>
> This also adds ring buffers support to tcp and udp
> server code.
>
> Signed-off-by: Lev Stipakov
> ---
> src/openvpn/forward.c | 42 +++---
> src/openvpn/forward.h | 47 +++-
> src/openvpn/mtcp.c| 28 +++-
> src/openvpn/mudp.c| 14 ++
> src/openvpn/options.c | 4 +-
> src/openvpn/syshead.h | 1 +
> src/openvpn/tun.c | 45 +++
> src/openvpn/tun.h | 121
> +-
> src/openvpn/win32.c | 120 +
> src/openvpn/win32.h | 47
> 10 files changed, 458 insertions(+), 11 deletions(-)
>
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 8451706..0be8b6d 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1256,12 +1256,30 @@ read_incoming_tun(struct context *c)
> perf_push(PERF_READ_IN_TUN);
>
> c->c2.buf = c->c2.buffers->read_tun_buf;
> +
> #ifdef _WIN32
> -read_tun_buffered(c->c1.tuntap, &c->c2.buf);
> +if (c->c1.tuntap->wintun)
> +{
> +read_wintun(c->c1.tuntap, &c->c2.buf);
> +if (c->c2.buf.len == -1)
> +{
> +register_signal(c, SIGHUP, "tun-abort");
> +c->persist.restart_sleep_seconds = 1;
> +msg(M_INFO, "Wintun read error, restarting");
> +perf_pop();
> +return;
> +}
> +}
> +else
> +{
> +read_tun_buffered(c->c1.tuntap, &c->c2.buf);
> #else
> -ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
> -ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
> -c->c2.buf.len = read_tun(c->c1.tuntap, BPTR(&c->c2.buf),
> MAX_RW_SIZE_TUN(&c->c2.frame));
> +ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
> +ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
> +c->c2.buf.len = read_tun(c->c1.tuntap, BPTR(&c->c2.buf),
> MAX_RW_SIZE_TUN(&c->c2.frame));
> +#endif
> +#ifdef _WIN32
> +}
> #endif
As Simon mentioned too, this code is getting more and more hard to read.
Can we maybe do this in some cleaner way? Maybe add some helper function(s)?
>
> #ifdef PACKET_TRUNCATION_CHECK
> @@ -2103,7 +2121,21 @@ io_wait_dowork(struct context *c, const unsigned int
> flags)
> * Configure event wait based on socket, tuntap flags.
> */
> socket_set(c->c2.link_socket, c->c2.event_set, socket, (void
> *)&socket_shift, NULL);
> -tun_set(c->c1.tuntap, c->c2.event_set, tuntap, (void *)&tun_shift, NULL);
> +
> +#ifdef _WIN32
> +if (c->c1.tuntap && c->c1.tuntap->wintun)
> +{
> +/* add ring buffer event */
> +struct rw_handle rw = {.read = c->c1.tuntap->send_tail_moved };
> +event_ctl(c->c2.event_set, &rw, EVENT_READ, (void *)&tun_shift);
> +}
> +else
> +{
> +#endif
> +tun_set(c->c1.tuntap, c->c2.event_set, tuntap, (void *)&tun_shift,
> NULL);
> +#ifdef _WIN32
> +}
> +#endif
As above :)
>
> #ifdef ENABLE_MANAGEMENT
> if (management)
> diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
> index 48202c0..6096fa8 100644
> --- a/src/openvpn/forward.h
> +++ b/src/openvpn/forward.h
> @@ -375,6 +375,19 @@ p2p_iow_flags(const struct context *c)
> {
> flags |= IOW_TO_TUN;
> }
> +#ifdef _WIN32
> +{
> +struct tuntap *tt = c->c1.tuntap;
> +if (tt && tt->wintun)
> +{
> +if (tt->send_ring->head == tt->send_ring->tail)
> +{
> +/* nothing to read from tun -> remove tun read flag set by
> IOW_READ */
> +flags &= ~IOW_READ_TUN;
> +}
> +}
> +}
> +#endif
Looks like this should be a helper function (with a good name that helps
understand what this does).
> return flags;
> }
>
> @@ -403,8 +416,38 @@ io_wait(struct cont
Re: [Openvpn-devel] [PATCH v3] wintun: add --windows-driver config option
gt; /*
> * parse/print topology coding
> */
> @@ -5281,6 +5316,13 @@ add_option(struct options *options,
> VERIFY_PERMISSION(OPT_P_GENERAL);
> options->dev_type = p[1];
> }
> +#ifdef _WIN32
> +else if (streq(p[0], "windows-driver") && p[1] && !p[2])
> +{
> +VERIFY_PERMISSION(OPT_P_GENERAL);
> +options->wintun = parse_windows_driver(p[1], M_FATAL);
> +}
> +#endif
> else if (streq(p[0], "dev-node") && p[1] && !p[2])
> {
> VERIFY_PERMISSION(OPT_P_GENERAL);
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index ff7a5bb..0a24e5e 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -632,6 +632,7 @@ struct options
> bool show_net_up;
> int route_method;
> bool block_outside_dns;
> +bool wintun;
> #endif
>
> bool use_peer_id;
> diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
> index 5a0a933..df935f6 100644
> --- a/src/openvpn/tun.h
> +++ b/src/openvpn/tun.h
> @@ -175,6 +175,7 @@ struct tuntap
> * ~0 if undefined */
> DWORD adapter_index;
>
> +bool wintun; /* true if wintun is used instead of tap-windows6 */
> int standby_iter;
> #else /* ifdef _WIN32 */
> int fd; /* file descriptor for TUN/TAP dev */
>
Thanks, this looks good to me now.
I don't have a working Windows test setup available, so only did
stare-at-code and verified that building with mingw still works for me.
So, as far as this goes without real testing:
Acked-by: Steffan Karger
-Steffan
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 2/7] wintun: add --windows-driver config option
Hi,
Thanks for looking into wintun support. Definitely feature-ack.
On 07-11-2019 18:45, Lev Stipakov wrote:
> From: Lev Stipakov
>
> This allows to specify which tun driver openvpn should use,
> tap-windows6 (default) or wintun.
>
> Note than wintun support will be added in follow-up patches.
>
> Signed-off-by: Lev Stipakov
> ---
> src/openvpn/init.c| 7 +++
> src/openvpn/options.c | 37 +
> src/openvpn/options.h | 1 +
> src/openvpn/tun.h | 1 +
> 4 files changed, 46 insertions(+)
Should there not be a manpage entry in this commit too?
>
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index ae7bd63..c6d4953 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1733,6 +1733,10 @@ do_init_tun(struct context *c)
> c->c2.es,
> &c->net_ctx);
>
> +#ifdef _WIN32
> +c->c1.tuntap->wintun = c->options.wintun;
> +#endif
> +
> init_tun_post(c->c1.tuntap,
>&c->c2.frame,
>&c->options.tuntap_options);
> @@ -1775,6 +1779,9 @@ do_open_tun(struct context *c)
> /* store (hide) interactive service handle in tuntap_options */
> c->c1.tuntap->options.msg_channel = c->options.msg_channel;
> msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int)
> c->options.msg_channel);
> +
> +c->c1.tuntap->wintun = c->options.wintun;
> +
Why do we have to set c->c2.tuntap->wintun in both do_init_tun and
do_open_tun? Should one of them not suffice?
> #endif
>
> /* allocate route list structure */
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 1838a69..5c5033e 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -747,6 +747,9 @@ static const char usage_message[] =
> " optional parameter controls the initial state of
> ex.\n"
> "--show-net-up : Show " PACKAGE_NAME "'s view of routing table and net
> adapter list\n"
> " after TAP adapter is up and routes have been added.\n"
> +"--windows-driver : Which tun driver to use?\n"
> +" tap-windows6 (default)\n"
> +" wintun\n"
> #ifdef _WIN32
Should this --windows-driver option not be inside the #ifdef _WIN32 ?
> "--block-outside-dns : Block DNS on other network adapters to prevent
> DNS leaks\n"
> #endif
> @@ -851,6 +854,7 @@ init_options(struct options *o, const bool init_gc)
> o->tuntap_options.dhcp_masq_offset = 0; /* use network address as
> internal DHCP server address */
> o->route_method = ROUTE_METHOD_ADAPTIVE;
> o->block_outside_dns = false;
> +o->wintun = false;
> #endif
> o->vlan_accept = VLAN_ONLY_UNTAGGED_OR_PRIORITY;
> o->vlan_pvid = 1;
> @@ -2994,6 +2998,12 @@ options_postprocess_mutate_invariant(struct options
> *options)
> options->ifconfig_noexec = false;
> }
>
> +/* for wintun kernel doesn't send DHCP requests, so use ipapi to set IP
> address and netmask */
> +if (options->wintun)
> +{
> +options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;
> +}
> +
> remap_redirect_gateway_flags(options);
> #endif
>
> @@ -4039,6 +4049,26 @@ foreign_option(struct options *o, char *argv[], int
> len, struct env_set *es)
> }
> }
>
> +#ifdef _WIN32
> +bool
> +parse_windows_driver(const char *str, const int msglevel)
I think this should be a static function. Also a short (doxygen) comment
explaining what the return value means would be nice.
> +{
> +if (streq(str, "tap-windows6"))
> +{
> +return false;
> +}
> +else if (streq(str, "wintun"))
> +{
> +return true;
> +}
> +else
> +{
> +msg(msglevel, "--windows-driver must be tap-windows6 or wintun");
> +return false;
> +}
> +}
> +#endif
> +
> /*
> * parse/print topology coding
> */
> @@ -5281,6 +5311,13 @@ add_option(struct options *options,
> VERIFY_PERMISSION(OPT_P_GENERAL);
> options->dev_type = p[1];
> }
> +#ifdef _WIN32
> +else if (streq(p[0], "windows-driver") && p[1] && !p[2])
> +{
> +VERIFY_PERMISSION(OPT_P_GENERAL);
> +options->wintun = parse_windows_driver(p[1], M_FATAL);
> +}
> +#endif
> else if (streq(p[0], "dev-node") && p[1] && !p[2])
> {
> VERIFY_PERMISSION(OPT_P_GENERAL);
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index ff7a5bb..0a24e5e 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -632,6 +632,7 @@ struct options
> bool show_net_up;
> int route_method;
> bool block_outside_dns;
> +bool wintun;
Did you consider using an enum instead? I think it would make the code
easier to read. E.g. with
typedef enum { WINDRV_TAP6, WINDRV_WINTUN } windrv_t;
in tun.h or so, we'd get
options->windrv = WINDRV_TAP6
instead of
options->wintun = false.
Re: [Openvpn-devel] [PATCH 1/7] Visual Studio: upgrade project files to VS2019
Hi, On Fri, Sep 20, 2019, 15:47 Lev Stipakov wrote: > Wintun driver is now signed for Windows 10, > > https://staging.openvpn.net/openvpn2/wintun-0.6-signed.zip > > So if you want to try out super fast openvpn2, no need to meddle anymore > with test mode / disabling signature checks. > Out of curiosity: does the (signed) driver from wintun.net not work? Of so, why? -Steffan > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] t_net.sh: wait for NO-CARRIER bit to settle before starting test
Hi,
On 19-09-19 09:28, Antonio Quartulli wrote:
> Interfaces of type tun are marked as NO-CARRIER when no process is
> attached to them. However, this bit gets set with some delay after
> creation.
>
> For this reason, it is better to wait for the bit to settle before
> starting any test, otherwise any timing influence on the test may lead
> to inconsistencies due to the NO-CARRIER bit randomly being or not in
> the snapshot output taken by t_net.sh.
>
> This patch add a 'sleep 1' command right after creation of the
> interface, to give the NO-CARRIER bit a chance to settle.
>
> This issue has been witnessed on a buildbot that is
> apparently slowler than average to run the unit tests.
>
> Signed-off-by: Antonio Quartulli
> ---
> tests/t_net.sh | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tests/t_net.sh b/tests/t_net.sh
> index 18799d12..97e947ab 100755
> --- a/tests/t_net.sh
> +++ b/tests/t_net.sh
> @@ -34,6 +34,13 @@ reload_dummy()
> {
> $RUN_SUDO $openvpn --dev $IFACE --dev-type tun --rmtun >/dev/null
> $RUN_SUDO $openvpn --dev $IFACE --dev-type tun --mktun >/dev/null
> +
> +# it seems that tun devices will settle on NO-CARRIER while not
> connected to
> +# any process, but this won't happen immediately. To avoid having the
> +# NO-CARRIER bit appear in the middle of the tests - which would
> compromise
> +# the results - let's wait 1 sec here for it to settle.
> +sleep 1
> +
> if [ $? -ne 0 ]; then
> echo "can't create interface $IFACE"
> exit 1
>
This return value check now checks that sleep succeeded instead of the
openvpn --mktun command. I don't think that is what you intended.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Adding support for wolfSSL backend
Hi, On 24-08-19 21:40, Gert Doering wrote: > On Sat, Aug 24, 2019 at 06:04:21PM +0200, Arne Schwabe wrote: >> I want to give you an honest opionion of mine to merging WolfSSL in >> OpenVPN. Please note, that this is my personal opinion and not to be >> confused to be an official OpenVPN community project or OpenVPN Inc >> position. > > Arne summarized things quite well. New and large additions need to > balance "what *our* users want/need", "what the core team finds > interesting enough to spend time on" and "how expensive in terms of > maintainer lifetime will it be to maintain that stuff". > > Since we're currently short on contributors that can review crypto > related code changes, and we do not have anyone in the team today > that can review WolfSSL interface code at all, this isn't likely > going to happen in the near future. Just like Arne and Gert, I believe we're currently not in the position to take on the maintenance burden of adding another crypto backend. I'd rather spend our time on fixing bugs and improving openvpn performance (the crypto libs are not the bottleneck). -Steffan signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/1] Start TLS after connection established without waiting.
Hi, On 24-07-19 13:41, Daniel Kaldor wrote: > There is a ~1s delay between establishing connection with remote > server and starting TLS handshake. > This change removes delay and improves connection time. While this sounds like something we'd like to have (i.e, feature-ACK), this doesn't really explain why this change resolves the issue, nor why this is the right fix. Can you please clarify what it changes in the connection sequence and why you believe this is the right way to get rid of the 1s second delay? Also, how did you test this patch? (TCP, UDP, with or without management, on fast/slow links, even running in production maybe? Etc.) Thanks, -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set
Hi,
On 13-08-19 23:31, Antonio Quartulli wrote:
> On 13/08/2019 23:26, David Sommerseth wrote:
>> wouldn't it be better to
>> do 'if (rgi6->iface[0])' instead? Since the buffer should be NULL terminated
>> and has to be NULL terminted for strlen() to function anyhow. But the
>> compiled code would be a bit more efficient (even though, this isn't
>> necessarily a performance critical code section).
>
> In my opinion strlen() is more readable for the casual developer
> checking this code. Behind iface[0] we may hide other ambiguous
> assumptions (even though this is not the case here, but we won't
> remember in some months from now).
Antionio's answer is of course the real reason to use strlen(), but you
nerd-sniped me by saying "compiled code would be a bit more efficient".
As you said, that wouldn't matter here, but still: compilers nowadays do
optimize calls to strlen, memset, memcpy, etc.
The following code
#include
int test(const char *buf) {
return (strlen(buf) > 0);
}
for example compiles with -O3 on my GCC 8.3 to
:
0: 31 c0 xor%eax,%eax
2: 80 3f 00cmpb $0x0,(%rdi)
5: 0f 95 c0setne %al
8: c3 retq
which indeed just compares the first byte of the buffer against zero.
So, unless there are important performance constraints, let's optimize
code for readability, rather than performance :)
FM [0]
-Steffan
[0] https://xkcd.com/356/
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH for 2.4] Correct the return value of cryptoapi RSA signature callbacks
On 27-07-19 05:12, selva.n...@gmail.com wrote: > From: Selva Nair > > Fixes the wrong check on siglen instead of *siglen for > signing failures. > > Bug reported by: lilulo > > Signed-off-by: Selva Nair > --- > src/openvpn/cryptoapi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index 720fce09..35a9ebc4 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -393,7 +393,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned > int m_len, > } > > *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), > padding); > -return (siglen == 0) ? 0 : 1; > + return (*siglen == 0) ? 0 : 1; > } > > /* decrypt */ > Acked-by: Steffan Karger ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Correct the return value of cryptoapi RSA signature callbacks
On 26-07-19 22:39, selva.n...@gmail.com wrote: > From: Selva Nair > > Fixes the wrong check on siglen instead of *siglen for > signing failures. > > Bug reported by: lilulo > > Signed-off-by: Selva Nair > --- > > 2.4 will need a separate patch > > src/openvpn/cryptoapi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index 0c11712e..2f2eee77 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -499,7 +499,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned > int m_len, > *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), > cng_padding_type(padding), 0); > > -return (siglen == 0) ? 0 : 1; > +return (*siglen == 0) ? 0 : 1; > } > > /* decrypt */ > @@ -973,7 +973,7 @@ pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, > size_t *siglen, > *siglen = priv_enc_CNG(cd, alg, tbs, (int)tbslen, sig, *siglen, > cng_padding_type(padding), (DWORD)saltlen); > > -return (siglen == 0) ? 0 : 1; > +return (*siglen == 0) ? 0 : 1; > } > > #endif /* OPENSSL_VERSION >= 1.1.0 */ > Acked-by: Steffan Karger ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] openssl: Fix compilation without deprecated OpenSSL 1.1 APIs
}
>
> -void
> -cipher_ctx_cleanup(EVP_CIPHER_CTX *ctx)
> -{
> -EVP_CIPHER_CTX_cleanup(ctx);
> -}
> -
> int
> cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx)
> {
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index a4072b9a..4ac8f24d 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -89,6 +89,18 @@ EVP_MD_CTX_new(void)
> }
> #endif
>
> +#if !defined(HAVE_EVP_CIPHER_CTX_RESET)
> +#define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_init
> +#endif
> +
> +#if !defined(HAVE_X509_GET0_NOTBEFORE)
> +#define X509_get0_notBefore X509_get_notBefore
> +#endif
> +
> +#if !defined(HAVE_X509_GET0_NOTAFTER)
> +#define X509_get0_notAfter X509_get_notAfter
> +#endif
> +
> #if !defined(HAVE_HMAC_CTX_RESET)
> /**
> * Reset a HMAC context
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 05ca4113..c029d0f2 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */
> void
> tls_init_lib(void)
> {
> +#if (OPENSSL_VERSION_NUMBER < 0x1010L &&
> !defined(LIBRESSL_VERSION_NUMBER))
> SSL_library_init();
> -#ifndef ENABLE_SMALL
> +# ifndef ENABLE_SMALL
> SSL_load_error_strings();
> -#endif
> +# endif
> OpenSSL_add_all_algorithms();
> -
> +#endif
> mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL,
> NULL);
> ASSERT(mydata_index >= 0);
> }
> @@ -89,9 +90,11 @@ tls_init_lib(void)
> void
> tls_free_lib(void)
> {
> +#if (OPENSSL_VERSION_NUMBER < 0x1010L &&
> !defined(LIBRESSL_VERSION_NUMBER))
> EVP_cleanup();
> -#ifndef ENABLE_SMALL
> +# ifndef ENABLE_SMALL
> ERR_free_strings();
> +# endif
> #endif
> }
>
> @@ -567,7 +570,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
> goto cleanup; /* Nothing to check if there is no certificate */
> }
>
> -ret = X509_cmp_time(X509_get_notBefore(cert), NULL);
> +ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
> if (ret == 0)
> {
> msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field.");
> @@ -577,7 +580,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
> msg(M_WARN, "WARNING: Your certificate is not yet valid!");
> }
>
> -ret = X509_cmp_time(X509_get_notAfter(cert), NULL);
> +ret = X509_cmp_time(X509_get0_notAfter(cert), NULL);
> if (ret == 0)
> {
> msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field.");
> @@ -660,10 +663,13 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx,
> const char *curve_name
> else
> {
> #if OPENSSL_VERSION_NUMBER >= 0x10002000L
> +#if (OPENSSL_VERSION_NUMBER < 0x1010L &&
> !defined(LIBRESSL_VERSION_NUMBER))
> +
> /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
> * loading */
> SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
> return;
> +#endif
> #else
> /* For older OpenSSL we have to extract the curve from key on our
> own */
> EC_KEY *eckey = NULL;
>
Code looks good, verified that TLS connections still works with mbedtls
and openssl builds.
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCHv2] openssl: Fix compilation without deprecated OpenSSL 1.1 APIs
Hi all, On 14-06-19 12:38, Arne Schwabe wrote: >> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) >> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset >> +#endif >> + >> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) >> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset >> +#endif > > These two keep the older API instead of switching to the new one, from > OpenSSL. > > # if OPENSSL_API_COMPAT < 0x1010L > # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) > # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c) > # endif > > Since just using only the new API in this case does not really work I > think in case it would be better to rather always use > EVP_CIPHER_CTX_reset isntead of init and have ifdefs in the 2-3 places > where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old > API when we bump our minimum OpenSSL version (and find this thing easy > since it is an ifdef depending on the openssl version). Why wouldn't using the new API work? _reset() is basically the new name for _cleanup(), which some some actual cleanup + init. As far as I can see it would work perfectly fine to use _reset() instead of the _init/_cleanup calls everywhere. We never call _init on uninitialized memory (which is the only case where _init() would work while _cleanup() would fail). All we'd have to do than is add something like #ifndef HAVE_EVP_CIPHER_CTX_RESET #define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_cleanup #endif to openssl_compat.h. That way we would just use the new API everywhere, and can get rid of the lines in the compat file once we drop support for OpenSSL 1.0. Or am I missing something here? -Steffan signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Fix broken fragment/mssfix with NCP
On 08-05-19 16:19, Arne Schwabe wrote: > Am 21.01.19 um 21:04 schrieb Lev Stipakov: >> From: Lev Stipakov >> >> NCP negotiation replaces worst cast crypto overhead >> with actual one in data channel frame. That frame >> params are used by mssfix. >> >> Fragment frame still contains worst case overhead. >> Because of that TCP packets are fragmented, since >> MSS value exceeds max fragment size. >> >> Fix by replacing worst case crypto overhead with >> actual one for fragment frame, as it is done for data >> channel frame. > > The change looks good and is tested. > > Acked-By: Arne Schwabe Since Arne agrees, I'm completely convinced now too: Acked-by: Steffan Karger signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Do not set pkcs11-helper "safe fork mode"
On 28-04-19 11:23, Steffan Karger wrote: > Together with the pkcs11-helper fixes, I do think this is the right fix. > I'll try to experiment a bit with it myself too. Finally got to some testing and staring at code. The patch resolves the issue for me, and I didn't find any other issues. Our code seems to be in good shape to disable safe fork mode as far as I can tell. I replied to the github thread on https://github.com/OpenVPN/openvpn/pull/121 with my conclusions: "The initialization order has been changed around 2015. Since then, OpenVPN initializes the crypto - including pkcs11 - always after daemonizing. Any PIN / password is always queried before that point. Since this patch will only be applied to release/2.4 and master, we're good in that aspect. With respect to slot events, I believe we're good too. If I understand the code correctly (@alonbl, please correct me if I'm wrong), slot events are only used if someone calls pkcs11h_setSlotEventHook(), which OpenVPN doesn't do. Summarizing, unless @alonbl tells me I'm wrong I think this patch is good and should be merged." -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Rate-limit incoming P_CONTROL_HARD_RESET_* packets.
Hi,
On 06-12-18 18:10, Gert Doering wrote:
> Creation of new instances (= new incoming reset packets with different
> source IP addresses / source ports) can be rate-limited in the current
> code by use of the "--connect-freq" option.
>
> For packets sent with the same source port, OpenVPN would dilligently
> reply to every single packet, causing route reflection issues (plus
> using somewhat more server cycles). This patch introduces a timestamp
> in the tls_multi context which records when the last "reset" packet
> was seen, and ignores all further "reset" packets coming in in the
> next 10 second interval.
This makes sense, but why 10s? Our default connect-retry value is 5
seconds. Wouldn't 5 (or 4?) be a better value in that regard?
> Signed-off-by: Gert Doering
> ---
> src/openvpn/ssl.c| 14 ++
> src/openvpn/ssl_common.h | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 74b88ce6..3078d76c 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -3468,6 +3468,20 @@ tls_pre_decrypt(struct tls_multi *multi,
> print_link_socket_actual(from, &gc));
> goto error;
> }
> +
> +/* only permit one is_hard_reset() packet per 10 seconds,
> + * ignore more frequent packets
> + */
> +time_t now = time(NULL);
> +if ( now - multi->last_hard_reset_seen < 10 )
I think we use the "if (now - multi->last_hard_reset_seen < 10)" style
in the vast majority of the code base.
> +{
> +msg(D_MULTI_ERRORS, "TLS: tls_pre_decrypt: ignore
> incoming"
> +" '%s' (only %ds since last RESET)",
> +packet_opcode_name(op),
> +(int)(now - multi->last_hard_reset_seen) );
Here too, no space before ).
> +goto error;
> +}
> +multi->last_hard_reset_seen = now;
> }
>
> /*
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 7bf82b3a..e71696b5 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -515,6 +515,7 @@ struct tls_multi
> */
> int n_hard_errors; /* errors due to TLS negotiation failure */
> int n_soft_errors; /* errors due to unrecognized or
> failed-to-authenticate incoming packets */
> +time_t last_hard_reset_seen; /* rate-limit incoming hard reset */
>
> /*
> * Our locked common name, username, and cert hashes (cannot change
> during the life of this tls_multi object)
>
Otherwise this looks good to me. I'll ACK once I understand your pick
for the timeout value.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Stop state-exhaustion attacks from a single source address.
Hi,
Sorry for the terrible response time, but here goes for initial review:
On 07-12-18 21:28, Gert Doering wrote:
> If an attacker sends lots of RESET packets from different source
> ports (but the same source address), every packet will create a
> new multi_instance, leading to resource waste on the server, and
> to lots of reply packets while OpenVPN tries to establish a
> TLS handshake.
>
> This can be rate-limited with "connect-freq", but if this is set
> too tightly, an attacker can drown out legitimate users of the
> same OpenVPN server.
>
> So, when deciding whether or not to create a new instance, iterate
> over all existing instances, counting all from the same source IP
> (ignoring source port info) that are "in TLS negotiation" state -
> if more than instances are already active, refuse new instance.
>
> The cutoff parameter can be configured by a newly introduced 3rd
> argument to "connect-freq". So something like this might be
> reasonable for a medium-sized server:
>
>connect-freq 20 20 3
>
> ("permit 20 new connections per 20 seconds, but only 3 from the same
> source IP address")
>
> Drawback: if many users are sitting behind a shared NAT ip address
> and they all reconnect at the same time, session setup will take
> longer for some of the users while the server is still handshaking
> with others.
This seems a very reasonable way - on top of tls-auth/tls-crypt - to
mitigate floods from a single IP. I'm not very familiar with this code
paths, what happens when a cient gets rejected? Does it get told "try
next server", or do we give it a silence treatment?
Also, I think it would be nice to have separate log messages in the
server log for "generic limit reached" and "per-ip limit reached".
Finally, aside from removing the debug prints, this will need some
whitespace polishing. The patch mixes tabs/spaces and styles surrounding
the parentheses in ifs.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
