Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1510?usp=email
to review the following change.
Change subject: auth_token: Clean up type handling in verify_auth_token and its
UT
......................................................................
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 <[email protected]>
---
M src/openvpn/auth_token.c
M tests/unit_tests/openvpn/test_auth_token.c
2 files changed, 22 insertions(+), 38 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/10/1510/1
diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index eb2b4d5..d8ca125 100644
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -287,11 +287,6 @@
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 @@
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 @@
}
/* 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 @@
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 @@
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 82c20c1..30d55f6 100644
--- a/tests/unit_tests/openvpn/test_auth_token.c
+++ b/tests/unit_tests/openvpn/test_auth_token.c
@@ -166,20 +166,19 @@
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 @@
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 @@
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 @@
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)
{
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1510?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I73dba29719ea685f0427a3c479e7f1f176f09eba
Gerrit-Change-Number: 1510
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
