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 <[email protected]> > # 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 -- [email protected] To unsubscribe send an email to [email protected]
