auth-token: Ensure tokens are always wiped on de-auth

If tls_deauthenticate() was called, it could in some scenarios leave the authentication token for a session in memory. This change just ensures auth-tokens are always wiped as soon as a TLS session is considered broken.

Signed-off-by David Sommerseth <[email protected]...>

---

The wipe_auth_token() function is otherwise moved to be declared before tls_deauthenticate() and the latter function is also slightly modified to make use of the C99 feature of inline declaration - mostly to have a more reasonable coding style when adding the wipe_auth_token() call.

---
 src/openvpn/ssl_verify.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

src/openvpn/ssl_verify.c +1 -0

index a6e9be3..008a60d 100644
@@ -80,6 +80,26 @@ setenv_untrusted(struct tls_session *session)
     setenv_link_socket_actual(session->opt->es, "untrusted", &session->untrusted_addr, SA_IP_PORT);
 }
 
+

src/openvpn/ssl_verify.c +13 -19

+	    secure_memzero(multi->auth_token, AUTH_TOKEN_SIZE);
+	    free(multi->auth_token);
+	}
+	multi->auth_token = NULL;
+	multi->auth_token_sent = false;
+    }
+}
+
+
 /*
  * Remove authenticated state from all sessions in the given tunnel
  */

@@ -88,10 +108,10 @@ tls_deauthenticate(struct tls_multi *multi)
 {
     if (multi)
     {
-        int i, j;
-        for (i = 0; i < TM_SIZE; ++i)
+        wipe_auth_token(multi);
+        for (int i = 0; i < TM_SIZE; ++i)
         {
-            for (j = 0; j < KS_SIZE; ++j)
+            for (int j = 0; j < KS_SIZE; ++j)
             {
                 multi->session[i].key[j].authenticated = false;
             }

@@ -1219,21 +1239,6 @@ verify_user_pass_management(struct tls_session *session, const struct user_pass
 }
 #endif /* ifdef MANAGEMENT_DEF_AUTH */
 
-/**
- *  Wipes the authentication token out of the memory, frees and cleans up related buffers and flags
- *
- *  @param multi  Pointer to a multi object holding the auth_token variables
- */
-static void
-wipe_auth_token(struct tls_multi *multi)
-{
-    secure_memzero(multi->auth_token, AUTH_TOKEN_SIZE);
-    free(multi->auth_token);
-    multi->auth_token = NULL;
-    multi->auth_token_sent = false;
-}
-
-
 /*
  * Main username/password verification entry point
  */

@@ -1285,7 +1290,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
         /* Ensure that the username has not changed */
         if (!tls_lock_username(multi, up->username))
         {
-            wipe_auth_token(multi);
+            /* auth-token cleared in tls_lock_username() on failure */
             ks->authenticated = false;
             goto done;
         }

Hi,

Quoted 17 lines · David Sommerseth

If tls_deauthenticate() was called, it could in some scenarios leave the authentication token for a session in memory. This change just ensures auth-tokens are always wiped as soon as a TLS session is considered broken.

Signed-off-by David Sommerseth <[email protected]...>

---

The wipe_auth_token() function is otherwise moved to be declared before tls_deauthenticate() and the latter function is also slightly modified to make use of the C99 feature of inline declaration - mostly to have a more reasonable coding style when adding the wipe_auth_token() call.

---
 src/openvpn/ssl_verify.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

Quoted 5 lines

src/openvpn/ssl_verify.c +5 -0

+
+/**
+ *  Wipes the authentication token out of the memory, frees and cleans up related buffers and flags
+ *
+ *  @param multi  Pointer to a multi object holding the auth_token variables

src/openvpn/ssl_verify.c setenv_untrusted(struct tls_session *session) · line 93

Quoted 6 lines

+ */

src/openvpn/ssl_verify.c +5 -0

+static void
+wipe_auth_token(struct tls_multi *multi)
+{
+    if( multi ) {
+	if (multi->auth_token ) {

The spaces in theses lines look a bit odd.

Shouldn't we also clear the token if the ccd authentication check in verify_final_auth_checks() fails?

If tls_deauthenticate() was called, it could in some scenarios leave the authentication token for a session in memory. This change just ensures auth-tokens are always wiped as soon as a TLS session is considered broken.

Signed-off-by David Sommerseth <[email protected]...>

---

The wipe_auth_token() function is otherwise moved to be declared before tls_deauthenticate() and the latter function is also slightly modified to make use of the C99 feature of inline declaration - mostly to have a more reasonable coding style when adding the wipe_auth_token() call.

v2 - Fixed coding style issues after changes in wipe_auth_token()
- Added wipe_auth_token() call when --ccd-exclusive auth fails

---
 src/openvpn/ssl_verify.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

src/openvpn/ssl_verify.c +3 -0

index a6e9be3..bc578fa 100644
@@ -80,6 +80,28 @@ setenv_untrusted(struct tls_session *session)
     setenv_link_socket_actual(session->opt->es, "untrusted", &session->untrusted_addr, SA_IP_PORT);
 }
 
+
+/**
+ *  Wipes the authentication token out of the memory, frees and cleans up related buffers and flags

src/openvpn/ssl_verify.c +13 -20

+            secure_memzero(multi->auth_token, AUTH_TOKEN_SIZE);
+            free(multi->auth_token);
+        }
+        multi->auth_token = NULL;
+        multi->auth_token_sent = false;
+    }
+}
+
+
 /*
  * Remove authenticated state from all sessions in the given tunnel
  */

@@ -88,10 +110,10 @@ tls_deauthenticate(struct tls_multi *multi)
 {
     if (multi)
     {
-        int i, j;
-        for (i = 0; i < TM_SIZE; ++i)
+        wipe_auth_token(multi);
+        for (int i = 0; i < TM_SIZE; ++i)
         {
-            for (j = 0; j < KS_SIZE; ++j)
+            for (int j = 0; j < KS_SIZE; ++j)
             {
                 multi->session[i].key[j].authenticated = false;
             }

@@ -1219,21 +1241,6 @@ verify_user_pass_management(struct tls_session *session, const struct user_pass
 }
 #endif /* ifdef MANAGEMENT_DEF_AUTH */
 
-/**
- *  Wipes the authentication token out of the memory, frees and cleans up related buffers and flags
- *
- *  @param multi  Pointer to a multi object holding the auth_token variables
- */
-static void
-wipe_auth_token(struct tls_multi *multi)
-{
-    secure_memzero(multi->auth_token, AUTH_TOKEN_SIZE);
-    free(multi->auth_token);
-    multi->auth_token = NULL;
-    multi->auth_token_sent = false;
-}
-
-
 /*
  * Main username/password verification entry point
  */

@@ -1285,7 +1292,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
         /* Ensure that the username has not changed */
         if (!tls_lock_username(multi, up->username))
         {
-            wipe_auth_token(multi);
+            /* auth-token cleared in tls_lock_username() on failure */
             ks->authenticated = false;
             goto done;
         }

@@ -1306,7 +1313,6 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
         if (memcmp_constant_time(multi->auth_token, up->password,
                                  strlen(multi->auth_token)) != 0)
         {
-            wipe_auth_token(multi);
             ks->authenticated = false;
             tls_deauthenticate(multi);