On Mon, Feb 21, 2022 at 05:51:42PM +0300, Sergey Kandaurov wrote:
> 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.

thanks, fixed this

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

sorry, this is a leftover. Indeed, there is no meaning in keeping this.
I had some ideas how to initialize it on declaration and merge 'info'
with 'label', but the result was too clumsy and unsafe to use.

removed info from structure and restored local variable..

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

yes, exactly. currently boringssl just defines max size 48 to keep
SHA384, and I think we can safely follow it. If anything will appear
in future, we can always adjust size.

I've renamed NGX_QUIC_KEY_LEN to NGX_QUIC_MAX_MD_SIZE and renamed
structure holding buffer to ngx_quic_md_t, and use it for secret,
key and hp, as it makes sens to use them uniformly.

> The error, if ever leave it there, deserves rasing logging level
> to "alert" as clearly a programmatic error.
>
agreed. This place is the worst in patch - we get size from the library
and hope that our bufer is big enough, otherwise fail with a runtime error.

On the other side, it is pretty clear that size here is just digest
length, and we can expect reasonable numbers here.

# HG changeset patch
# User Vladimir Homutov <vl@nginx.com>
# Date 1645524401 -10800
# Tue Feb 22 13:06:41 2022 +0300
# Branch quic
# Node ID bb1717365759760bc8175b8d8084819a6ec35c26
# 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,9 @@

#define NGX_QUIC_AES_128_KEY_LEN 16

+/* largest hash used in TLS is SHA-384 */
+#define NGX_QUIC_MAX_MD_SIZE 48
+
#define NGX_AES_128_GCM_SHA256 0x1301
#define NGX_AES_256_GCM_SHA384 0x1302
#define NGX_CHACHA20_POLY1305_SHA256 0x1303
@@ -30,6 +33,18 @@


typedef struct {
+ size_t len;
+ u_char data[NGX_QUIC_MAX_MD_SIZE];
+} ngx_quic_md_t;
+
+
+typedef struct {
+ size_t len;
+ u_char data[NGX_QUIC_IV_LEN];
+} ngx_quic_iv_t;
+
+
+typedef struct {
const ngx_quic_cipher_t *c;
const EVP_CIPHER *hp;
const EVP_MD *d;
@@ -37,10 +52,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_md_t secret;
+ ngx_quic_md_t key;
+ ngx_quic_iv_t iv;
+ ngx_quic_md_t hp;
} ngx_quic_secret_t;


@@ -57,6 +72,25 @@ 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;
+} 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), \
+ }
+
+
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 +112,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 +238,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 +261,41 @@ 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;
+ 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);
+ info[1] = h->out_len;
+ info[2] = h->label_len;
+
+ p = ngx_cpymem(&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, info, 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 +679,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 +696,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_ALERT, pool->log, 0,
+ "unexpected secret len: %uz", secret_len);
return NGX_ERROR;
}

@@ -680,22 +709,17 @@ ngx_quic_keys_set_encryption_secret(ngx_
peer_secret->iv.len = NGX_QUIC_IV_LEN;
peer_secret->hp.len = key_len;

- struct {
- ngx_str_t label;
- ngx_str_t *key;
- const uint8_t *secret;
- } seq[] = {
- { ngx_string("tls13 quic key"), &peer_secret->key, secret },
- { ngx_string("tls13 quic iv"), &peer_secret->iv, secret },
- { ngx_string("tls13 quic hp"), &peer_secret->hp, secret },
+ secret_str.len = secret_len;
+ secret_str.data = (u_char *) secret;
+
+ ngx_quic_hkdf_t seq[] = {
+ ngx_quic_hkdf_set("tls13 quic key", &peer_secret->key, &secret_str),
+ ngx_quic_hkdf_set("tls13 quic iv", &peer_secret->iv, &secret_str),
+ ngx_quic_hkdf_set("tls13 quic hp", &peer_secret->hp, &secret_str),
};

for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
-
- if (ngx_quic_hkdf_expand(pool, ciphers.d, seq[i].key, &seq[i].label,
- seq[i].secret, secret_len)
- != NGX_OK)
- {
+ if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, pool) != NGX_OK) {
return NGX_ERROR;
}
}
@@ -769,49 +793,23 @@ ngx_quic_keys_update(ngx_connection_t *c
next->server.iv.len = NGX_QUIC_IV_LEN;
next->server.hp = current->server.hp;

- struct {
- ngx_str_t label;
- ngx_str_t *key;
- ngx_str_t *secret;
- } seq[] = {
- {
- ngx_string("tls13 quic ku"),
- &next->client.secret,
- &current->client.secret,
- },
- {
- ngx_string("tls13 quic key"),
- &next->client.key,
- &next->client.secret,
- },
- {
- ngx_string("tls13 quic iv"),
- &next->client.iv,
- &next->client.secret,
- },
- {
- ngx_string("tls13 quic ku"),
- &next->server.secret,
- &current->server.secret,
- },
- {
- ngx_string("tls13 quic key"),
- &next->server.key,
- &next->server.secret,
- },
- {
- ngx_string("tls13 quic iv"),
- &next->server.iv,
- &next->server.secret,
- },
+ ngx_quic_hkdf_t seq[] = {
+ ngx_quic_hkdf_set("tls13 quic ku",
+ &next->client.secret, &current->client.secret),
+ ngx_quic_hkdf_set("tls13 quic key",
+ &next->client.key, &next->client.secret),
+ ngx_quic_hkdf_set("tls13 quic iv",
+ &next->client.iv, &next->client.secret),
+ ngx_quic_hkdf_set("tls13 quic ku",
+ &next->server.secret, &current->server.secret),
+ ngx_quic_hkdf_set("tls13 quic key",
+ &next->server.key, &next->server.secret),
+ ngx_quic_hkdf_set("tls13 quic iv",
+ &next->server.iv, &next->server.secret),
};

for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
-
- if (ngx_quic_hkdf_expand(c->pool, ciphers.d, seq[i].key, &seq[i].label,
- seq[i].secret->data, seq[i].secret->len)
- != NGX_OK)
- {
+ if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, c->pool) != NGX_OK) {
return NGX_ERROR;
}
}
@@ -909,7 +907,7 @@ ngx_quic_create_retry_packet(ngx_quic_he
}

secret.key.len = sizeof(key);
- secret.key.data = key;
+ ngx_memcpy(secret.key.data, key, sizeof(key));
secret.iv.len = NGX_QUIC_IV_LEN;

if (ngx_quic_tls_seal(ciphers.c, &secret, &itag, nonce, &in, &ad, pkt->log)
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org