diff mbox series

[Openvpn-devel] Ensure --auth-nocache is handled during renegotiation

Message ID 20221023195105.31714-1-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Ensure --auth-nocache is handled during renegotiation |

Commit Message

Selva Nair Oct. 23, 2022, 8:51 a.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Currently, clearing auth_user_pass struct is delayed until
push-reply processing to support auth-token. This results in
username/password not purged after renegotiations that may
not accompany any pushed tokens -- say, when auth-token is not
in use.

Fix by always clearing auth_user_pass soon after it is used,
instead of delaying the purge as in pre-token days. But, when
"pull" is true, retain the username in auth_token in anticipation
of a token that may or may not arrive later.

Remove ssl_clean_user_pass() as there is no delayed purge any
longer -- auth-nocache handling is now done immediately after
writing username/password to the send-buffer.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/init.c | 13 -------------
 src/openvpn/misc.c | 10 ++--------
 src/openvpn/ssl.c  | 23 +++++------------------
 src/openvpn/ssl.h  |  6 ------
 4 files changed, 7 insertions(+), 45 deletions(-)

Comments

Arne Schwabe Oct. 24, 2022, 1:10 a.m. UTC | #1 
Am 23.10.22 um 21:51 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Currently, clearing auth_user_pass struct is delayed until
> push-reply processing to support auth-token. This results in
> username/password not purged after renegotiations that may
> not accompany any pushed tokens -- say, when auth-token is not
> in use.
> 
> Fix by always clearing auth_user_pass soon after it is used,
> instead of delaying the purge as in pre-token days. But, when
> "pull" is true, retain the username in auth_token in anticipation
> of a token that may or may not arrive later.
> 
> Remove ssl_clean_user_pass() as there is no delayed purge any
> longer -- auth-nocache handling is now done immediately after
> writing username/password to the send-buffer.

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

I think the delaying and complicated logic, which thiss patch removes, 
is the last part of the attempt to try to use the same auth_user_pass 
struct for both auth token and user/password.

Arne
Gert Doering Oct. 26, 2022, 7:07 p.m. UTC | #2 
Looking at the code, I'd say your commit description makes a lot of
sense, and explains the observed "funnies" quite well (which I could
reproduce).  The code change looks reasonable, Arne ACKed this, and
I could not find a way to break it.  Good :-)

One comment, though: when auth-tokens are in use, this call chain

        if (auth_token.token_defined && auth_token.defined)
        {
            up = &auth_token;
        }   
        /* save username for auth-token which may get pushed later */
        if (session->opt->pull)
        {   
            strncpynt(auth_token.username, up->username, USER_PASS_LEN);
        } 

.. will effectively copy auth_token.username to itself, which looks
a bit silly but *should* be well-defined.  At least it did not cause
issues in my auth-token tests.  If we think this is unwanted, having
a check like

        if (session->opt->pull && up != &auth_token)

would avoid that ("copy only if not working on auth_token already").


Testing took quite a bit of test cases + renegotiations to catch
"the original problem" and verify that "the fix" does not break 
related use cases (since this is a tangled mess of a-u-p and tokens)...
but it looks good!

old code (commit 7d48d31b8226d5, fairly recent "master", and also 
with 2.5.6 / commit 7b1b100557):

 * --auth-user-pass --auth-nocache --reneg-sec 150  (no tokens)
   expected behaviour:
     - u+p reauth every 150 seconds, prompt for username+password
   result: 
     - asks on *first* renegotiation
     - ran into a --ping-restart timeout
     - reconnected with *cached* username + password  <<< this is unwanted
     - asks again on *first* reneg
     - next 100 renegs with *cached* u+p <<< unwanted

   -> so, clearly somewhat nondeterministic and not what was ordered

with "master + patch" and also "release/2.5 + patch":

 * --auth-user-pass --reneg-sec 150
   expected behaviour:
     - u+p reauth every 150 seconds, no prompting on reneg
   result: works fine (no prompts after the initial ones)

 * --auth-user-pass --auth-nocache --reneg-sec 150
   expected behaviour:
     - u+p reauth every 150 seconds, prompt for username+password
   result: works fine (prompts on every reneg)

 * --auth-user-pass --reneg-sec 150                 + server-pushed auth-token
   expected behaviour:
     - token reauth ever 150 sec
     - u+p reauth when the token expires (600s here), no prompting
   result: works fine (no prompts on reneg ever)
     - terminate + restart server -> reauth with token, no prompting
   result: works fine

   NOTE: due to reasons outside of this patch, "renegotiate with UP without
   prompting" only works if there is a continuous connection to the server
   - if the server connection breaks, server_pushed_signal()->ssl_purge_auth()
   happens, and UP is forgotten, so when the token expires, prompts ensue
   (surprising behaviour, but not caused by this patch).

 * --auth-user-pass --auth-nocache --reneg-sec 150  + server-pushed auth-token
   expected behaviour:
     - token reauth ever 150 sec
     - u+p reauth when the token expires (600s), prompt for username+password
   result: works fine (prompts at token expiry, *only*)
     - terminate + restart server -> reauth with token, no prompting
   result: works fine

(never using --auth-user-pass <file>, because that makes it hard to verify
without extra instrumentation whether it used the cached copy, or the
on-file copy)

Your patch has been applied to the master and release/2.5 branch.

commit 3a4fb17d103be37599d72d072bbee42cc121a39d (master)
commit 5ad4b4b374f072459ab2436ed372c92d3a42d65d (HEAD -> release/2.5)
Author: Selva Nair
Date:   Sun Oct 23 15:51:05 2022 -0400

     Ensure --auth-nocache is handled during renegotiation

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20221023195105.31714-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25452.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair Oct. 27, 2022, 4:18 a.m. UTC | #3 
On Thu, Oct 27, 2022 at 2:08 AM Gert Doering <gert@greenie.muc.de> wrote:

> Looking at the code, I'd say your commit description makes a lot of
> sense, and explains the observed "funnies" quite well (which I could
> reproduce).  The code change looks reasonable, Arne ACKed this, and
> I could not find a way to break it.  Good :-)
>

Thanks for the laborious testing!

2.4 is also affected  as commit dfd624b52 was cherrypicked into it as well:
(commit 607dfa9)
But this patch won't apply due to some context changes.

Would you like a resolved patch or is 2.4 out of "support" now?

Selva
Gert Doering Oct. 27, 2022, 7:37 a.m. UTC | #4 
Hi,

On Thu, Oct 27, 2022 at 11:18:07AM -0400, Selva Nair wrote:
> On Thu, Oct 27, 2022 at 2:08 AM Gert Doering <gert@greenie.muc.de> wrote:
> 
> > Looking at the code, I'd say your commit description makes a lot of
> > sense, and explains the observed "funnies" quite well (which I could
> > reproduce).  The code change looks reasonable, Arne ACKed this, and
> > I could not find a way to break it.  Good :-)
> Thanks for the laborious testing!

We've been bitten *so* often already by --auth-nocache + --auth-token
combinations on the path here that I've become very careful ;-)

> 2.4 is also affected  as commit dfd624b52 was cherrypicked into it as well:
> (commit 607dfa9)
> But this patch won't apply due to some context changes.
> 
> Would you like a resolved patch or is 2.4 out of "support" now?

It's in "semisupport", so I'd merge a patch for that - but it's not
clear if we ever do a formal 2.4.13 release, and even if we do, we are
*not* going to a full windows installer release (so, just tag + tarball,
if needed).

2.3, no :-)  (I do have 2.2 and 2.3 test clients for the "master" test,
but actually getting these to build on modern systems with recent OpenSSL is
painful...)

gert
diff mbox series

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index c48048a1..65a822e6 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1548,19 +1548,6 @@  initialization_sequence_completed(struct context *c, const unsigned int flags)
     /* If we delayed UID/GID downgrade or chroot, do it now */
     do_uid_gid_chroot(c, true);
 
-
-    /*
-     * In some cases (i.e. when receiving auth-token via
-     * push-reply) the auth-nocache option configured on the
-     * client is overridden; for this reason we have to wait
-     * for the push-reply message before attempting to wipe
-     * the user/pass entered by the user
-     */
-    if (c->options.mode == MODE_POINT_TO_POINT)
-    {
-        ssl_clean_user_pass();
-    }
-
     /* Test if errors */
     if (flags & ISC_ERRORS)
     {
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 50f7f975..d78106cd 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -504,19 +504,13 @@  set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token)
          * --auth-token has no username, so it needs the username
          * either already set or copied from up, or later set by
          * --auth-token-user
-         *
-         * Do not overwrite the username if already set to avoid
-         * overwriting an username set by --auth-token-user
+         * If already set, tk is fully defined.
          */
-        if (up->defined && !tk->defined)
+        if (strlen(tk->username))
         {
-            strncpynt(tk->username, up->username, USER_PASS_LEN);
             tk->defined = true;
         }
     }
-
-    /* Cleans user/pass for nocache */
-    purge_user_pass(up, false);
 }
 
 void
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 4c0d78a1..765861b1 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2180,20 +2180,13 @@  key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi
         {
             goto error;
         }
-        /* if auth-nocache was specified, the auth_user_pass object reaches
-         * a "complete" state only after having received the push-reply
-         * message. The push message might contain an auth-token that needs
-         * the username of auth_user_pass.
-         *
-         * For this reason, skip the purge operation here if no push-reply
-         * message has been received yet.
-         *
-         * This normally happens upon first negotiation only.
-         */
-        if (!session->opt->pull)
+        /* save username for auth-token which may get pushed later */
+        if (session->opt->pull)
         {
-            purge_user_pass(&auth_user_pass, false);
+            strncpynt(auth_token.username, up->username, USER_PASS_LEN);
         }
+        /* respect auth-nocache */
+        purge_user_pass(&auth_user_pass, false);
     }
     else
     {
@@ -4092,9 +4085,3 @@  print_data:
 done:
     return BSTR(&out);
 }
-
-void
-ssl_clean_user_pass(void)
-{
-    purge_user_pass(&auth_user_pass, false);
-}
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 9ae6ae8f..96de9ccc 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -538,12 +538,6 @@  void extract_x509_field_test(void);
  */
 bool is_hard_reset_method2(int op);
 
-/**
- * Cleans the saved user/password unless auth-nocache is in use.
- */
-void ssl_clean_user_pass(void);
-
-
 /*
  * Show the TLS ciphers that are available for us to use in the SSL
  * library with headers hinting their usage and warnings about usage.