auth_token: Clean up type handling in verify_auth_token and its UT

First of all remove the testing of renegotiation_seconds.
Commit 9a5161704173e31f2510d3f5c29361f76e275d0f made it
irrelevant for verify_auth_token but still left UTs for it.
But AFAICT these UTs only test that renegotiation_seconds
is bigger than auth_token_renewal, so it tests the UT
setup routine...

Also improve the code to require less casts under
-Wsign-compare.

Add a comment that this code is not y38 safe if time_t
is 32bit. Probably nothing we want to do from our side
since in that case everything that uses "now" is borked.
So we trust in the OS here...

Change-Id: I73dba29719ea685f0427a3c479e7f1f176f09eba
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1510
Message-Id: <20260312173144.15602-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg36079.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index eb2b4d5ff7bf72ae1e1dc56f7bae7516961bbbf3..d8ca125231d946179ee6d3bdd8a41d6b6ca8add7 100644 (file)
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -287,11 +287,6 @@ check_hmac_token(hmac_ctx_t *ctx, const uint8_t *b64decoded, const char *usernam
     return memcmp_constant_time(&hmac_output, hmac, 32) == 0;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wsign-compare"
-#endif
-
 unsigned int
 verify_auth_token(struct user_pass *up, struct tls_multi *multi, struct tls_session *session)
 {
@@ -318,7 +313,7 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi, struct tls_sess
 
     const uint8_t *sessid = b64decoded;
     const uint8_t *tstamp_initial = sessid + AUTH_TOKEN_SESSION_ID_LEN;
-    const uint8_t *tstamp = tstamp_initial + sizeof(int64_t);
+    const uint8_t *tstamp = tstamp_initial + sizeof(uint64_t);
 
     /* tstamp, tstamp_initial might not be aligned to an uint64, use memcpy
      * to avoid unaligned access */
@@ -348,9 +343,11 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi, struct tls_sess
     }
 
     /* 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->auth_token_renewal;
+     * for renegotiations.
+     * Cast is required for systems with 32bit time_t, e.g. Windows x86.
+     */
+    const time_t token_reneg_deadline = (time_t)timestamp + 2 * session->opt->auth_token_renewal;
+    bool in_renegotiation_time = now >= (time_t)timestamp && now < token_reneg_deadline;
 
     if (!in_renegotiation_time)
     {
@@ -369,7 +366,8 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi, struct tls_sess
         ret |= AUTH_TOKEN_EXPIRED;
     }
 
-    if (multi->opt.auth_token_lifetime && now > timestamp_initial + multi->opt.auth_token_lifetime)
+    const time_t token_eol = (time_t)timestamp_initial + multi->opt.auth_token_lifetime;
+    if (multi->opt.auth_token_lifetime && now > token_eol)
     {
         ret |= AUTH_TOKEN_EXPIRED;
     }
@@ -396,10 +394,6 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi, struct tls_sess
     return ret;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 void
 wipe_auth_token(struct tls_multi *multi)
 {

diff --git a/tests/unit_tests/openvpn/test_auth_token.c b/tests/unit_tests/openvpn/test_auth_token.c
index d28bd8a9532716021487b4a8a3cd9da92c7f11fd..c7ba5fd825a30f062f06a709a9bd6b369a92ba97 100644 (file)
--- a/tests/unit_tests/openvpn/test_auth_token.c
+++ b/tests/unit_tests/openvpn/test_auth_token.c
@@ -166,20 +166,19 @@ auth_token_fail_invalid_key(void **state)
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), AUTH_TOKEN_HMAC_OK);
 }
 
-/* Note: only on 32bit Windows builds */
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wsign-compare"
-#endif
-
 static void
 auth_token_test_timeout(void **state)
 {
     struct test_context *ctx = (struct test_context *)*state;
 
-    now = 100000;
+    const time_t initial_time = 100000;
+    now = initial_time;
     generate_auth_token(&ctx->up, &ctx->multi);
 
+    const time_t token_renew_window =
+        initial_time + 2 * ctx->session->opt->auth_token_renewal;
+    const time_t token_eol = initial_time + ctx->session->opt->auth_token_lifetime + 1;
+
     strcpy(ctx->up.password, ctx->multi.auth_token);
     free(ctx->multi.auth_token_initial);
     ctx->multi.auth_token_initial = NULL;
@@ -188,26 +187,21 @@ auth_token_test_timeout(void **state)
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), AUTH_TOKEN_HMAC_OK);
 
     /* Token before validity, should be rejected */
-    now = 100000 - 100;
+    now = initial_time - 100;
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK | AUTH_TOKEN_EXPIRED);
 
-    /* Token no valid for renegotiate_seconds but still for renewal_time */
-    now = 100000 + 2 * ctx->session->opt->renegotiate_seconds - 20;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
-                     AUTH_TOKEN_HMAC_OK | AUTH_TOKEN_EXPIRED);
-
-
-    now = 100000 + 2 * ctx->session->opt->auth_token_renewal - 20;
+    /* Token still valid for renewal_time */
+    now = token_renew_window - 20;
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), AUTH_TOKEN_HMAC_OK);
 
     /* Token past validity, should be rejected */
-    now = 100000 + 2 * ctx->session->opt->renegotiate_seconds + 20;
+    now = token_renew_window + 20;
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK | AUTH_TOKEN_EXPIRED);
 
-    /* But not when we reached our timeout */
-    now = 100000 + ctx->session->opt->auth_token_lifetime + 1;
+    /* Token past lifetime, should be rejected */
+    now = token_eol;
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK | AUTH_TOKEN_EXPIRED);
 
@@ -215,8 +209,8 @@ auth_token_test_timeout(void **state)
     ctx->multi.auth_token_initial = NULL;
 
     /* regenerate the token util it hits the expiry */
-    now = 100000;
-    while (now < 100000 + ctx->session->opt->auth_token_lifetime + 1)
+    now = initial_time;
+    while (now < token_eol)
     {
         assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                          AUTH_TOKEN_HMAC_OK);
@@ -234,10 +228,6 @@ auth_token_test_timeout(void **state)
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), AUTH_TOKEN_HMAC_OK);
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 static void
 zerohmac(char *token)
 {