On Mon, Feb 21, 2022 at 02:10:34PM +0300, Vladimir Homutov wrote:
> Patch subject is complete summary.
>
>
> src/event/quic/ngx_event_quic_output.c | 11 +++++++++--
> src/event/quic/ngx_event_quic_tokens.c | 22 ++++------------------
> src/event/quic/ngx_event_quic_tokens.h | 14 +++++++++++++-
> src/event/quic/ngx_event_quic_transport.h | 1 +
> 4 files changed, 27 insertions(+), 21 deletions(-)
>
>

> # HG changeset patch
> # User Vladimir Homutov <vl@nginx.com>
> # Date 1645440587 -10800
> # Mon Feb 21 13:49:47 2022 +0300
> # Branch quic
> # Node ID b3fb81ecc3431c4dbf9e849d72d13a84fe02703b
> # Parent dfc2fc335990e05da1a6f087ca75721cbf8c8891
> QUIC: avoided pool usage in token calculation.
>
> diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> --- a/src/event/quic/ngx_event_quic_output.c
> +++ b/src/event/quic/ngx_event_quic_output.c
> @@ -1009,10 +1009,13 @@ ngx_quic_send_retry(ngx_connection_t *c,
>
> u_char buf[NGX_QUIC_RETRY_BUFFER_SIZE];
> u_char dcid[NGX_QUIC_SERVER_CID_LEN];
> + u_char tbuf[NGX_QUIC_TOKEN_BUF_SIZE];
>
> expires = ngx_time() + NGX_QUIC_RETRY_TOKEN_LIFETIME;
>
> - if (ngx_quic_new_token(c, c->sockaddr, c->socklen, conf->av_token_key,
> + token.data = tbuf;

This part looks fragile to me. I suggest that we assign the available length
to token.len:

token.len = NGX_QUIC_TOKEN_BUF_SIZE;

Then inside ngx_quic_new_token() verify if length is enough.

> +
> + if (ngx_quic_new_token(c->log, c->sockaddr, c->socklen, conf->av_token_key,
> &token, &inpkt->dcid, expires, 1)
> != NGX_OK)
> {
> @@ -1075,11 +1078,15 @@ ngx_quic_send_new_token(ngx_connection_t
> ngx_quic_frame_t *frame;
> ngx_quic_connection_t *qc;
>
> + u_char tbuf[NGX_QUIC_TOKEN_BUF_SIZE];
> +
> qc = ngx_quic_get_connection(c);
>
> expires = ngx_time() + NGX_QUIC_NEW_TOKEN_LIFETIME;
>
> - if (ngx_quic_new_token(c, path->sockaddr, path->socklen,
> + token.data = tbuf;

Same here.

> + if (ngx_quic_new_token(c->log, path->sockaddr, path->socklen,
> qc->conf->av_token_key, &token, NULL, expires, 0)
> != NGX_OK)
> {
> 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
> @@ -11,14 +11,6 @@
> #include <ngx_event_quic_connection.h>
>
>
> -#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
> -
> -
> static void ngx_quic_address_hash(struct sockaddr *sockaddr, socklen_t socklen,
> ngx_uint_t no_port, u_char buf[20]);
>
> @@ -48,7 +40,7 @@ ngx_quic_new_sr_token(ngx_connection_t *
>
>
> ngx_int_t
> -ngx_quic_new_token(ngx_connection_t *c, struct sockaddr *sockaddr,
> +ngx_quic_new_token(ngx_log_t *log, struct sockaddr *sockaddr,
> socklen_t socklen, u_char *key, ngx_str_t *token, ngx_str_t *odcid,
> time_t exp, ngx_uint_t is_retry)
> {
> @@ -81,10 +73,6 @@ ngx_quic_new_token(ngx_connection_t *c,
> iv_len = NGX_QUIC_AES_256_CBC_IV_LEN;
>
> token->len = iv_len + len + NGX_QUIC_AES_256_CBC_BLOCK_SIZE;

Since there's no allocation anymore, this assignment makes no sense.
As I suggested earlier, what could make sense is verifying token->data buffer
is large enough for the token:

if ((size_t) (iv_len + len + NGX_QUIC_AES_256_CBC_BLOCK_SIZE) > token->len)
{
ngx_log_error(NGX_LOG_ALERT, log, 0, "quic token buffer is too small");
return NGX_ERROR;
}

> - token->data = ngx_pnalloc(c->pool, token->len);
> - if (token->data == NULL) {
> - return NGX_ERROR;
> - }
>
> ctx = EVP_CIPHER_CTX_new();
> if (ctx == NULL) {
> @@ -119,7 +107,7 @@ ngx_quic_new_token(ngx_connection_t *c,
> EVP_CIPHER_CTX_free(ctx);
>
> #ifdef NGX_QUIC_DEBUG_PACKETS
> - ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
> + ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
> "quic new token len:%uz %xV", token->len, token);
> #endif
>
> @@ -268,10 +256,8 @@ ngx_quic_validate_token(ngx_connection_t
>
> if (odcid.len) {
> pkt->odcid.len = odcid.len;
> - pkt->odcid.data = ngx_pstrdup(c->pool, &odcid);
> - if (pkt->odcid.data == NULL) {
> - return NGX_ERROR;
> - }
> + pkt->odcid.data = pkt->odcid_data;
> + ngx_memcpy(pkt->odcid.data, odcid.data, odcid.len);
>
> } else {
> pkt->odcid = pkt->dcid;
> 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
> @@ -12,9 +12,21 @@
> #include <ngx_core.h>
>
>
> +#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_TOKEN_BUF_SIZE (NGX_QUIC_AES_256_CBC_IV_LEN \
> + + NGX_QUIC_MAX_TOKEN_SIZE \
> + + NGX_QUIC_AES_256_CBC_BLOCK_SIZE)
> +
> +
> ngx_int_t ngx_quic_new_sr_token(ngx_connection_t *c, ngx_str_t *cid,
> u_char *secret, u_char *token);
> -ngx_int_t ngx_quic_new_token(ngx_connection_t *c, struct sockaddr *sockaddr,
> +ngx_int_t ngx_quic_new_token(ngx_log_t *log, struct sockaddr *sockaddr,
> socklen_t socklen, u_char *key, ngx_str_t *token, ngx_str_t *odcid,
> time_t expires, ngx_uint_t is_retry);
> ngx_int_t ngx_quic_validate_token(ngx_connection_t *c,
> diff --git a/src/event/quic/ngx_event_quic_transport.h b/src/event/quic/ngx_event_quic_transport.h
> --- a/src/event/quic/ngx_event_quic_transport.h
> +++ b/src/event/quic/ngx_event_quic_transport.h
> @@ -322,6 +322,7 @@ typedef struct {
>
> /* cleartext fields */
> ngx_str_t odcid; /* retry packet tag */
> + u_char odcid_data[NGX_QUIC_MAX_CID_LEN];
> ngx_str_t dcid;
> ngx_str_t scid;
> uint64_t pn;

I will include the proposed changes in the new series.
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org