> On 21 Sep 2023, at 17:29, Roman Arutyunyan <arut@nginx.com> wrote:
>
> Hi,
>
> On Thu, Sep 07, 2023 at 07:14:00PM +0400, Sergey Kandaurov wrote:
>> # HG changeset patch
>> # User Sergey Kandaurov <pluknet@nginx.com>
>> # Date 1694099425 -14400
>> # Thu Sep 07 19:10:25 2023 +0400
>> # Node ID 813128cee322830435a95903993b17fb24683da7
>> # Parent 8bd0104b7e6b658a1696fe7f3e2f1868ac2ae1f9
>> QUIC: explicitly zero out unused keying material.
>>
>> diff --git a/src/event/quic/ngx_event_quic_openssl_compat.c b/src/event/quic/ngx_event_quic_openssl_compat.c
>> --- a/src/event/quic/ngx_event_quic_openssl_compat.c
>> +++ b/src/event/quic/ngx_event_quic_openssl_compat.c
>> @@ -245,15 +245,6 @@ ngx_quic_compat_set_encryption_secret(ng
>> return NGX_ERROR;
>> }
>>
>> - if (sizeof(peer_secret->secret.data) < secret_len) {
>> - ngx_log_error(NGX_LOG_ALERT, c->log, 0,
>> - "unexpected secret len: %uz", secret_len);
>> - return NGX_ERROR;
>> - }
>> -
>> - peer_secret->secret.len = secret_len;
>> - ngx_memcpy(peer_secret->secret.data, secret, secret_len);
>> -
>> peer_secret->key.len = key_len;
>> peer_secret->iv.len = NGX_QUIC_IV_LEN;
>>
>> @@ -275,6 +266,9 @@ ngx_quic_compat_set_encryption_secret(ng
>> return NGX_ERROR;
>> }
>>
>> + ngx_explicit_memzero(secret_str.data, secret_str.len);
>> + ngx_explicit_memzero(peer_secret->key.data, peer_secret->key.len);
>> +
>> /* register cleanup handler once */
>>
>> if (level == ssl_encryption_handshake) {
>> 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
>> @@ -719,6 +719,8 @@ ngx_quic_keys_set_encryption_secret(ngx_
>> return NGX_ERROR;
>> }
>>
>> + ngx_explicit_memzero(peer_secret->key.data, peer_secret->key.len);
>> +
>> return NGX_OK;
>> }
>>
>> @@ -749,6 +751,12 @@ ngx_quic_keys_discard(ngx_quic_keys_t *k
>>
>> ngx_quic_crypto_hp_cleanup(client);
>> ngx_quic_crypto_hp_cleanup(server);
>> +
>> + ngx_explicit_memzero(client->secret.data, client->secret.len);
>> + ngx_explicit_memzero(client->key.data, client->key.len);
>> +
>> + ngx_explicit_memzero(server->secret.data, server->secret.len);
>> + ngx_explicit_memzero(server->key.data, server->key.len);
>> }
>>
>>
>> @@ -838,6 +846,14 @@ ngx_quic_keys_update(ngx_event_t *ev)
>> goto failed;
>> }
>>
>> + ngx_explicit_memzero(current->client.secret.data,
>> + current->client.secret.len);
>> + ngx_explicit_memzero(current->server.secret.data,
>> + current->server.secret.len);
>> +
>> + ngx_explicit_memzero(next->client.key.data, next->client.key.len);
>> + ngx_explicit_memzero(next->server.key.data, next->server.key.len);
>> +
>> return;
>>
>> failed:
>> @@ -866,6 +882,12 @@ ngx_quic_keys_cleanup(void *data)
>> secrets = &keys->next_key;
>> ngx_quic_crypto_cleanup(&secrets->client);
>> ngx_quic_crypto_cleanup(&secrets->server);
>> +
>> + ngx_explicit_memzero(secrets->client.secret.data,
>> + secrets->client.secret.len);
>> +
>> + ngx_explicit_memzero(secrets->server.secret.data,
>> + secrets->server.secret.len);
>> }
>>
>>
>
> Maybe also we need to zero out the secret in ngx_quic_compat_keylog_callback()?
>

Yes, OpenSSL behaviour should be preserved.
It was just done the wrong way, patch on top:

diff --git a/src/event/quic/ngx_event_quic_openssl_compat.c b/src/event/quic/ngx_event_quic_openssl_compat.c
--- a/src/event/quic/ngx_event_quic_openssl_compat.c
+++ b/src/event/quic/ngx_event_quic_openssl_compat.c
@@ -219,6 +219,8 @@ ngx_quic_compat_keylog_callback(const SS
(void) ngx_quic_compat_set_encryption_secret(c, &com->keys, level,
cipher, secret, n);
}
+
+ ngx_explicit_memzero(secret, n);
}


@@ -281,7 +283,6 @@ ngx_quic_compat_set_encryption_secret(ng
return NGX_ERROR;
}

- ngx_explicit_memzero(secret_str.data, secret_str.len);
ngx_explicit_memzero(peer_secret->key.data, peer_secret->key.len);

return NGX_OK;

> Also, this patch made me think about removing key and hp from ngx_quic_secret_t.
> Since we have ctx/hp_ctx now, we only need them when creating these contexts.
> This will reduce the amount of sensitive data we permanently store in memory.
>

This was addressed, we can additionally reduce
sizeof(ngx_quic_secret_t), now that it became possible for "key".

> Attached is my effort towards making key and hp local.

"hp" is needed for BoringSSL, but we can through away "key".
Anyway, "key" is the key part of cryptography, keeping "hp" should be ok.

# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1697209278 -14400
# Fri Oct 13 19:01:18 2023 +0400
# Node ID 1316dd35650b2a95d6454515100d889d44b7fa8b
# Parent 16bf91cd32ca6667967b5321232f076a42e7200e
QUIC: removed key field from ngx_quic_secret_t.

It is made local as it is only needed now when creating crypto context.

BoringSSL lacks EVP interface for ChaCha20, providing instead
a function for one-shot encryption, thus hp is still preserved.

Based on a patch by Roman Arutyunyan.

diff --git a/src/event/quic/ngx_event_quic_openssl_compat.c b/src/event/quic/ngx_event_quic_openssl_compat.c
--- a/src/event/quic/ngx_event_quic_openssl_compat.c
+++ b/src/event/quic/ngx_event_quic_openssl_compat.c
@@ -232,6 +232,7 @@ ngx_quic_compat_set_encryption_secret(ng
ngx_int_t key_len;
ngx_str_t secret_str;
ngx_uint_t i;
+ ngx_quic_md_t key;
ngx_quic_hkdf_t seq[2];
ngx_quic_secret_t *peer_secret;
ngx_quic_ciphers_t ciphers;
@@ -248,13 +249,14 @@ ngx_quic_compat_set_encryption_secret(ng
return NGX_ERROR;
}

- peer_secret->key.len = key_len;
+ key.len = key_len;
+
peer_secret->iv.len = NGX_QUIC_IV_LEN;

secret_str.len = secret_len;
secret_str.data = (u_char *) secret;

- ngx_quic_hkdf_set(&seq[0], "tls13 key", &peer_secret->key, &secret_str);
+ ngx_quic_hkdf_set(&seq[0], "tls13 key", &key, &secret_str);
ngx_quic_hkdf_set(&seq[1], "tls13 iv", &peer_secret->iv, &secret_str);

for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
@@ -279,11 +281,13 @@ ngx_quic_compat_set_encryption_secret(ng

ngx_quic_crypto_cleanup(peer_secret);

- if (ngx_quic_crypto_init(ciphers.c, peer_secret, 1, c->log) == NGX_ERROR) {
+ if (ngx_quic_crypto_init(ciphers.c, peer_secret, &key, 1, c->log)
+ == NGX_ERROR)
+ {
return NGX_ERROR;
}

- ngx_explicit_memzero(peer_secret->key.data, peer_secret->key.len);
+ ngx_explicit_memzero(key.data, key.len);

return NGX_OK;
}
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
@@ -117,6 +117,7 @@ ngx_quic_keys_set_initial_secret(ngx_qui
ngx_str_t iss;
ngx_uint_t i;
const EVP_MD *digest;
+ ngx_quic_md_t client_key, server_key;
ngx_quic_hkdf_t seq[8];
ngx_quic_secret_t *client, *server;
ngx_quic_ciphers_t ciphers;
@@ -160,8 +161,8 @@ ngx_quic_keys_set_initial_secret(ngx_qui
client->secret.len = SHA256_DIGEST_LENGTH;
server->secret.len = SHA256_DIGEST_LENGTH;

- client->key.len = NGX_QUIC_AES_128_KEY_LEN;
- server->key.len = NGX_QUIC_AES_128_KEY_LEN;
+ client_key.len = NGX_QUIC_AES_128_KEY_LEN;
+ server_key.len = NGX_QUIC_AES_128_KEY_LEN;

client->hp.len = NGX_QUIC_AES_128_KEY_LEN;
server->hp.len = NGX_QUIC_AES_128_KEY_LEN;
@@ -171,11 +172,11 @@ ngx_quic_keys_set_initial_secret(ngx_qui

/* labels per RFC 9001, 5.1. Packet Protection Keys */
ngx_quic_hkdf_set(&seq[0], "tls13 client in", &client->secret, &iss);
- ngx_quic_hkdf_set(&seq[1], "tls13 quic key", &client->key, &client->secret);
+ ngx_quic_hkdf_set(&seq[1], "tls13 quic key", &client_key, &client->secret);
ngx_quic_hkdf_set(&seq[2], "tls13 quic iv", &client->iv, &client->secret);
ngx_quic_hkdf_set(&seq[3], "tls13 quic hp", &client->hp, &client->secret);
ngx_quic_hkdf_set(&seq[4], "tls13 server in", &server->secret, &iss);
- ngx_quic_hkdf_set(&seq[5], "tls13 quic key", &server->key, &server->secret);
+ ngx_quic_hkdf_set(&seq[5], "tls13 quic key", &server_key, &server->secret);
ngx_quic_hkdf_set(&seq[6], "tls13 quic iv", &server->iv, &server->secret);
ngx_quic_hkdf_set(&seq[7], "tls13 quic hp", &server->hp, &server->secret);

@@ -189,11 +190,15 @@ ngx_quic_keys_set_initial_secret(ngx_qui
return NGX_ERROR;
}

- if (ngx_quic_crypto_init(ciphers.c, client, 0, log) == NGX_ERROR) {
+ if (ngx_quic_crypto_init(ciphers.c, client, &client_key, 0, log)
+ == NGX_ERROR)
+ {
return NGX_ERROR;
}

- if (ngx_quic_crypto_init(ciphers.c, server, 1, log) == NGX_ERROR) {
+ if (ngx_quic_crypto_init(ciphers.c, server, &server_key, 1, log)
+ == NGX_ERROR)
+ {
goto failed;
}

@@ -376,13 +381,13 @@ failed:

ngx_int_t
ngx_quic_crypto_init(const ngx_quic_cipher_t *cipher, ngx_quic_secret_t *s,
- ngx_int_t enc, ngx_log_t *log)
+ ngx_quic_md_t *key, ngx_int_t enc, ngx_log_t *log)
{

#ifdef OPENSSL_IS_BORINGSSL
EVP_AEAD_CTX *ctx;

- ctx = EVP_AEAD_CTX_new(cipher, s->key.data, s->key.len,
+ ctx = EVP_AEAD_CTX_new(cipher, key->data, key->len,
EVP_AEAD_DEFAULT_TAG_LENGTH);
if (ctx == NULL) {
ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_new() failed");
@@ -423,7 +428,7 @@ ngx_quic_crypto_init(const ngx_quic_ciph
return NGX_ERROR;
}

- if (EVP_CipherInit_ex(ctx, NULL, NULL, s->key.data, NULL, enc) != 1) {
+ if (EVP_CipherInit_ex(ctx, NULL, NULL, key->data, NULL, enc) != 1) {
EVP_CIPHER_CTX_free(ctx);
ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherInit_ex() failed");
return NGX_ERROR;
@@ -652,6 +657,7 @@ ngx_quic_keys_set_encryption_secret(ngx_
ngx_int_t key_len;
ngx_str_t secret_str;
ngx_uint_t i;
+ ngx_quic_md_t key;
ngx_quic_hkdf_t seq[3];
ngx_quic_secret_t *peer_secret;
ngx_quic_ciphers_t ciphers;
@@ -677,15 +683,14 @@ ngx_quic_keys_set_encryption_secret(ngx_
peer_secret->secret.len = secret_len;
ngx_memcpy(peer_secret->secret.data, secret, secret_len);

- peer_secret->key.len = key_len;
+ key.len = key_len;
peer_secret->iv.len = NGX_QUIC_IV_LEN;
peer_secret->hp.len = key_len;

secret_str.len = secret_len;
secret_str.data = (u_char *) secret;

- ngx_quic_hkdf_set(&seq[0], "tls13 quic key",
- &peer_secret->key, &secret_str);
+ ngx_quic_hkdf_set(&seq[0], "tls13 quic key", &key, &secret_str);
ngx_quic_hkdf_set(&seq[1], "tls13 quic iv", &peer_secret->iv, &secret_str);
ngx_quic_hkdf_set(&seq[2], "tls13 quic hp", &peer_secret->hp, &secret_str);

@@ -695,7 +700,7 @@ ngx_quic_keys_set_encryption_secret(ngx_
}
}

- if (ngx_quic_crypto_init(ciphers.c, peer_secret, is_write, log)
+ if (ngx_quic_crypto_init(ciphers.c, peer_secret, &key, is_write, log)
== NGX_ERROR)
{
return NGX_ERROR;
@@ -705,7 +710,7 @@ ngx_quic_keys_set_encryption_secret(ngx_
return NGX_ERROR;
}

- ngx_explicit_memzero(peer_secret->key.data, peer_secret->key.len);
+ ngx_explicit_memzero(key.data, key.len);

return NGX_OK;
}
@@ -739,10 +744,7 @@ ngx_quic_keys_discard(ngx_quic_keys_t *k
ngx_quic_crypto_hp_cleanup(server);

ngx_explicit_memzero(client->secret.data, client->secret.len);
- ngx_explicit_memzero(client->key.data, client->key.len);
-
ngx_explicit_memzero(server->secret.data, server->secret.len);
- ngx_explicit_memzero(server->key.data, server->key.len);
}


@@ -766,7 +768,9 @@ ngx_quic_keys_switch(ngx_connection_t *c
void
ngx_quic_keys_update(ngx_event_t *ev)
{
+ ngx_int_t key_len;
ngx_uint_t i;
+ ngx_quic_md_t client_key, server_key;
ngx_quic_hkdf_t seq[6];
ngx_quic_keys_t *keys;
ngx_connection_t *c;
@@ -785,18 +789,21 @@ ngx_quic_keys_update(ngx_event_t *ev)

c->log->action = "updating keys";

- if (ngx_quic_ciphers(keys->cipher, &ciphers) == NGX_ERROR) {
+ key_len = ngx_quic_ciphers(keys->cipher, &ciphers);
+
+ if (key_len == NGX_ERROR) {
goto failed;
}

+ client_key.len = key_len;
+ server_key.len = key_len;
+
next->client.secret.len = current->client.secret.len;
- next->client.key.len = current->client.key.len;
next->client.iv.len = NGX_QUIC_IV_LEN;
next->client.hp = current->client.hp;
next->client.hp_ctx = current->client.hp_ctx;

next->server.secret.len = current->server.secret.len;
- next->server.key.len = current->server.key.len;
next->server.iv.len = NGX_QUIC_IV_LEN;
next->server.hp = current->server.hp;
next->server.hp_ctx = current->server.hp_ctx;
@@ -804,13 +811,13 @@ ngx_quic_keys_update(ngx_event_t *ev)
ngx_quic_hkdf_set(&seq[0], "tls13 quic ku",
&next->client.secret, &current->client.secret);
ngx_quic_hkdf_set(&seq[1], "tls13 quic key",
- &next->client.key, &next->client.secret);
+ &client_key, &next->client.secret);
ngx_quic_hkdf_set(&seq[2], "tls13 quic iv",
&next->client.iv, &next->client.secret);
ngx_quic_hkdf_set(&seq[3], "tls13 quic ku",
&next->server.secret, &current->server.secret);
ngx_quic_hkdf_set(&seq[4], "tls13 quic key",
- &next->server.key, &next->server.secret);
+ &server_key, &next->server.secret);
ngx_quic_hkdf_set(&seq[5], "tls13 quic iv",
&next->server.iv, &next->server.secret);

@@ -820,12 +827,14 @@ ngx_quic_keys_update(ngx_event_t *ev)
}
}

- if (ngx_quic_crypto_init(ciphers.c, &next->client, 0, c->log) == NGX_ERROR)
+ if (ngx_quic_crypto_init(ciphers.c, &next->client, &client_key, 0, c->log)
+ == NGX_ERROR)
{
goto failed;
}

- if (ngx_quic_crypto_init(ciphers.c, &next->server, 1, c->log) == NGX_ERROR)
+ if (ngx_quic_crypto_init(ciphers.c, &next->server, &server_key, 1, c->log)
+ == NGX_ERROR)
{
goto failed;
}
@@ -835,8 +844,8 @@ ngx_quic_keys_update(ngx_event_t *ev)
ngx_explicit_memzero(current->server.secret.data,
current->server.secret.len);

- ngx_explicit_memzero(next->client.key.data, next->client.key.len);
- ngx_explicit_memzero(next->server.key.data, next->server.key.len);
+ ngx_explicit_memzero(client_key.data, client_key.len);
+ ngx_explicit_memzero(server_key.data, server_key.len);

return;

@@ -927,10 +936,11 @@ ngx_quic_create_retry_packet(ngx_quic_he
{
u_char *start;
ngx_str_t ad, itag;
+ ngx_quic_md_t key;
ngx_quic_ciphers_t ciphers;

/* 5.8. Retry Packet Integrity */
- static u_char key[16] =
+ static u_char key_data[16] =
"\xbe\x0c\x69\x0b\x9f\x66\x57\x5a\x1d\x76\x6b\x54\xe3\x68\xc8\x4e";
static u_char nonce[NGX_QUIC_IV_LEN] =
"\x46\x15\x99\xd3\x5d\x63\x2b\xf2\x23\x98\x25\xbb";
@@ -957,11 +967,13 @@ ngx_quic_create_retry_packet(ngx_quic_he
return NGX_ERROR;
}

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

- if (ngx_quic_crypto_init(ciphers.c, &secret, 1, pkt->log) == NGX_ERROR) {
+ if (ngx_quic_crypto_init(ciphers.c, &secret, &key, 1, pkt->log)
+ == NGX_ERROR)
+ {
return NGX_ERROR;
}

diff --git a/src/event/quic/ngx_event_quic_protection.h b/src/event/quic/ngx_event_quic_protection.h
--- a/src/event/quic/ngx_event_quic_protection.h
+++ b/src/event/quic/ngx_event_quic_protection.h
@@ -47,7 +47,6 @@ typedef struct {

typedef struct {
ngx_quic_md_t secret;
- ngx_quic_md_t key;
ngx_quic_iv_t iv;
ngx_quic_md_t hp;
ngx_quic_crypto_ctx_t *ctx;
@@ -110,7 +109,7 @@ ngx_int_t ngx_quic_decrypt(ngx_quic_head
void ngx_quic_compute_nonce(u_char *nonce, size_t len, uint64_t pn);
ngx_int_t ngx_quic_ciphers(ngx_uint_t id, ngx_quic_ciphers_t *ciphers);
ngx_int_t ngx_quic_crypto_init(const ngx_quic_cipher_t *cipher,
- ngx_quic_secret_t *s, ngx_int_t enc, ngx_log_t *log);
+ ngx_quic_secret_t *s, ngx_quic_md_t *key, ngx_int_t enc, ngx_log_t *log);
ngx_int_t ngx_quic_crypto_seal(ngx_quic_secret_t *s, ngx_str_t *out,
u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log);
void ngx_quic_crypto_cleanup(ngx_quic_secret_t *s);

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel