cron2 has uploaded a new patch set (#9) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/1510?usp=email )
The following approvals got outdated and were removed: Code-Review+2 by plaisthos 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]> Acked-by: Arne Schwabe <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1510 Message-Id: <[email protected]> URL: https://www.mail-archive.com/[email protected]/msg36079.html Signed-off-by: Gert Doering <[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/9 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 d28bd8a..c7ba5fd 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: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I73dba29719ea685f0427a3c479e7f1f176f09eba Gerrit-Change-Number: 1510 Gerrit-PatchSet: 9 Gerrit-Owner: flichtenheld <[email protected]> Gerrit-Reviewer: plaisthos <[email protected]> Gerrit-CC: openvpn-devel <[email protected]>
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
