| Message ID | 20210604143938.779193-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Delegated to: | Antonio Quartulli |
| Headers | show |
| Series | None | expand |
Hi, On 04/06/2021 16:39, Arne Schwabe wrote: > Previously we relied on checking tls_authentication_status to check > wether to determine if the context auth state is actually valid or not. > This patch eliminates that check by introducing waiting on the > authentication as extra state in the context auth, state machine. > > Patch v3: Fix ccd config from management being ignored > Patch v4: Fix race condition, we need to accept the config from > management if we are in CAS_WAITING_AUTH or earlier states > and not just in CAS_WAITING_AUTH state > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> This patch gets an ACK from me because all my tests have passed and I find the code reasonable. My compile zoo was happy too. My tests included client/server, with auth, with deferred auth, p2p. I have not tested this patch using the mgmt interface. Acked-by: Antonio Quartulli <antonio@openvpn.net> From now on, when Acking a patch I will imply that: * my GitLab CI is all green with this patch applied; * the patch compiled against mbedtls (various versions), openssl (various versions), libressl (various versions), wolfssl (master branch). This way I don't have to talk about my compile zoo every time :-)
Stared at the code a bit, discussed on IRC about "what state does what?"
- so this new state is "TLS is ok, waiting for (deferred) authentication"
and CAS_PENDING* is "waiting for (deferred) *client-connect* things" -
which MUST NOT run before authentication is finished (= CVE...).
With that explanation, the changes looks straightforward enough, with the
new state added and the explanation given.
Arne also stated that a patch will come that better documents all
CAS_ values.
Tested on the client side (no surprises) and on the server side test
rig, with all the nasties - deferred plugin auth, deferred client connect,
deferred script auth, succeeding and failing, config from ccd/ and from
--client-connect scripts - and it behaved nicely.
[Note: I still have no test rig with management auth, so we need to trust
the AS QA team to test all these cases...]
This still does not fix the "PUSH_REPLY is sent too quickly" CVE in
all cases, it seems. But with the *next* one, it is finally fixed.
As discussed on IRC, added a note about the CVE to the commit message.
Your patch has been applied to the master branch.
commit 489c45fb373adfb22c2f1dd0a524bde17c686876
Author: Arne Schwabe
Date: Fri Jun 4 16:39:38 2021 +0200
Make waiting on auth an explicit state in the context state machine
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210604143938.779193-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22491.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 3f9710134..eada7e155 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2596,11 +2596,6 @@ static const multi_client_connect_handler client_connect_handlers[] = { static void multi_connection_established(struct multi_context *m, struct multi_instance *mi) { - if (tls_authentication_status(mi->context.c2.tls_multi) != TLS_AUTHENTICATION_SUCCEEDED) - { - return; - } - /* We are only called for the CAS_PENDING_x states, so we * can ignore other states here */ bool from_deferred = (mi->context.c2.tls_multi->multi_state != CAS_PENDING); @@ -3970,7 +3965,7 @@ management_client_auth(void *arg, { if (auth) { - if (is_cas_pending(mi->context.c2.tls_multi->multi_state)) + if (mi->context.c2.tls_multi->multi_state <= CAS_WAITING_AUTH) { set_cc_config(mi, cc_config); cc_config_owned = false; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 9f3f83f16..fd64b8d4e 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2810,7 +2810,7 @@ tls_process(struct tls_multi *multi, if (session->opt->mode == MODE_SERVER) { /* On a server we continue with running connect scripts next */ - multi->multi_state = CAS_PENDING; + multi->multi_state = CAS_WAITING_AUTH; } else { @@ -3136,6 +3136,13 @@ tls_multi_process(struct tls_multi *multi, enum tls_auth_status tas = tls_authentication_status(multi); + /* If we have successfully authenticated and are still waiting for the authentication to finish + * move the state machine for the multi context forward */ + if (multi->multi_state == CAS_WAITING_AUTH && tas == TLS_AUTHENTICATION_SUCCEEDED) + { + multi->multi_state = CAS_PENDING; + } + /* * If lame duck session expires, kill it. */ diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 8a65ab984..66700bf68 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -511,6 +511,7 @@ struct tls_session * connect scripts/plugins */ enum multi_status { CAS_NOT_CONNECTED, + CAS_WAITING_AUTH, /**< TLS connection established but deferred auth not finished */ CAS_PENDING, CAS_PENDING_DEFERRED, CAS_PENDING_DEFERRED_PARTIAL, /**< at least handler succeeded, no result yet*/
Previously we relied on checking tls_authentication_status to check wether to determine if the context auth state is actually valid or not. This patch eliminates that check by introducing waiting on the authentication as extra state in the context auth, state machine. Patch v3: Fix ccd config from management being ignored Patch v4: Fix race condition, we need to accept the config from management if we are in CAS_WAITING_AUTH or earlier states and not just in CAS_WAITING_AUTH state Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/multi.c | 7 +------ src/openvpn/ssl.c | 9 ++++++++- src/openvpn/ssl_common.h | 1 + 3 files changed, 10 insertions(+), 7 deletions(-)