From: Arne Schwabe Date: Mon, 15 Jun 2026 18:35:07 +0000 (+0200) Subject: Clean up metadata handling in tls_crypt_v2_extract_client_key X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bc5ec987eee8fee290f27dd22fb0672ec9ea2001;p=thirdparty%2Fopenvpn.git Clean up metadata handling in tls_crypt_v2_extract_client_key This makes the metadata a local variable instead of a member of the wrap_context struct. Also always ensure that this buffer is freed to avoid any leak of the metadata buffer. This touches the check methods. Ensure that they still work as intended by adding unit tests for both script and age checks. CVE: 2026-12932 Github: OpenVPN/openvpn-private-issues#133 Github: OpenVPN/openvpn-private-issues#136 Reported-By: Valton Tahiri Acked-by: MaxF Change-Id: I9b8b8afcc5d4d5914b2008c0efccb309f0d07d4b --- diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index 5822336f8..f90b24f1e 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -482,7 +482,6 @@ tls_wrap_free(struct tls_wrap_ctx *tls_wrap) free_key_ctx_bi(&tls_wrap->opt.key_ctx_bi); } - free_buf(&tls_wrap->tls_crypt_v2_metadata); free_buf(&tls_wrap->work); secure_memzero(&tls_wrap->original_wrap_keydata, sizeof(tls_wrap->original_wrap_keydata)); } diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 6f310a56a..9c90242fa 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -285,7 +285,6 @@ struct tls_wrap_ctx struct key_ctx tls_crypt_v2_server_key; /**< Decrypts client keys */ const struct buffer *tls_crypt_v2_wkc; /**< Wrapped client key, * sent to server */ - struct buffer tls_crypt_v2_metadata; /**< Received from client */ bool cleanup_key_ctx; /**< opt.key_ctx_bi is owned by * this context */ /** original key data to be xored in to the key for dynamic tls-crypt. diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 1805995ed..2922f96fb 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -273,7 +273,6 @@ void free_tls_pre_decrypt_state(struct tls_pre_decrypt_state *state) { free_buf(&state->newbuf); - free_buf(&state->tls_wrap_tmp.tls_crypt_v2_metadata); if (state->tls_wrap_tmp.cleanup_key_ctx) { free_key_ctx_bi(&state->tls_wrap_tmp.opt.key_ctx_bi); diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 8c3d7222e..c188cef13 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -34,6 +34,7 @@ #include "run_command.h" #include "session_id.h" #include "ssl.h" +#include "buffer.h" #include "tls_crypt.h" @@ -520,15 +521,15 @@ error_exit: } static bool -tls_crypt_v2_check_client_key_age(const struct tls_wrap_ctx *ctx, int max_days) +tls_crypt_v2_check_client_key_age(const struct buffer *tls_crypt_v2_metadata, int max_days) { - if (BLENZ(&ctx->tls_crypt_v2_metadata) < 1 + sizeof(int64_t)) + if (BLENZ(tls_crypt_v2_metadata) < 1 + sizeof(int64_t)) { msg(M_WARN, "ERROR: Client key metadata is too small to contain a timestamp."); return false; } - const uint8_t *metadata = ctx->tls_crypt_v2_metadata.data; + const uint8_t *metadata = buf_bptr(tls_crypt_v2_metadata); if (*metadata != TLS_CRYPT_METADATA_TYPE_TIMESTAMP) { msg(M_WARN, "ERROR: Client key does not have a timestamp."); @@ -548,12 +549,14 @@ tls_crypt_v2_check_client_key_age(const struct tls_wrap_ctx *ctx, int max_days) } static bool -tls_crypt_v2_verify_metadata(const struct tls_wrap_ctx *ctx, const struct tls_options *opt) +tls_crypt_v2_verify_metadata(const struct buffer *tls_crypt_v2_metadata, const struct tls_options *opt) { bool ret = false; struct gc_arena gc = gc_new(); const char *tmp_file = NULL; - struct buffer metadata = ctx->tls_crypt_v2_metadata; + + struct buffer metadata = *tls_crypt_v2_metadata; + int metadata_type = buf_read_u8(&metadata); if (metadata_type < 0) { @@ -654,26 +657,24 @@ tls_crypt_v2_extract_client_key(struct buffer *buf, struct tls_wrap_ctx *ctx, return true; } - ctx->tls_crypt_v2_metadata = alloc_buf(TLS_CRYPT_V2_MAX_METADATA_LEN); - if (!tls_crypt_v2_unwrap_client_key(&ctx->original_wrap_keydata, &ctx->tls_crypt_v2_metadata, + struct buffer tls_crypt_v2_metadata = alloc_buf(TLS_CRYPT_V2_MAX_METADATA_LEN); + if (!tls_crypt_v2_unwrap_client_key(&ctx->original_wrap_keydata, &tls_crypt_v2_metadata, wrapped_client_key, &ctx->tls_crypt_v2_server_key)) { msg(D_TLS_ERRORS, "Can not unwrap tls-crypt-v2 client key"); - secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); - return false; + goto error; } - if (opt && opt->tls_crypt_v2_max_age > 0 && !tls_crypt_v2_check_client_key_age(ctx, opt->tls_crypt_v2_max_age)) + if (opt && opt->tls_crypt_v2_max_age > 0 && !tls_crypt_v2_check_client_key_age(&tls_crypt_v2_metadata, opt->tls_crypt_v2_max_age)) { - secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); - return false; + goto error; } - if (opt && opt->tls_crypt_v2_verify_script && !tls_crypt_v2_verify_metadata(ctx, opt)) + if (opt && opt->tls_crypt_v2_verify_script && !tls_crypt_v2_verify_metadata(&tls_crypt_v2_metadata, opt)) { - secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); - return false; + goto error; } + free_buf(&tls_crypt_v2_metadata); /* Load the decrypted key */ ctx->mode = TLS_WRAP_CRYPT; @@ -686,6 +687,10 @@ tls_crypt_v2_extract_client_key(struct buffer *buf, struct tls_wrap_ctx *ctx, ASSERT(buf_inc_len(buf, -(BLEN(&wrapped_client_key)))); return true; +error: + secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); + free_buf(&tls_crypt_v2_metadata); + return false; } void diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c b/tests/unit_tests/openvpn/test_tls_crypt.c index 161ee9086..4044cd79e 100644 --- a/tests/unit_tests/openvpn/test_tls_crypt.c +++ b/tests/unit_tests/openvpn/test_tls_crypt.c @@ -100,6 +100,27 @@ int __wrap_parse_line(const char *line, char **p, const int n, const char *file, const int line_num, msglvl_t msglevel, struct gc_arena *gc) { + if (strcmp(line, "/usr/bin/true") == 0) + { + p[0] = "/usr/bin/true"; + return 1; + } + if (strcmp(line, "/usr/bin/false") == 0) + { + p[0] = "/usr/bin/false"; + return 1; + } + if (strcmp(line, "/bin/true") == 0) + { + p[0] = "/bin/true"; + return 1; + } + if (strcmp(line, "/bin/false") == 0) + { + p[0] = "/bin/false"; + return 1; + } + p[0] = PATH1 PATH2; p[1] = PARAM1; p[2] = PARAM2; @@ -458,6 +479,10 @@ test_tls_crypt_v2_setup(void **state) static int test_tls_crypt_v2_teardown(void **state) { + /* Ensure that if we modified now by using update_time, it is back to the + * 0 state for any other test that relies on it. */ + now = 0; + struct test_tls_crypt_v2_context *ctx = (struct test_tls_crypt_v2_context *)*state; free_key_ctx_bi(&ctx->server_keys); @@ -598,6 +623,149 @@ tls_crypt_v2_wrap_unwrap_dst_too_small(void **state) assert_int_equal(0, BLEN(&ctx->unwrapped_metadata)); } +static struct buffer +create_metadata_days_ago(struct gc_arena *gc, int days_ago) +{ + struct buffer metadata = alloc_buf_gc(1 + sizeof(int64_t), gc); + + assert_int_not_equal(now, 0); + + int64_t create_time = now - (days_ago * 24 * 60 * 60); + assert_true(create_time > 0); + + int64_t timestamp = htonll((uint64_t)create_time); + ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_TIMESTAMP, 1)); + ASSERT(buf_write(&metadata, ×tamp, sizeof(timestamp))); + + return metadata; +} + +/** + * Creates a minimal buffer that allows tls_crypt_v2_extract_client_key + * to extract the client key. This will add metadata with a tls-crypt key + * created with the given days_ago. + */ +static struct buffer +create_client_key_input(struct test_tls_crypt_v2_context *ctx, int days_ago) +{ + struct buffer metadata = create_metadata_days_ago(&ctx->gc, days_ago); + + buf_reset_len(&ctx->wkc); + assert_true(tls_crypt_v2_wrap_client_key(&ctx->wkc, &ctx->client_key2, &metadata, + &ctx->server_keys.encrypt, &ctx->gc)); + + /* add a opcode in front of the key to make it valid to extract */ + struct buffer pseudo_packet = alloc_buf_gc(buf_len(&ctx->wkc) + 1, &ctx->gc); + + buf_write_u8(&pseudo_packet, 0x50); + buf_copy(&pseudo_packet, &ctx->wkc); + + return pseudo_packet; +} + +/** + * Check that unwrapping an invalid key fails as expected and does not + * leave extra memory around (via ASAN) + */ +static void +tls_crypt_v2_wrap_unwrap_invalid(void **state) +{ + struct test_tls_crypt_v2_context *ctx = (struct test_tls_crypt_v2_context *)*state; + + struct tls_wrap_ctx wrap_ctx = { + .mode = TLS_WRAP_CRYPT, + .tls_crypt_v2_server_key = ctx->server_keys.encrypt, + }; + + update_time(); + struct buffer tmp = create_client_key_input(ctx, 12); + + /* Make the wrapped key invalid by flipping a few bits */ + buf_bptr(&tmp)[buf_len(&tmp) - 20] ^= 0x55; + + assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, NULL, true)); + tls_wrap_free(&wrap_ctx); +} + + +static void +tls_crypt_v2_unwrap_checks(void **state) +{ + update_time(); + + struct test_tls_crypt_v2_context *ctx = *state; + + + struct tls_wrap_ctx wrap_ctx = { + .mode = TLS_WRAP_CRYPT, + .tls_crypt_v2_server_key = ctx->server_keys.encrypt, + }; + + struct tls_options tls_options = { + .tls_crypt_v2_max_age = 30 + }; + + + struct buffer tmp = create_client_key_input(ctx, 29); + assert_true(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + tls_wrap_free(&wrap_ctx); + + + /* Use /bin/true as verify script */ + script_security_set(2); + tls_options.tls_crypt_v2_verify_script = "/usr/bin/true"; + /* Some Linux system only have /bin/true */ + if (access(tls_options.tls_crypt_v2_verify_script, X_OK) != 0) + { + tls_options.tls_crypt_v2_verify_script = "/bin/true"; + } + + tls_options.tmp_dir = "/tmp"; + + /* Since we override rand_bytes the tmpfile name is non-random as well */ + const char *non_random_tmpfile = "/tmp/openvpn_tls_crypt_v2_metadata__706050403020100706050403020100.tmp"; + unlink(non_random_tmpfile); + + expect_string(__wrap_buffer_write_file, filename, non_random_tmpfile); + + /* We do not write the first byte (type) to the file but rather to a + * metadata_type environment variable */ + struct buffer expected_metadata = create_metadata_days_ago(&ctx->gc, 29); + buf_advance(&expected_metadata, 1); + + expect_memory(__wrap_buffer_write_file, pem, buf_bptr(&expected_metadata), buf_len(&expected_metadata)); + will_return(__wrap_buffer_write_file, true); + + tmp = create_client_key_input(ctx, 29); + assert_true(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + tls_wrap_free(&wrap_ctx); + + tls_options.tls_crypt_v2_verify_script = "/usr/bin/false"; + /* Some Linux system only have /bin/false */ + if (access(tls_options.tls_crypt_v2_verify_script, X_OK) != 0) + { + tls_options.tls_crypt_v2_verify_script = "/bin/false"; + } + + expect_string(__wrap_buffer_write_file, filename, non_random_tmpfile); + expect_memory(__wrap_buffer_write_file, pem, buf_bptr(&expected_metadata), buf_len(&expected_metadata)); + will_return(__wrap_buffer_write_file, true); + + tmp = create_client_key_input(ctx, 29); + assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + tls_wrap_free(&wrap_ctx); + + + tls_options.tls_crypt_v2_verify_script = NULL; + + /* An outdated time should fail */ + tmp = create_client_key_input(ctx, 31); + assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + + script_security_set(0); + tls_wrap_free(&wrap_ctx); +} + static void test_tls_crypt_v2_write_server_key_file(void **state) { @@ -675,6 +843,10 @@ main(void) test_tls_crypt_v2_teardown), cmocka_unit_test_setup_teardown(tls_crypt_v2_wrap_unwrap_dst_too_small, test_tls_crypt_v2_setup, test_tls_crypt_v2_teardown), + cmocka_unit_test_setup_teardown(tls_crypt_v2_wrap_unwrap_invalid, + test_tls_crypt_v2_setup, test_tls_crypt_v2_teardown), + cmocka_unit_test_setup_teardown(tls_crypt_v2_unwrap_checks, + test_tls_crypt_v2_setup, test_tls_crypt_v2_teardown), cmocka_unit_test_setup_teardown(test_tls_crypt_secure_reneg_key, test_tls_crypt_setup, test_tls_crypt_teardown), cmocka_unit_test(test_tls_crypt_v2_write_server_key_file),