[PATCH 4 of 4] QUIC: avoided pool usage in token calculation

Roman Arutyunyan arut at nginx.com
Tue May 31 06:55:35 UTC 2022


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 at 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.



More information about the nginx-devel mailing list