On Mon, Feb 21, 2022 at 02:10:31PM +0300, Vladimir Homutov wrote:
> Patch subject is complete summary.
>
>
> src/event/quic/ngx_event_quic_protection.c | 202 +++++++++++++++-------------
> 1 files changed, 105 insertions(+), 97 deletions(-)
>
>

> # HG changeset patch
> # User Vladimir Homutov <vl@nginx.com>
> # Date 1645440604 -10800
> # Mon Feb 21 13:50:04 2022 +0300
> # Branch quic
> # Node ID 1a0a12bef7f00b5422d449b2d4642fff39e0a47e
> # Parent 55b38514729b8f848709b31295e72d6886a7a433
> QUIC: fixed-length buffers for secrets.
>
> diff --git a/src/event/quic/ngx_event_quic_protection.c b/src/event/quic/ngx_event_quic_protection.c
> --- a/src/event/quic/ngx_event_quic_protection.c
> +++ b/src/event/quic/ngx_event_quic_protection.c
> @@ -17,6 +17,8 @@
>
> #define NGX_QUIC_AES_128_KEY_LEN 16
>
> +#define NGX_QUIC_KEY_LEN 32
> +
> #define NGX_AES_128_GCM_SHA256 0x1301
> #define NGX_AES_256_GCM_SHA384 0x1302
> #define NGX_CHACHA20_POLY1305_SHA256 0x1303
> @@ -30,6 +32,27 @@
>
>
> typedef struct {
> + size_t len;
> + u_char data[SHA256_DIGEST_LENGTH];
> +} ngx_quic_okm_t;
> +
> +typedef struct {
> + size_t len;
> + u_char data[NGX_QUIC_KEY_LEN];
> +} ngx_quic_key_t;
> +
> +typedef struct {
> + size_t len;
> + u_char data[NGX_QUIC_KEY_LEN];
> +} ngx_quic_hp_t;
> +
> +typedef struct {
> + size_t len;
> + u_char data[NGX_QUIC_IV_LEN];
> +} ngx_quic_iv_t;

Style: two empty lines between struct declarations.

> +
> +
> +typedef struct {
> const ngx_quic_cipher_t *c;
> const EVP_CIPHER *hp;
> const EVP_MD *d;
> @@ -37,10 +60,10 @@ typedef struct {
>
>
> typedef struct ngx_quic_secret_s {
> - ngx_str_t secret;
> - ngx_str_t key;
> - ngx_str_t iv;
> - ngx_str_t hp;
> + ngx_quic_okm_t secret;
> + ngx_quic_key_t key;
> + ngx_quic_iv_t iv;
> + ngx_quic_hp_t hp;
> } ngx_quic_secret_t;
>
>
> @@ -57,6 +80,29 @@ struct ngx_quic_keys_s {
> };
>
>
> +typedef struct {
> + size_t out_len;
> + u_char *out;
> +
> + size_t prk_len;
> + const uint8_t *prk;
> +
> + size_t label_len;
> + const u_char *label;
> +
> + size_t info_len;
> + uint8_t info[20];
> +} ngx_quic_hkdf_t;
> +
> +#define ngx_quic_hkdf_set(label, out, prk) \
> + { \
> + (out)->len, (out)->data, \
> + (prk)->len, (prk)->data, \
> + (sizeof(label) - 1), (u_char *)(label), \
> + 0, { 0 } \
> + }
> +
> +
> static ngx_int_t ngx_hkdf_expand(u_char *out_key, size_t out_len,
> const EVP_MD *digest, const u_char *prk, size_t prk_len,
> const u_char *info, size_t info_len);
> @@ -78,8 +124,8 @@ static ngx_int_t ngx_quic_tls_seal(const
> ngx_str_t *ad, ngx_log_t *log);
> static ngx_int_t ngx_quic_tls_hp(ngx_log_t *log, const EVP_CIPHER *cipher,
> ngx_quic_secret_t *s, u_char *out, u_char *in);
> -static ngx_int_t ngx_quic_hkdf_expand(ngx_pool_t *pool, const EVP_MD *digest,
> - ngx_str_t *out, ngx_str_t *label, const uint8_t *prk, size_t prk_len);
> +static ngx_int_t ngx_quic_hkdf_expand(ngx_quic_hkdf_t *hkdf,
> + const EVP_MD *digest, ngx_pool_t *pool);
>
> static ngx_int_t ngx_quic_create_packet(ngx_quic_header_t *pkt,
> ngx_str_t *res);
> @@ -204,28 +250,20 @@ ngx_quic_keys_set_initial_secret(ngx_poo
> client->iv.len = NGX_QUIC_IV_LEN;
> server->iv.len = NGX_QUIC_IV_LEN;
>
> - struct {
> - ngx_str_t label;
> - ngx_str_t *key;
> - ngx_str_t *prk;
> - } seq[] = {
> + ngx_quic_hkdf_t seq[] = {
> /* labels per RFC 9001, 5.1. Packet Protection Keys */
> - { ngx_string("tls13 client in"), &client->secret, &iss },
> - { ngx_string("tls13 quic key"), &client->key, &client->secret },
> - { ngx_string("tls13 quic iv"), &client->iv, &client->secret },
> - { ngx_string("tls13 quic hp"), &client->hp, &client->secret },
> - { ngx_string("tls13 server in"), &server->secret, &iss },
> - { ngx_string("tls13 quic key"), &server->key, &server->secret },
> - { ngx_string("tls13 quic iv"), &server->iv, &server->secret },
> - { ngx_string("tls13 quic hp"), &server->hp, &server->secret },
> + ngx_quic_hkdf_set("tls13 client in", &client->secret, &iss),
> + ngx_quic_hkdf_set("tls13 quic key", &client->key, &client->secret),
> + ngx_quic_hkdf_set("tls13 quic iv", &client->iv, &client->secret),
> + ngx_quic_hkdf_set("tls13 quic hp", &client->hp, &client->secret),
> + ngx_quic_hkdf_set("tls13 server in", &server->secret, &iss),
> + ngx_quic_hkdf_set("tls13 quic key", &server->key, &server->secret),
> + ngx_quic_hkdf_set("tls13 quic iv", &server->iv, &server->secret),
> + ngx_quic_hkdf_set("tls13 quic hp", &server->hp, &server->secret),
> };
>
> for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
> -
> - if (ngx_quic_hkdf_expand(pool, digest, seq[i].key, &seq[i].label,
> - seq[i].prk->data, seq[i].prk->len)
> - != NGX_OK)
> - {
> + if (ngx_quic_hkdf_expand(&seq[i], digest, pool) != NGX_OK) {
> return NGX_ERROR;
> }
> }
> @@ -235,40 +273,39 @@ ngx_quic_keys_set_initial_secret(ngx_poo
>
>
> static ngx_int_t
> -ngx_quic_hkdf_expand(ngx_pool_t *pool, const EVP_MD *digest, ngx_str_t *out,
> - ngx_str_t *label, const uint8_t *prk, size_t prk_len)
> +ngx_quic_hkdf_expand(ngx_quic_hkdf_t *h, const EVP_MD *digest, ngx_pool_t *pool)
> {
> - size_t info_len;
> uint8_t *p;
> - uint8_t info[20];
>
> - if (out->data == NULL) {
> - out->data = ngx_pnalloc(pool, out->len);
> - if (out->data == NULL) {
> + if (h->out == NULL) {
> + h->out = ngx_pnalloc(pool, h->out_len);
> + if (h->out == NULL) {
> return NGX_ERROR;
> }
> }
>
> - info_len = 2 + 1 + label->len + 1;
> + h->info_len = 2 + 1 + h->label_len + 1;
>
> - info[0] = 0;
> - info[1] = out->len;
> - info[2] = label->len;
> - p = ngx_cpymem(&info[3], label->data, label->len);
> + h->info[0] = 0;
> + h->info[1] = h->out_len;
> + h->info[2] = h->label_len;

Why?
info/info_len aren't used/useful outside of ngx_quic_hkdf_expand(),
they are barely one-time local storages to produce traffic secrets.

> +
> + p = ngx_cpymem(&h->info[3], h->label, h->label_len);
> *p = '\0';
>
> - if (ngx_hkdf_expand(out->data, out->len, digest,
> - prk, prk_len, info, info_len)
> + if (ngx_hkdf_expand(h->out, h->out_len, digest,
> + h->prk, h->prk_len, h->info, h->info_len)
> != NGX_OK)
> {
> ngx_ssl_error(NGX_LOG_INFO, pool->log, 0,
> - "ngx_hkdf_expand(%V) failed", label);
> + "ngx_hkdf_expand(%*s) failed", h->label_len, h->label);
> return NGX_ERROR;
> }
>
> #ifdef NGX_QUIC_DEBUG_CRYPTO
> - ngx_log_debug3(NGX_LOG_DEBUG_EVENT, pool->log, 0,
> - "quic expand %V key len:%uz %xV", label, out->len, out);
> + ngx_log_debug5(NGX_LOG_DEBUG_EVENT, pool->log, 0,
> + "quic expand \"%*s\" key len:%uz %*xs",
> + h->label_len, h->label, h->out_len, h->out_len, h->out);
> #endif
>
> return NGX_OK;
> @@ -652,6 +689,7 @@ ngx_quic_keys_set_encryption_secret(ngx_
> const SSL_CIPHER *cipher, const uint8_t *secret, size_t secret_len)
> {
> ngx_int_t key_len;
> + ngx_str_t secret_str;
> ngx_uint_t i;
> ngx_quic_secret_t *peer_secret;
> ngx_quic_ciphers_t ciphers;
> @@ -668,8 +706,9 @@ ngx_quic_keys_set_encryption_secret(ngx_
> return NGX_ERROR;
> }
>
> - peer_secret->secret.data = ngx_pnalloc(pool, secret_len);
> - if (peer_secret->secret.data == NULL) {
> + if (sizeof(peer_secret->secret.data) < secret_len) {
> + ngx_log_error(NGX_LOG_ERR, pool->log, 0,
> + "unexpected secret len: %uz", secret_len);
> return NGX_ERROR;
> }

This won't work with cipher suite hash algorithms used to produce
HKDF Hash.length (read: secret length) above SHA256_DIGEST_LENGTH,
such as TLS_AES_256_GCM_SHA384 or any future TLSv1.3 cipher suites
with SHA384 or above. The same for hardcoding NGX_QUIC_KEY_LEN.

The error, if ever leave it there, deserves rasing logging level
to "alert" as clearly a programmatic error.

[..]
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org