[Openvpn-devel] Fix auth-token usage with management-def-auth

Message ID 20220704025840.2558-1-selva.nair@gmail.com
State New
Headers show
Series
  • [Openvpn-devel] Fix auth-token usage with management-def-auth
Related show

Commit Message

Selva Nair July 4, 2022, 2:58 a.m.
From: Selva Nair <selva.nair@gmail.com>

When auth-token verify succeeds during a reauth, other auth
methods (plugin, script, management) are skipped unless
external-auth is in effect (skip_auth gets set to true).

However, in this case, the status of management-def-auth
(ks->mda_satus) stays at its default value of ACF_PENDING
and will never change. This causes TLS keys to go out of sync
and an eventual client disconnect.

Further, a message saying username/password authentication is
"deferred" gets logged which is misleading.
For example:

test/127.0.0.1:35874 TLS: Username/auth-token authentication
    succeeded for username 'test'

followed by

test/127.0.0.1:35874 TLS: Username/Password authentication
    deferred for username 'test' [CN SET]

Fix by setting ks->mda_status to ACF_DISABLED, and do not
set ks->authenticated = KS_AUTH_DEFERRED when skip_auth is true.

Also log a warning message when token is marked as expired on
missing the reneg window.

Reported by: Connor Edwards <connor.edwards@b2c2.com>

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/auth_token.c | 8 +++++---
 src/openvpn/ssl_verify.c | 9 ++++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

Comments

Kristof Provost via Openvpn-devel July 4, 2022, 8:30 a.m. | #1
Looks good. Shortly after my last email about the issue I found this bit of
code and also came to the conclusion it was faulty.

Thanks for the patch Selva.
Arne Schwabe July 4, 2022, 9:50 a.m. | #2
Am 04.07.22 um 04:58 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> When auth-token verify succeeds during a reauth, other auth
> methods (plugin, script, management) are skipped unless
> external-auth is in effect (skip_auth gets set to true).
> 
> However, in this case, the status of management-def-auth
> (ks->mda_satus) stays at its default value of ACF_PENDING

ks->mda_status

> and will never change. This causes TLS keys to go out of sync
> and an eventual client disconnect.
> 
> Further, a message saying username/password authentication is
> "deferred" gets logged which is misleading.
> For example:
> 
> test/127.0.0.1:35874 TLS: Username/auth-token authentication
>      succeeded for username 'test'
> 
> followed by
> 
> test/127.0.0.1:35874 TLS: Username/Password authentication
>      deferred for username 'test' [CN SET]
> 
> Fix by setting ks->mda_status to ACF_DISABLED, and do not
> set ks->authenticated = KS_AUTH_DEFERRED when skip_auth is true.
> 
> Also log a warning message when token is marked as expired on
> missing the reneg window.
> 
> Reported by: Connor Edwards <connor.edwards@b2c2.com>
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>

Note that you need have management enabled for this bug to trigger. If 
you go through all the effort to talk to management like this, you 
probably want to use external-auth anyway.

Basically currently AS is the only management+server mode application 
that active developer use, bugs like this tend to slip.

Arne
Selva Nair July 4, 2022, 10:35 a.m. | #3
Hi

On Mon, Jul 4, 2022 at 5:50 AM Arne Schwabe <arne@rfc2549.org> wrote:

> Am 04.07.22 um 04:58 schrieb selva.nair@gmail.com:
> > From: Selva Nair <selva.nair@gmail.com>
> >
> > When auth-token verify succeeds during a reauth, other auth
> > methods (plugin, script, management) are skipped unless
> > external-auth is in effect (skip_auth gets set to true).
> >
> > However, in this case, the status of management-def-auth
> > (ks->mda_satus) stays at its default value of ACF_PENDING
>
> ks->mda_status
>
> > and will never change. This causes TLS keys to go out of sync
> > and an eventual client disconnect.
> >
> > Further, a message saying username/password authentication is
> > "deferred" gets logged which is misleading.
> > For example:
> >
> > test/127.0.0.1:35874 TLS: Username/auth-token authentication
> >      succeeded for username 'test'
> >
> > followed by
> >
> > test/127.0.0.1:35874 TLS: Username/Password authentication
> >      deferred for username 'test' [CN SET]
> >
> > Fix by setting ks->mda_status to ACF_DISABLED, and do not
> > set ks->authenticated = KS_AUTH_DEFERRED when skip_auth is true.
> >
> > Also log a warning message when token is marked as expired on
> > missing the reneg window.
> >
> > Reported by: Connor Edwards <connor.edwards@b2c2.com>
> >
>
> Acked-By: Arne Schwabe <arne@rfc2549.org>
>
> Note that you need have management enabled for this bug to trigger. If
> you go through all the effort to talk to management like this, you
> probably want to use external-auth anyway.
>

I agree. This kind of fiddling with flags like mda_status is not clean and
easy to break again. I use management-def-auth but do not use
auth-gen-token -- instead the management script keeps track of
reauth, lifetime of 2FA etc. If I were to "modernize" that setup I would
use auth-gen-token with external-auth as well.

That said, for 2.5, an easy fix like this is good enough?

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 4, 2022 at 5:50 AM Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 04.07.22 um 04:58 schrieb <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>:<br>
&gt; From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
&gt; <br>
&gt; When auth-token verify succeeds during a reauth, other auth<br>
&gt; methods (plugin, script, management) are skipped unless<br>
&gt; external-auth is in effect (skip_auth gets set to true).<br>
&gt; <br>
&gt; However, in this case, the status of management-def-auth<br>
&gt; (ks-&gt;mda_satus) stays at its default value of ACF_PENDING<br>
<br>
ks-&gt;mda_status<br>
<br>
&gt; and will never change. This causes TLS keys to go out of sync<br>
&gt; and an eventual client disconnect.<br>
&gt; <br>
&gt; Further, a message saying username/password authentication is<br>
&gt; &quot;deferred&quot; gets logged which is misleading.<br>
&gt; For example:<br>
&gt; <br>
&gt; test/<a href="http://127.0.0.1:35874" rel="noreferrer" target="_blank">127.0.0.1:35874</a> TLS: Username/auth-token authentication<br>
&gt;      succeeded for username &#39;test&#39;<br>
&gt; <br>
&gt; followed by<br>
&gt; <br>
&gt; test/<a href="http://127.0.0.1:35874" rel="noreferrer" target="_blank">127.0.0.1:35874</a> TLS: Username/Password authentication<br>
&gt;      deferred for username &#39;test&#39; [CN SET]<br>
&gt; <br>
&gt; Fix by setting ks-&gt;mda_status to ACF_DISABLED, and do not<br>
&gt; set ks-&gt;authenticated = KS_AUTH_DEFERRED when skip_auth is true.<br>
&gt; <br>
&gt; Also log a warning message when token is marked as expired on<br>
&gt; missing the reneg window.<br>
&gt; <br>
&gt; Reported by: Connor Edwards &lt;<a href="mailto:connor.edwards@b2c2.com" target="_blank">connor.edwards@b2c2.com</a>&gt;<br>
&gt; <br>
<br>
Acked-By: Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>&gt;<br>
<br>
Note that you need have management enabled for this bug to trigger. If <br>
you go through all the effort to talk to management like this, you <br>
probably want to use external-auth anyway.<br></blockquote><div><br></div><div>I agree. This kind of fiddling with flags like mda_status is not clean and easy to break again. I use management-def-auth but do not use auth-gen-token -- instead the management script keeps track of reauth, lifetime of 2FA etc. If I were to &quot;modernize&quot; that setup I would use auth-gen-token with external-auth as well.</div><div><br></div><div>That said, for 2.5, an easy fix like this is good enough?</div><div><br></div><div>Selva</div></div></div>

Patch

diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index 096edc75..b5f9f6dd 100644
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -346,20 +346,22 @@  verify_auth_token(struct user_pass *up, struct tls_multi *multi,
         return 0;
     }
 
-    /* Accept session tokens that not expired are in the acceptable range
-     * for renogiations */
+    /* Accept session tokens only if their timestamp is in the acceptable range
+     * for renegotiations */
     bool in_renegotiation_time = now >= timestamp
                                  && now < timestamp + 2 * session->opt->renegotiate_seconds;
 
     if (!in_renegotiation_time)
     {
+        msg(M_WARN, "Timestamp (%" PRIu64 ") of auth-token is out of the renegotiation window",
+            timestamp);
         ret |= AUTH_TOKEN_EXPIRED;
     }
 
     /* Sanity check the initial timestamp */
     if (timestamp < timestamp_initial)
     {
-        msg(M_WARN, "Initial timestamp (%" PRIu64 " in token from client earlier than "
+        msg(M_WARN, "Initial timestamp (%" PRIu64 ") in token from client earlier than "
             "current timestamp %" PRIu64 ". Broken/unsynchronised clock?",
             timestamp_initial, timestamp);
         ret |= AUTH_TOKEN_EXPIRED;
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index c01841fa..45eaf8ed 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1599,7 +1599,14 @@  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
 #ifdef ENABLE_MANAGEMENT
         if (man_def_auth != KMDA_UNDEF)
         {
-            ks->authenticated = KS_AUTH_DEFERRED;
+            if (skip_auth)
+            {
+                ks->mda_status = ACF_DISABLED;
+            }
+            else
+            {
+                ks->authenticated = KS_AUTH_DEFERRED;
+            }
         }
 #endif
         if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME))