[Openvpn-devel] [PATCH] Fix regression of ignoring --user
Commit facb6fffb changed a call in the style of if(a() | b()) to if(a() || b()). While this looks identical, it is not. The first statement always executes b() while the second only executes b() if a() returns false. This lead to to the platform_state_user never to set as side effect and thus --user being ignored. Rewrite the code to make this more explicit. Signed-off-by: Arne Schwabe --- src/openvpn/init.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 07d43d49d..7de65a3ec 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -3597,10 +3597,14 @@ do_init_first_time(struct context *c) ALLOC_OBJ_CLEAR_GC(c->c0, struct context_0, >gc); c0 = c->c0; -/* get user and/or group that we want to setuid/setgid to */ -c0->uid_gid_specified = -platform_group_get(c->options.groupname, >platform_state_group) -|| platform_user_get(c->options.username, >platform_state_user); +/* get user and/or group that we want to setuid/setgid to, + * sets also platform_x_state */ +bool group_defined = platform_group_get(c->options.groupname, + >platform_state_group); +bool user_defined = platform_user_get(c->options.username, + >platform_state_user); + +c0->uid_gid_specified = user_defined || group_defined; /* perform postponed chdir if --daemon */ if (c->did_we_daemonize && c->options.cd_dir == NULL) -- 2.37.0 (Apple Git-136) ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] TLS: do not lock empty usernames
On Wed, Oct 19, 2022 at 1:05 AM Arne Schwabe wrote: > > > If we can conjure up usernames (like with empty --> token-user) why not > allow other username > changes too? > > In general the current authentication system in OpenVPN is ill equipped to > handle them. On renegotiation we only do auth but no read in ccd or other > other user specific data. So allowing a username change could in many > instances give the new user the permissions/IP etc of the old user. There > can be situations where this is okay and we can add options or auth results > that explicitly allow. In general a username change probably leads > authorisation problems. For the auth-token-user the idea there is that you > get a username on the first auth assigned that you should use in the future > but no actual user change. > That makes a lot of sense. Even I have setups where what is pushed to the client depends on the username in addition to the commonname. This has implications when we have interactively authenticated, but long running connections which multiple users may be able to use though it will appear to be associated with one user from the server's pov (so-called Persistent connections in Windows GUI). Same with PLAP. Requiring a new tunnel on user name change alleviates this a bit, but still there will be a duration until next reauth or token expiry when userA is using a tunnel started by userB. Instead of wading into an OT discussion, I will raise this issue elsewhere / a new thread Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Fix regression of ignoring --user
Acked-by: Gert Doering
Glad that this obscure AS option caught this breakage... *sigh* - that's
what you get for "just fix this compiler warning" (which I am usually
quite reluctant to do, but "it looked straightforward enough").
The new code is less magic, and much easier to understand - good.
Lightly tested only ("run one instance with --user or --group, see
if log and ps output agree").
Your patch has been applied to the master branch.
commit 66f6a3b7991d5316116ce7cb91dfa4d71d4df42f
Author: Arne Schwabe
Date: Wed Oct 19 15:36:27 2022 +0200
Fix regression of ignoring --user
Signed-off-by: Arne Schwabe
Acked-by: Gert Doering
Message-Id: <20221019133627.2918110-1-a...@rfc2549.org>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25430.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Improve data key id not found error message
Patch v2: fix comparing key_id to state value, improve message Patch v3: also take key_id into account Code looks good to me now. Is there any way of testing this? You need to get both peer in some kind of inconsistent state. Like short reneg-sec on one side and dealyed auth on the other side or deauthing (e.g. via management) and seeing packet in the last five seconds before the session gets killed. Also somethning like reneg-sec 60 on one side might trigger these warnings. If I trigger them again with something reliable to trigger them, I will follow up. Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3] Introduce dynamic tls-crypt for secure soft_reset/session renegotiation
Currently we have only one slot for renegotiation of the session/keys If a replayed/faked packet is inserted by a malicous attacker, the legimate peer cannot renegotiate anymore. This commit introduces dynamic tls-crypt. When both peer support this feature, both peer create a dynamic tls-crypt key using TLS EKM (export key material) and will enforce using that key and tls-crypt for all renegotiations. Since one of tls-crypt/tls-crypt-v2 purpose is to provide poor man's post quantum crypto guarantees, we have to ensure that the dynamic key tls-crypt key that replace the original tls-crypt key is as strong as the orginal key to avoid problems if there is a weak RNG or TLS EKM produces weak keys. We ensure this but XORing the original key with the key from TLS EKM. If tls-crypt/tls-cryptv2 is not active, we use just the key generated by TLS EKM. We also do not use hashing or anything else on the original key before XOR to avoid any potential of a structure in the key or something else that might weaken post-quantum use cases. OpenVPN 2.x reserves the TM_ACTIVE session for renegotians. When a SOFT_RESET_V1 packet is received, the active TLS session is moved from KS_PRIMARY to KS_SECONDARY. If the SOFT_RESET_V1 came from a replay or a was faked (no tls-auth/tls-crypt), the session is blocked until the TLS renegotiation attempt times out, blocking the legimitate client. Using a dynamic tls-crypt key here block any SOFT_RESET_V1 as replay and fake packets will not have a matching authentication/encryption are discarded. HARD_RESET packets that are from a reconnecting peer are instead put in the TM_UNTRUSTED/KS_PRIMARY slot until they are sufficiently verified, so the dyanmic tls-crypt key is not used here. Replay/fake packets also do not block the legimiate client. This commit delays the purging of the original tls-crypt key data from directly after passing it to crypto library to tls_wrap_free. We do this to allow us mixing the new exported key with the original key. Since need to be able to generate the secure renegotiation key, deleting the key after generating a secure renegotiation key is not an option. Even when the client does not support secure renegotiation, deleting the key is not a good as the reconnecting client or (especially in p2p mode with float) another client does the reconnect, we might need to generate a secure renegotiation key. Delaying the deletion of the key has also little effect as the key is still present in the OpenSSL/mbed TLS structures in the tls_wrap structure, so only the number the keys is in memory would be reduced. The issue was initially reported by Fabio Streun and has been assigned CVE-2021-3568. Patch v2: fix spellings of reneg and renegotiations. Patch v3: expand comment to original_tlscrypt_keydata and commit message, add Changes.rst Signed-off-by: Arne Schwabe --- Changes.rst | 6 ++ src/openvpn/auth_token.h | 2 +- src/openvpn/crypto.c | 7 +- src/openvpn/crypto.h | 16 +++- src/openvpn/init.c| 8 +- src/openvpn/multi.c | 4 + src/openvpn/openvpn.h | 2 + src/openvpn/options.c | 4 + src/openvpn/push.c| 5 ++ src/openvpn/ssl.c | 25 -- src/openvpn/ssl.h | 5 ++ src/openvpn/ssl_backend.h | 1 + src/openvpn/ssl_common.h | 13 src/openvpn/ssl_ncp.c | 4 + src/openvpn/ssl_pkt.c | 2 +- src/openvpn/ssl_pkt.h | 21 + src/openvpn/tls_crypt.c | 93 --- src/openvpn/tls_crypt.h | 19 - tests/unit_tests/openvpn/test_pkt.c | 17 - tests/unit_tests/openvpn/test_tls_crypt.c | 85 + 20 files changed, 310 insertions(+), 29 deletions(-) diff --git a/Changes.rst b/Changes.rst index fc5a1a853..5c3876512 100644 --- a/Changes.rst +++ b/Changes.rst @@ -99,6 +99,12 @@ Inline auth username and password missing OpenVPN will prompt for input via stdin. This applies to inline'd http-proxy-user-pass too. +Secure renegotiation +When both peer are OpenVPN 2.6.0+, OpenVPN will use secure renegotiation +using a dynamically created tls-crypt key. This ensure that only the +previously authenticated peer can do trigger renegotiation and complete +renegotiations. This also closes CVE-2021-3568. + Deprecated features --- diff --git a/src/openvpn/auth_token.h b/src/openvpn/auth_token.h index 5e23d8c44..0a847ce56 100644 --- a/src/openvpn/auth_token.h +++ b/src/openvpn/auth_token.h @@ -43,7 +43,7 @@ * * The second timestamp is the time the token was renewed/regenerated and is used * to determine if this token has been renewed in the acceptable time range - * (2 * renogiation timeout) +
Re: [Openvpn-devel] [PATCH] test_crypto: fix test_occ_mtu_calculation with --disable-fragment
Am 24.06.22 um 14:26 schrieb Frank Lichtenheld: Doesn't make sense to test with fragment, if the code ignores it. Acked-By: Arne Schwabe ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Is auth-nocache broken?
Hi, Using --auth-user-pass, --auth-nocache and --reneg-sec , no auth-tokens in use, I see that username/password is prompted on the first connection attempt and at first renegotiation. After that reneg completes without prompting for user/pass. Looking at the server it shows the previously entered password is passed in.So auth-nocache is no longer effective after the first renegotiation? A log snippet using a local build that also prints when purge_user_pass() and get_user_pass_cr() are entered is attached. After reneg, the client progresses beyond AUTH state (as reported on Trac #1471 ( https://community.openvpn.net/openvpn/ticket/1471) which may be related. Unless it has been like this all along. Selva nocache.log Description: Binary data ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: test_crypto: fix test_occ_mtu_calculation with --disable-fragment
Change makes sense :-) - have not tested any actual builds without ENABLE_FRAGMENT to verify that it exploded *before*, but it looks like this will fix it. Your patch has been applied to the master branch. commit 252e5d6888261d7b2aae1034c834fba8a393cef4 Author: Frank Lichtenheld Date: Fri Jun 24 14:26:57 2022 +0200 test_crypto: fix test_occ_mtu_calculation with --disable-fragment Signed-off-by: Frank Lichtenheld Acked-by: Arne Schwabe Message-Id: <20220624122657.28675-1-fr...@lichtenheld.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24550.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Is auth-nocache broken?
> > After reneg, the client progresses beyond AUTH state (as reported on Trac > #1471 ( https://community.openvpn.net/openvpn/ticket/1471) which may be > related. Unless it has been like this all along. > Please Ignore that comment -- Trac# 1471 is a special case and may not be related at all. In this case there is not state stuck at AUTH issue -- just password does not get purged. Selva > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH for 2.5/2.6] Purge auth-token as well while purging passwords
From: Selva Nair Starting from commit e61b401a auth-token is saved in a separate struct from auth-user-pass and is not cleared when ssl_purge_auth() is called. This makes "forget-passwords" sent to the management interface or "--management-forget-disconnect" option not to work as expected. Purging caused by --auth-nocache is not affected (auth-token is retained in that case as it should be). Use case: For Pre-Logon access and persistent connections on Windows, use of "forget-passwords" before disconnect is probably the only way to ensure that no credentials are left behind. Note that openvpn.exe continues to run after disconnect in these cases. Also, the original intent of "forget-passwords" appears to be to clear all "passwords" that can be used to reconnect. Signed-off-by: Selva Nair --- src/openvpn/ssl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index d9b21819..4c0d78a1 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -492,6 +492,7 @@ ssl_purge_auth(const bool auth_user_pass_only) purge_user_pass(, true); } purge_user_pass(_user_pass, true); +purge_user_pass(_token, true); #ifdef ENABLE_MANAGEMENT ssl_purge_auth_challenge(); #endif -- 2.30.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Is auth-nocache broken?
Hi, On Wed, Oct 19, 2022 at 02:33:27PM -0400, Selva Nair wrote: > Using --auth-user-pass, --auth-nocache and --reneg-sec , no > auth-tokens in use, I see that username/password is prompted on the first > connection attempt and at first renegotiation. After that reneg completes > without prompting for user/pass. It's possibly we broke that by trying to repair all the corner cases with either pushing tokens from the server, or *not* using auth-nocache. I assume you tested with master? (Unfortunately my automated tests all use "username + passwords are coming from a file", which means "send the same one as before" and "go read the file again" both produce the same effect... having a management-interface driven client test would help here... no time yet to write one) gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: FreeBSD: for topology subnet, put tun interface into IFF_BROADCAST mode
Thanks for the review, Kristof. Just to be sure, I've run this through extensive client/server tests on FreeBSD 14 + 12.3 again. Still works :-) (Except that this patch, as documented, breaks "topology subnet with DCO" - to be fixed in the next commit) Patch has been applied to the master branch. commit 94db32616597497e57eb2fa6fab05297da314a53 Author: Gert Doering Date: Wed Oct 12 16:59:14 2022 +0200 FreeBSD: for topology subnet, put tun interface into IFF_BROADCAST mode Signed-off-by: Gert Doering Acked-by: Kristof Provost Message-Id: <20221012145915.25810-1-g...@greenie.muc.de> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25396.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: FreeBSD DCO: introduce real subnet mode
Thanks, Kristof for the review, and for merging the kernel side.
I have added the kernel commit ID to the commit message for this
patch ("if the kernel is too old, SET_IFMODE will fail and
topology subnet will not work") - and verified that this is indeed
the result, p2p mode with net30 works, subnet fails. And of course,
tested with a newer if_ovpn.ko, and all works :-)
I have also taken the liberty to remove the debug message printing
the ifmode to-be-set, and to add the ifmode + 'dco_set_ifmode:' to
the error message, so log file output is more useful.
Patch has been applied to the master branch.
commit 22bc63c78439ed23b974b8f822330d75ec79c7fc
Author: Gert Doering
Date: Wed Oct 12 16:59:15 2022 +0200
FreeBSD DCO: introduce real subnet mode
Signed-off-by: Gert Doering
Acked-by: Kristof Provost
Message-Id: <20221012145915.25810-2-g...@greenie.muc.de>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25395.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 2/3] Allow setting control channel packet size with max-packet-size
On Wed, Sep 21, 2022 at 12:49:29PM +0200, Arne Schwabe wrote: > Currently control packet size is controlled by tun-mtu in a very > non-obvious way since the control overhead is not taken into account > and control channel packet will end up with a different size than > data channel packet. > > Instead we decouple this and introduce max-packet-size. Control packet size > defaults to 1250 if max-packet-size is not set. > > Patch v2: rebase on latest patch set > Patch v3: Introduce TLS_CHANNEL_MTU_MIN define and give explaination > of its value. > Patch v4: introduce max-packet-size instead of tls-mtu In general I think this change really could use more documentation. E.g. so we imply --mssfix, this should be documented under --mssfix. Also we should look at all existing examples with --mssfix/--fragment/--tun-mtu and check whether they should be changed to use different options. E.g. how does --max-packet-size interact with the argument-less --mssfix? That should be mentioned. At the moment this patch adds an option but gives the user no indication when and why to use it compared to all other MTU-related options. Some other small suggestions below. > diff --git a/Changes.rst b/Changes.rst > index 275f8d647..25b0e9846 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -88,6 +88,12 @@ Data channel offloading with ovpn-dco > which brings DATA_V2 packet support. > > > +Improved control channel packet size control (``max-packet-size``) > +The size of control channel is no longer tied to > +``--link-mtu``/``--tun-mtu`` and can be set using ``--max-packet-size``. > +Setting the size to small sizes no longer breaks the OpenVPN protocol in > +certain situations. This should mention the implied --mssfix. This way it sounds like it only affects control channel. > Deprecated features > --- > ``inetd`` has been removed > @@ -150,6 +156,9 @@ User-visible Changes > - Option ``--nobind`` is default when ``--client`` or ``--pull`` is used in > the configuration > - :code:`link_mtu` parameter is removed from environment or replaced with 0 > when scripts are >called with parameters. This parameter is unreliable and no longer > internally calculated. > +- control channel packet maximum size is no longer influenced by > ``--link-mtu``/``--tun-mtu`` > + and must be set by ``--max-packet-size`` now. > + > > Overview of changes in 2.5 > == > diff --git a/doc/man-sections/link-options.rst > b/doc/man-sections/link-options.rst > index 373193aa3..176c2b0db 100644 > --- a/doc/man-sections/link-options.rst > +++ b/doc/man-sections/link-options.rst > @@ -454,3 +454,19 @@ the local and the remote host. > if mode server: > socket-flags TCP_NODELAY > push "socket-flags TCP_NODELAY" > + > +--max-packet-size size > + This option will instruct OpenVPN to try to limit the maximum on-write > packet > + size by restricting the control channel packet size and setting > ``--mssfix``. > + > + OpenVPN will try to keep its control channel messages below this size but > + due to some constraints in the protocol this is not always possible. If the > + option is not set, the control packet maximum size defaults to 1250. > + The control channel packet size will be restricted to values between > + 512 and 2048. The maximum packet size includes encapsulation overhead like > + UDP and IP. > + > + For ``--mssfix`` it will expand to: > + :: > + > + mssfix size mtu > \ No newline at end of file Missing newline > diff --git a/src/openvpn/common.h b/src/openvpn/common.h > index b94680885..056c25438 100644 > --- a/src/openvpn/common.h > +++ b/src/openvpn/common.h > @@ -68,6 +68,16 @@ typedef unsigned long ptr_type; > */ > #define TLS_CHANNEL_BUF_SIZE 2048 > > +/* TLS control buffer minimum size, this size is not actually inherent to > + * the protocol but. Our current sending window is 6 and the receive window > + * is 8 or 12 depending on the OpenVPN version. We need to be able to send > + * a TLS record of size TLS_CHANNEL_BUF_SIZE. Splitting this into more than > + * 6 packets (with overhead) would complicate our sending logic a lot more, > so > + * we settle here for a "round" number that allow with overhead of ~100 bytes > + * to be larger than TLS_CHANNEL_BUF_SIZE. E.g. 6x ~400 > 2048. That sentence is difficult to parse, specifically "so we settle here for a "round" number that allow with overhead of ~100 bytes to be larger than TLS_CHANNEL_BUF_SIZE". I'm not completely sure what you want to say so I find it difficult to make recommendations. > + * */ > +#define TLS_CHANNEL_MTU_MIN 512 > + > /* > * This parameter controls the maximum size of a bundle > * of pushed options. [...] > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index e44993c03..a7385118e 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -825,6 +825,7 @@ init_options(struct options *o, const bool
Re: [Openvpn-devel] [PATCH v4 3/3] Add unit test for reliable_get_num_output_sequenced_available
On Tue, Oct 18, 2022 at 06:16:54PM +0200, Frank Lichtenheld wrote: > Acked-By: Frank Lichtenheld > > Already acked in 1957647252.642516.1652264253...@office.mailbox.org > Note: this patch does not depend on 2/3, only on 1/3. Regards, -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
