[Openvpn-devel] [M] Change in openvpn[master]: Implement support for AEAD tag at the end

Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/506?usp=email to review the following change. Change subject: Implement support for AEAD tag at the end ...................................................................... Implement support for AEAD tag at the end Change-Id: I00821d75342daf3f813b829812d648fe298bea81 --- M src/openvpn/crypto.c M src/openvpn/crypto.h M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/push.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/test_ssl.c 7 files changed, 85 insertions(+), 26 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/06/506/1 diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c index 2fca131..9988ebe 100644 --- a/src/openvpn/crypto.c +++ b/src/openvpn/crypto.c @@ -104,14 +104,10 @@ ASSERT(cipher_ctx_reset(ctx->cipher, iv)); } - /* Reserve space for authentication tag */ - mac_out = buf_write_alloc(&work, mac_len); - ASSERT(mac_out); - dmsg(D_PACKET_CONTENT, "ENCRYPT FROM: %s", format_hex(BPTR(buf), BLEN(buf), 80, &gc)); /* Buffer overflow check */ - if (!buf_safe(&work, buf->len + cipher_ctx_block_size(ctx->cipher))) + if (!buf_safe(&work, buf->len + mac_len + cipher_ctx_block_size(ctx->cipher))) { msg(D_CRYPT_ERRORS, "ENCRYPT: buffer size error, bc=%d bo=%d bl=%d wc=%d wo=%d wl=%d", @@ -121,9 +117,16 @@ } /* For AEAD ciphers, authenticate Additional Data, including opcode */ - ASSERT(cipher_ctx_update_ad(ctx->cipher, BPTR(&work), BLEN(&work) - mac_len)); + ASSERT(cipher_ctx_update_ad(ctx->cipher, BPTR(&work), BLEN(&work))); dmsg(D_PACKET_CONTENT, "ENCRYPT AD: %s", - format_hex(BPTR(&work), BLEN(&work) - mac_len, 0, &gc)); + format_hex(BPTR(&work), BLEN(&work), 0, &gc)); + + if (!(opt->flags & CO_AEAD_TAG_AT_THE_END)) + { + /* Reserve space for authentication tag */ + mac_out = buf_write_alloc(&work, mac_len); + ASSERT(mac_out); + } /* Encrypt packet ID, payload */ ASSERT(cipher_ctx_update(ctx->cipher, BEND(&work), &outlen, BPTR(buf), BLEN(buf))); @@ -133,6 +136,14 @@ ASSERT(cipher_ctx_final(ctx->cipher, BEND(&work), &outlen)); ASSERT(buf_inc_len(&work, outlen)); + /* if the tag is at end the end, allocate it now */ + if (opt->flags & CO_AEAD_TAG_AT_THE_END) + { + /* Reserve space for authentication tag */ + mac_out = buf_write_alloc(&work, mac_len); + ASSERT(mac_out); + } + /* Write authentication tag */ ASSERT(cipher_ctx_get_tag(ctx->cipher, mac_out, mac_len)); @@ -353,7 +364,6 @@ static const char error_prefix[] = "AEAD Decrypt error"; struct packet_id_net pin = { 0 }; const struct key_ctx *ctx = &opt->key_ctx_bi.decrypt; - uint8_t *tag_ptr = NULL; int outlen; struct gc_arena gc; @@ -406,19 +416,29 @@ /* keep the tag value to feed in later */ const int tag_size = OPENVPN_AEAD_TAG_LENGTH; - if (buf->len < tag_size) + if (buf->len < tag_size + 1) { - CRYPT_ERROR("missing tag"); + CRYPT_ERROR("missing tag or no payload"); } - tag_ptr = BPTR(buf); - ASSERT(buf_advance(buf, tag_size)); + + const int ad_size = BPTR(buf) - ad_start; + + uint8_t *tag_ptr = NULL; + int data_len = 0; + + if (opt->flags & CO_AEAD_TAG_AT_THE_END) + { + data_len = BLEN(buf) - tag_size; + tag_ptr = BPTR(buf) + data_len; + } + else + { + tag_ptr = BPTR(buf); + ASSERT(buf_advance(buf, tag_size)); + data_len = BLEN(buf); + } + dmsg(D_PACKET_CONTENT, "DECRYPT MAC: %s", format_hex(tag_ptr, tag_size, 0, &gc)); - - if (buf->len < 1) - { - CRYPT_ERROR("missing payload"); - } - dmsg(D_PACKET_CONTENT, "DECRYPT FROM: %s", format_hex(BPTR(buf), BLEN(buf), 0, &gc)); /* Buffer overflow check (should never fail) */ @@ -427,20 +447,19 @@ CRYPT_ERROR("potential buffer overflow"); } - { - /* feed in tag and the authenticated data */ - const int ad_size = BPTR(buf) - ad_start - tag_size; - ASSERT(cipher_ctx_update_ad(ctx->cipher, ad_start, ad_size)); - dmsg(D_PACKET_CONTENT, "DECRYPT AD: %s", - format_hex(BPTR(buf) - ad_size - tag_size, ad_size, 0, &gc)); - } + + /* feed in tag and the authenticated data */ + ASSERT(cipher_ctx_update_ad(ctx->cipher, ad_start, ad_size)); + dmsg(D_PACKET_CONTENT, "DECRYPT AD: %s", + format_hex(ad_start, ad_size, 0, &gc)); /* Decrypt and authenticate packet */ if (!cipher_ctx_update(ctx->cipher, BPTR(&work), &outlen, BPTR(buf), - BLEN(buf))) + data_len)) { CRYPT_ERROR("cipher update failed"); } + ASSERT(buf_inc_len(&work, outlen)); if (!cipher_ctx_final_check_tag(ctx->cipher, BPTR(&work) + outlen, &outlen, tag_ptr, tag_size)) diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h index 4201524..95a5b31 100644 --- a/src/openvpn/crypto.h +++ b/src/openvpn/crypto.h @@ -279,6 +279,10 @@ /**< Bit-flag indicating that renegotiations are using tls-crypt * with a TLS-EKM derived key. */ +#define CO_AEAD_TAG_AT_THE_END (1<<8) + /**< Bit-flag indicating that the AEAD tag is at the end of the + * packet. + */ unsigned int flags; /**< Bit-flags determining behavior of * security operation functions. */ diff --git a/src/openvpn/init.c b/src/openvpn/init.c index c5cc154..cd37b36 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2323,6 +2323,10 @@ { buf_printf(&out, " dyn-tls-crypt"); } + if (o->imported_protocol_flags & CO_AEAD_TAG_AT_THE_END) + { + buf_printf(&out, " aead-tag-end"); + } } if (buf_len(&out) > strlen(header)) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 2c79a1e..39f00c0 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -8686,6 +8686,10 @@ options->imported_protocol_flags |= CO_USE_DYNAMIC_TLS_CRYPT; } #endif + else if (streq(p[j], "aead-tag-end")) + { + options->imported_protocol_flags |= CO_AEAD_TAG_AT_THE_END; + } else { msg(msglevel, "Unknown protocol-flags flag: %s", p[j]); diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 2249434..e4c122c 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -29,6 +29,7 @@ #include "push.h" #include "options.h" +#include "crypto.h" #include "ssl.h" #include "ssl_verify.h" #include "ssl_ncp.h" @@ -686,6 +687,11 @@ buf_printf(&proto_flags, " dyn-tls-crypt"); } + if (o->imported_protocol_flags & CO_AEAD_TAG_AT_THE_END) + { + buf_printf(&proto_flags, " aead-tag-end"); + } + if (buf_len(&proto_flags) > 0) { push_option_fmt(gc, push_list, M_USAGE, "protocol-flags%s", buf_str(&proto_flags)); diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index 71b99db..a1c67a2 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -107,6 +107,9 @@ /** Support to dynamic tls-crypt (renegotiation with TLS-EKM derived tls-crypt key) */ #define IV_PROTO_DYN_TLS_CRYPT (1<<9) +/** Suport for the AEAD tag at the end a larger peer id and IV */ +#define IV_PROTO_DATA_V3 (1<<10) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/test_ssl.c b/tests/unit_tests/openvpn/test_ssl.c index 8c1fb5b..0ded052 100644 --- a/tests/unit_tests/openvpn/test_ssl.c +++ b/tests/unit_tests/openvpn/test_ssl.c @@ -266,6 +266,19 @@ } +/* This adds a few more methods that strictly necessary but this allows + * us to see which exact test was run from the backtrace of the test + * when it fails */ + +static void +run_data_channel_with_cipher_end(const char *cipher) +{ + struct crypto_options co = init_crypto_options(cipher, "none"); + co.flags |= CO_AEAD_TAG_AT_THE_END; + do_data_channel_round_trip(&co); + uninit_crypto_options(&co); +} + static void run_data_channel_with_cipher(const char *cipher, const char *auth) { @@ -274,21 +287,25 @@ uninit_crypto_options(&co); } + static void test_data_channel_roundtrip_aes_128_gcm(void **state) { + run_data_channel_with_cipher_end("AES-128-GCM"); run_data_channel_with_cipher("AES-128-GCM", "none"); } static void test_data_channel_roundtrip_aes_192_gcm(void **state) { + run_data_channel_with_cipher_end("AES-192-GCM"); run_data_channel_with_cipher("AES-192-GCM", "none"); } static void test_data_channel_roundtrip_aes_256_gcm(void **state) { + run_data_channel_with_cipher_end("AES-256-GCM"); run_data_channel_with_cipher("AES-256-GCM", "none"); } @@ -318,6 +335,8 @@ skip(); return; } + + run_data_channel_with_cipher_end("ChaCha20-Poly1305"); run_data_channel_with_cipher("ChaCha20-Poly1305", "none"); } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/506?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I00821d75342daf3f813b829812d648fe298bea81 Gerrit-Change-Number: 506 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange