[PATCH 3 of 3] QUIC: TLS_AES_128_CCM_SHA256 cipher suite support
Sergey Kandaurov
pluknet at nginx.com
Fri Jun 16 15:43:25 UTC 2023
> On 9 Jun 2023, at 11:12, Roman Arutyunyan <arut at nginx.com> wrote:
>
> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # Date 1686292815 -14400
> # Fri Jun 09 10:40:15 2023 +0400
> # Node ID 91c420191eaa6b8d8bd3dc1447d5e0880f41108f
> # Parent 74fa16d2a34a63a83fa76b46a56d8d68d44fa1bf
> QUIC: TLS_AES_128_CCM_SHA256 cipher suite support.
>
> 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
> @@ -94,6 +94,15 @@ ngx_quic_ciphers(ngx_uint_t id, ngx_quic
> len = 32;
> break;
>
> +#ifndef OPENSSL_IS_BORINGSSL
> + case TLS1_3_CK_AES_128_CCM_SHA256:
> + ciphers->c = EVP_aes_128_ccm();
> + ciphers->hp = EVP_aes_128_ctr();
> + ciphers->d = EVP_sha256();
> + len = 16;
> + break;
> +#endif
> +
For the record, I discussed with Roman whether to wrap this instead
with TLS1_3_CK_AES_128_CCM_SHA256 and think that's not going to work.
If ever this macro is added to BoringSSL, this may not work due to
AEAD API divergence, see TLS1_3_CK_CHACHA20_POLY1305_SHA256 precedent.
Using OPENSSL_IS_BORINGSSL looks more future promising.
> default:
> return NGX_ERROR;
> }
> @@ -393,18 +402,49 @@ ngx_quic_tls_open(const ngx_quic_cipher_
> return NGX_ERROR;
> }
>
> + if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
> + && EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
> + NULL)
> + == 0)
bad ident, should be aligned to EVP_CIPHER_CTX_ctrl
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + ngx_ssl_error(NGX_LOG_INFO, log, 0,
> + "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
> + return NGX_ERROR;
> + }
> +
This control becomes redundant here, documentation says wrt CCM:
This call is made to set the expected CCM tag value when decrypting
or the length of the tag (with the "tag" parameter set to NULL) when
encrypting.
So, setting just the expected CCM tag is enough if further tune up
the below SET_TAG, such that it comes before any EVP_DecryptUpdate().
> if (EVP_DecryptInit_ex(ctx, NULL, NULL, s->key.data, nonce) != 1) {
> EVP_CIPHER_CTX_free(ctx);
> ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptInit_ex() failed");
> return NGX_ERROR;
> }
>
> + if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
> + && EVP_DecryptUpdate(ctx, NULL, &len, NULL, in->len - NGX_QUIC_TAG_LEN)
> + != 1)
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
> + return NGX_ERROR;
> + }
> +
> if (EVP_DecryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
> EVP_CIPHER_CTX_free(ctx);
> ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
> return NGX_ERROR;
> }
>
> + tag = in->data + in->len - NGX_QUIC_TAG_LEN;
> +
> + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN, tag)
> + == 0)
bad ident
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + ngx_ssl_error(NGX_LOG_INFO, log, 0,
> + "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
> + return NGX_ERROR;
> + }
> +
So this SET_TAG control should be lifted up to avoid the above call.
Any way, calling EVP_CIPHER_CTX_ctrl during decryption looks ugly
(even though it doesn't seem to contradict documentation which claims that
tag can be set *after* passing AD; that could be due to documentation bug).
> if (EVP_DecryptUpdate(ctx, out->data, &len, in->data,
> in->len - NGX_QUIC_TAG_LEN)
> != 1)
> @@ -415,16 +455,6 @@ ngx_quic_tls_open(const ngx_quic_cipher_
> }
>
> out->len = len;
> - tag = in->data + in->len - NGX_QUIC_TAG_LEN;
> -
> - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN, tag)
> - == 0)
> - {
> - EVP_CIPHER_CTX_free(ctx);
> - ngx_ssl_error(NGX_LOG_INFO, log, 0,
> - "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
> - return NGX_ERROR;
> - }
>
> if (EVP_DecryptFinal_ex(ctx, out->data + len, &len) <= 0) {
> EVP_CIPHER_CTX_free(ctx);
> @@ -491,12 +521,31 @@ ngx_quic_tls_seal(const ngx_quic_cipher_
> return NGX_ERROR;
> }
>
> + if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
> + && EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
> + NULL)
> + == 0)
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + ngx_ssl_error(NGX_LOG_INFO, log, 0,
> + "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
> + return NGX_ERROR;
> + }
> +
Another refinement is to set tag before ivlen, for symmetry with _tls_open().
This makes the following update on top of your change:
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
@@ -393,6 +393,17 @@ ngx_quic_tls_open(const ngx_quic_cipher_
return NGX_ERROR;
}
+ tag = in->data + in->len - NGX_QUIC_TAG_LEN;
+
+ if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN, tag)
+ == 0)
+ {
+ EVP_CIPHER_CTX_free(ctx);
+ ngx_ssl_error(NGX_LOG_INFO, log, 0,
+ "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
+ return NGX_ERROR;
+ }
+
if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, s->iv.len, NULL)
== 0)
{
@@ -402,17 +413,6 @@ ngx_quic_tls_open(const ngx_quic_cipher_
return NGX_ERROR;
}
- if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
- && EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
- NULL)
- == 0)
- {
- EVP_CIPHER_CTX_free(ctx);
- ngx_ssl_error(NGX_LOG_INFO, log, 0,
- "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
- return NGX_ERROR;
- }
-
if (EVP_DecryptInit_ex(ctx, NULL, NULL, s->key.data, nonce) != 1) {
EVP_CIPHER_CTX_free(ctx);
ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptInit_ex() failed");
@@ -434,17 +434,6 @@ ngx_quic_tls_open(const ngx_quic_cipher_
return NGX_ERROR;
}
- tag = in->data + in->len - NGX_QUIC_TAG_LEN;
-
- if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN, tag)
- == 0)
- {
- EVP_CIPHER_CTX_free(ctx);
- ngx_ssl_error(NGX_LOG_INFO, log, 0,
- "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
- return NGX_ERROR;
- }
-
if (EVP_DecryptUpdate(ctx, out->data, &len, in->data,
in->len - NGX_QUIC_TAG_LEN)
!= 1)
@@ -512,15 +501,6 @@ ngx_quic_tls_seal(const ngx_quic_cipher_
return NGX_ERROR;
}
- if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, s->iv.len, NULL)
- == 0)
- {
- EVP_CIPHER_CTX_free(ctx);
- ngx_ssl_error(NGX_LOG_INFO, log, 0,
- "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_IVLEN) failed");
- return NGX_ERROR;
- }
-
if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
&& EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
NULL)
@@ -532,6 +512,15 @@ ngx_quic_tls_seal(const ngx_quic_cipher_
return NGX_ERROR;
}
+ if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, s->iv.len, NULL)
+ == 0)
+ {
+ EVP_CIPHER_CTX_free(ctx);
+ ngx_ssl_error(NGX_LOG_INFO, log, 0,
+ "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_IVLEN) failed");
+ return NGX_ERROR;
+ }
+
if (EVP_EncryptInit_ex(ctx, NULL, NULL, s->key.data, nonce) != 1) {
EVP_CIPHER_CTX_free(ctx);
ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptInit_ex() failed");
For further symmetry, we can cut tag length off the incoming length.
While here, found an occurrence of in->len which would be fine to fix.
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
@@ -393,7 +393,8 @@ ngx_quic_tls_open(const ngx_quic_cipher_
return NGX_ERROR;
}
- tag = in->data + in->len - NGX_QUIC_TAG_LEN;
+ in->len -= NGX_QUIC_TAG_LEN;
+ tag = in->data + in->len;
if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN, tag)
== 0)
@@ -420,8 +421,7 @@ ngx_quic_tls_open(const ngx_quic_cipher_
}
if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
- && EVP_DecryptUpdate(ctx, NULL, &len, NULL, in->len - NGX_QUIC_TAG_LEN)
- != 1)
+ && EVP_DecryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
{
EVP_CIPHER_CTX_free(ctx);
ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
@@ -434,10 +434,7 @@ ngx_quic_tls_open(const ngx_quic_cipher_
return NGX_ERROR;
}
- if (EVP_DecryptUpdate(ctx, out->data, &len, in->data,
- in->len - NGX_QUIC_TAG_LEN)
- != 1)
- {
+ if (EVP_DecryptUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
EVP_CIPHER_CTX_free(ctx);
ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
return NGX_ERROR;
@@ -558,7 +555,7 @@ ngx_quic_tls_seal(const ngx_quic_cipher_
out->len += len;
if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, NGX_QUIC_TAG_LEN,
- out->data + in->len)
+ out->data + out->len)
== 0)
{
EVP_CIPHER_CTX_free(ctx);
> if (EVP_EncryptInit_ex(ctx, NULL, NULL, s->key.data, nonce) != 1) {
> EVP_CIPHER_CTX_free(ctx);
> ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptInit_ex() failed");
> return NGX_ERROR;
> }
>
> + if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
> + && EVP_EncryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
> + {
> + EVP_CIPHER_CTX_free(ctx);
> + ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
> + return NGX_ERROR;
> + }
> +
> if (EVP_EncryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
> EVP_CIPHER_CTX_free(ctx);
> ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
> 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
> @@ -16,7 +16,7 @@
>
> #define NGX_QUIC_ENCRYPTION_LAST ((ssl_encryption_application) + 1)
>
> -/* RFC 5116, 5.1 and RFC 8439, 2.3/2.5 for all supported ciphers */
> +/* RFC 5116, 5.1/5.3 and RFC 8439, 2.3/2.5 for all supported ciphers */
> #define NGX_QUIC_IV_LEN 12
> #define NGX_QUIC_TAG_LEN 16
>
Otherwise, looks fine.
On a related note, it may have sense to finally stop wrapping
TLS1_3_CK_* macros, they are in BoringSSL for roughly a year,
which looks enough.
# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1686921209 -14400
# Fri Jun 16 17:13:29 2023 +0400
# Node ID 4d13f40bafe2158fd21275dac0782189a6192061
# Parent b1e7e783cfd8908d7894d6094ff00f5d5bdee657
QUIC: removed TLS1_3_CK_* macros wrap up.
They were preserved in 172705615d04 to ease building with older BoringSSL.
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
@@ -15,13 +15,6 @@
#define NGX_QUIC_AES_128_KEY_LEN 16
-#ifndef TLS1_3_CK_AES_128_GCM_SHA256
-#define TLS1_3_CK_AES_128_GCM_SHA256 0x03001301
-#define TLS1_3_CK_AES_256_GCM_SHA384 0x03001302
-#define TLS1_3_CK_CHACHA20_POLY1305_SHA256 \
- 0x03001303
-#endif
-
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,
--
Sergey Kandaurov
More information about the nginx-devel
mailing list