> On 7 Jun 2023, at 09:44, Roman Arutyunyan <arut@nginx.com> wrote:
>
> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1686116578 -14400
> # Wed Jun 07 09:42:58 2023 +0400
> # Node ID 931f7f2b2aab96ca6783d42a6dc4d1150b0d45ee
> # Parent b4a57278bf24dd28d39afea0eb09732c05bf1606
> QUIC: use AEAD cipher to encrypt address validation tokens.
>
> Previously used AES256-CBC is now substituted with AES256-GCM. Although there
> seem to be no tangible consequences of token integrity loss.
>

For the record.
With the currently used AES-CBC it should not be feasible to generate
a valid token for an attacker: to pass token validation, attacker's
address is required to match the checksum value stored in the encrypted
token, which effectively would mean to gain token encryption keys.
What remains is breaking integrity by corrupting certain token parts.

> diff --git a/src/event/quic/ngx_event_quic_tokens.c b/src/event/quic/ngx_event_quic_tokens.c
> --- a/src/event/quic/ngx_event_quic_tokens.c
> +++ b/src/event/quic/ngx_event_quic_tokens.c
> @@ -69,11 +69,10 @@ ngx_quic_new_token(ngx_log_t *log, struc
>
> len = p - in;
>
> - cipher = EVP_aes_256_cbc();
> - iv_len = NGX_QUIC_AES_256_CBC_IV_LEN;
> + cipher = EVP_aes_256_gcm();
> + iv_len = NGX_QUIC_AES_256_GCM_IV_LEN;
>
> - if ((size_t) (iv_len + len + NGX_QUIC_AES_256_CBC_BLOCK_SIZE) > token->len)
> - {
> + if ((size_t) (iv_len + len + NGX_QUIC_AES_256_GCM_TAG_LEN) > token->len) {
> ngx_log_error(NGX_LOG_ALERT, log, 0, "quic token buffer is too small");
> return NGX_ERROR;
> }
> @@ -108,6 +107,17 @@ ngx_quic_new_token(ngx_log_t *log, struc
>
> token->len += len;
>
> + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG,
> + NGX_QUIC_AES_256_GCM_TAG_LEN,
> + token->data + token->len)
> + == 0)
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + return NGX_ERROR;
> + }
> +
> + token->len += NGX_QUIC_AES_256_GCM_TAG_LEN;
> +
> EVP_CIPHER_CTX_free(ctx);
>
> #ifdef NGX_QUIC_DEBUG_PACKETS
> @@ -184,17 +194,19 @@ ngx_quic_validate_token(ngx_connection_t
>
> /* Retry token or NEW_TOKEN in a previous connection */
>
> - cipher = EVP_aes_256_cbc();
> + cipher = EVP_aes_256_gcm();
> iv = pkt->token.data;
> - iv_len = NGX_QUIC_AES_256_CBC_IV_LEN;
> + iv_len = NGX_QUIC_AES_256_GCM_IV_LEN;
>
> /* sanity checks */
>
> - if (pkt->token.len < (size_t) iv_len + NGX_QUIC_AES_256_CBC_BLOCK_SIZE) {
> + if (pkt->token.len < (size_t) iv_len + NGX_QUIC_AES_256_GCM_TAG_LEN) {
> goto garbage;
> }
>
> - if (pkt->token.len > (size_t) iv_len + NGX_QUIC_MAX_TOKEN_SIZE) {
> + if (pkt->token.len > (size_t) iv_len + NGX_QUIC_MAX_TOKEN_SIZE
> + + NGX_QUIC_AES_256_GCM_TAG_LEN)
> + {
> goto garbage;
> }
>
> @@ -209,15 +221,23 @@ ngx_quic_validate_token(ngx_connection_t
> }
>
> p = pkt->token.data + iv_len;
> - len = pkt->token.len - iv_len;
> + len = pkt->token.len - iv_len - NGX_QUIC_AES_256_GCM_TAG_LEN;
>
> - if (EVP_DecryptUpdate(ctx, tdec, &len, p, len) != 1) {
> + if (EVP_DecryptUpdate(ctx, tdec, &tlen, p, len) != 1) {
> EVP_CIPHER_CTX_free(ctx);
> goto garbage;
> }
> - total = len;
> + total = tlen;
>
> - if (EVP_DecryptFinal_ex(ctx, tdec + len, &tlen) <= 0) {
> + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG,
> + NGX_QUIC_AES_256_GCM_TAG_LEN, p + len)
> + == 0)
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + goto garbage;
> + }
> +
> + if (EVP_DecryptFinal_ex(ctx, tdec + tlen, &tlen) <= 0) {
> EVP_CIPHER_CTX_free(ctx);
> goto garbage;
> }
> diff --git a/src/event/quic/ngx_event_quic_tokens.h b/src/event/quic/ngx_event_quic_tokens.h
> --- a/src/event/quic/ngx_event_quic_tokens.h
> +++ b/src/event/quic/ngx_event_quic_tokens.h
> @@ -15,13 +15,12 @@
> #define NGX_QUIC_MAX_TOKEN_SIZE 64
> /* SHA-1(addr)=20 + sizeof(time_t) + retry(1) + odcid.len(1) + odcid */
>
> -/* RFC 3602, 2.1 and 2.4 for AES-CBC block size and IV length */
> -#define NGX_QUIC_AES_256_CBC_IV_LEN 16
> -#define NGX_QUIC_AES_256_CBC_BLOCK_SIZE 16
> +#define NGX_QUIC_AES_256_GCM_IV_LEN 12
> +#define NGX_QUIC_AES_256_GCM_TAG_LEN 16

JFTR: although NGX_QUIC_TAG_LEN / NGX_QUIC_IV_LEN could be reused,
I think it's fine to take separate tag/iv macros, since token
encryption algorithm isn't related to TLS cipher suites.

>
> -#define NGX_QUIC_TOKEN_BUF_SIZE (NGX_QUIC_AES_256_CBC_IV_LEN \
> +#define NGX_QUIC_TOKEN_BUF_SIZE (NGX_QUIC_AES_256_GCM_IV_LEN \
> + NGX_QUIC_MAX_TOKEN_SIZE \
> - + NGX_QUIC_AES_256_CBC_BLOCK_SIZE)
> + + NGX_QUIC_AES_256_GCM_TAG_LEN)
>
>
> ngx_int_t ngx_quic_new_sr_token(ngx_connection_t *c, ngx_str_t *cid,

Patch looks good.

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel