From 8a7d327d24a2989a3f43ec5b511e36134d7b2f02 Mon Sep 17 00:00:00 2001 From: sebastian-carpenter Date: Fri, 6 Mar 2026 10:35:24 -0700 Subject: [PATCH] ECH fixes F-293, F-201, F-358, F-203 --- src/ssl_ech.c | 180 +++++++++++++++++++++++++++++++++++--------------- tests/api.c | 132 +++++++++++++++++++++++++++--------- 2 files changed, 228 insertions(+), 84 deletions(-) diff --git a/src/ssl_ech.c b/src/ssl_ech.c index 747810d998..6eccc3f871 100644 --- a/src/ssl_ech.c +++ b/src/ssl_ech.c @@ -490,68 +490,61 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, const byte* echConfigs, word32 echConfigsLen) { int ret = 0; - int i; + word32 configIdx; + word32 idx; int j; word16 totalLength; word16 version; word16 length; word16 hpkePubkeyLen; word16 cipherSuitesLen; - word16 publicNameLen; + word16 extensionsLen; + byte publicNameLen; WOLFSSL_EchConfig* configList = NULL; WOLFSSL_EchConfig* workingConfig = NULL; WOLFSSL_EchConfig* lastConfig = NULL; - byte* echConfig = NULL; + const byte* echConfig = NULL; - if (outputConfigs == NULL || echConfigs == NULL || echConfigsLen == 0) + if (outputConfigs == NULL || echConfigs == NULL || echConfigsLen < 2) return BAD_FUNC_ARG; /* check that the total length is well formed */ ato16(echConfigs, &totalLength); - if (totalLength != echConfigsLen - 2) { return WOLFSSL_FATAL_ERROR; } - - /* skip the total length uint16_t */ - i = 2; + configIdx = 2; do { - echConfig = (byte*)echConfigs + i; + if (configIdx + 4 > echConfigsLen) { + ret = BUFFER_E; + break; + } + echConfig = echConfigs + configIdx; ato16(echConfig, &version); ato16(echConfig + 2, &length); - /* if the version does not match */ - if (version != TLSX_ECH) { - /* we hit the end of the configs */ - if ( (word32)i + 2 >= echConfigsLen ) { - break; - } - - /* skip this config, +4 for version and length */ - i += length + 4; - continue; - } - - /* check if the length will overrun the buffer */ - if ((word32)i + length + 4 > echConfigsLen) { + if (configIdx + length + 4 > echConfigsLen) { + ret = BUFFER_E; break; } + else if (version != TLSX_ECH) { + /* skip this config and try the next one */ + configIdx += length + 4; + continue; + } if (workingConfig == NULL) { workingConfig = (WOLFSSL_EchConfig*)XMALLOC(sizeof(WOLFSSL_EchConfig), heap, - DYNAMIC_TYPE_TMP_BUFFER); + DYNAMIC_TYPE_TMP_BUFFER); configList = workingConfig; - if (workingConfig != NULL) { - workingConfig->next = NULL; - } } else { lastConfig = workingConfig; workingConfig->next = - (WOLFSSL_EchConfig*)XMALLOC(sizeof(WOLFSSL_EchConfig), - heap, DYNAMIC_TYPE_TMP_BUFFER); + (WOLFSSL_EchConfig*)XMALLOC(sizeof(WOLFSSL_EchConfig), heap, + DYNAMIC_TYPE_TMP_BUFFER); workingConfig = workingConfig->next; } @@ -566,8 +559,8 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, workingConfig->rawLen = length + 4; /* raw body */ - workingConfig->raw = (byte*)XMALLOC(workingConfig->rawLen, - heap, DYNAMIC_TYPE_TMP_BUFFER); + workingConfig->raw = (byte*)XMALLOC(workingConfig->rawLen, heap, + DYNAMIC_TYPE_TMP_BUFFER); if (workingConfig->raw == NULL) { ret = MEMORY_E; break; @@ -578,8 +571,14 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, /* skip over version and length */ echConfig += 4; + idx = 5; + if (idx >= length) { + ret = BUFFER_E; + break; + } + /* configId, 1 byte */ - workingConfig->configId = *(echConfig); + workingConfig->configId = *echConfig; echConfig++; /* kemId, 2 bytes */ ato16(echConfig, &workingConfig->kemId); @@ -587,15 +586,41 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, /* hpke public_key length, 2 bytes */ ato16(echConfig, &hpkePubkeyLen); echConfig += 2; + /* hpke public_key */ - if (hpkePubkeyLen > HPKE_Npk_MAX) { + if (hpkePubkeyLen > HPKE_Npk_MAX || hpkePubkeyLen == 0) { ret = BUFFER_E; break; } + idx += hpkePubkeyLen; + if (idx >= length) { + ret = BUFFER_E; + break; + } + XMEMCPY(workingConfig->receiverPubkey, echConfig, hpkePubkeyLen); echConfig += hpkePubkeyLen; + /* cipherSuitesLen */ + idx += 2; + if (idx >= length) { + ret = BUFFER_E; + break; + } ato16(echConfig, &cipherSuitesLen); + if (cipherSuitesLen == 0 || cipherSuitesLen % 4 != 0 || + cipherSuitesLen >= 1024) { + /* numCipherSuites is a byte so only 256 ciphersuites (each 4 bytes) + * can be accessed */ + ret = BUFFER_E; + break; + } + + idx += cipherSuitesLen; + if (idx >= length) { + ret = BUFFER_E; + break; + } workingConfig->cipherSuites = (EchCipherSuite*)XMALLOC(cipherSuitesLen, heap, DYNAMIC_TYPE_TMP_BUFFER); @@ -605,32 +630,68 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, } echConfig += 2; - workingConfig->numCipherSuites = cipherSuitesLen / 4; + workingConfig->numCipherSuites = (byte)(cipherSuitesLen / 4); /* cipherSuites */ for (j = 0; j < workingConfig->numCipherSuites; j++) { - ato16(echConfig + j * 4, &workingConfig->cipherSuites[j].kdfId); - ato16(echConfig + j * 4 + 2, - &workingConfig->cipherSuites[j].aeadId); + ato16(echConfig, &workingConfig->cipherSuites[j].kdfId); + ato16(echConfig + 2, &workingConfig->cipherSuites[j].aeadId); + echConfig += 4; } - echConfig += cipherSuitesLen; + /* ignore the maximum name length */ + idx++; + if (idx >= length) { + ret = BUFFER_E; + break; + } echConfig++; + /* publicNameLen */ - publicNameLen = *(echConfig); + idx++; + if (idx >= length) { + ret = BUFFER_E; + break; + } + + publicNameLen = *echConfig; + if (publicNameLen == 0) { + ret = BUFFER_E; + break; + } + + idx += publicNameLen; + if (idx >= length) { + ret = BUFFER_E; + break; + } + echConfig++; + workingConfig->publicName = (char*)XMALLOC(publicNameLen + 1, heap, DYNAMIC_TYPE_TMP_BUFFER); if (workingConfig->publicName == NULL) { ret = MEMORY_E; break; } - echConfig++; + /* publicName */ XMEMCPY(workingConfig->publicName, echConfig, publicNameLen); - /* null terminated */ - workingConfig->publicName[publicNameLen] = 0; + workingConfig->publicName[publicNameLen] = '\0'; + echConfig += publicNameLen; - /* add length to go to next config, +4 for version and length */ - i += length + 4; + /* TODO: Parse ECHConfigExtension */ + /* --> for now just ignore it */ + idx += 2; + if (idx > length) { + ret = BUFFER_E; + break; + } + ato16(echConfig, &extensionsLen); + + idx += extensionsLen; + if (idx != length) { + ret = BUFFER_E; + break; + } /* check that we support this config */ for (j = 0; j < HPKE_SUPPORTED_KEM_LEN; j++) { @@ -638,18 +699,31 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, break; } - /* if we don't support the kem or at least one cipher suite */ + /* KEM or ciphersuite not supported, free this config and then try to + * parse another */ if (j >= HPKE_SUPPORTED_KEM_LEN || - EchConfigGetSupportedCipherSuite(workingConfig) < 0) - { - XFREE(workingConfig->cipherSuites, heap, - DYNAMIC_TYPE_TMP_BUFFER); - XFREE(workingConfig->publicName, heap, - DYNAMIC_TYPE_TMP_BUFFER); + EchConfigGetSupportedCipherSuite(workingConfig) < 0) { + XFREE(workingConfig->cipherSuites, heap, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(workingConfig->publicName, heap, DYNAMIC_TYPE_TMP_BUFFER); XFREE(workingConfig->raw, heap, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(workingConfig, heap, DYNAMIC_TYPE_TMP_BUFFER); workingConfig = lastConfig; + + if (workingConfig != NULL) { + workingConfig->next = NULL; + } + else { + /* if one (or more) of the leading configs are unsupported then + * this case will be hit */ + configList = NULL; + } } - } while ((word32)i < echConfigsLen); + + configIdx += 4 + length; + } while (configIdx < echConfigsLen); + if (ret == 0 && configIdx != echConfigsLen){ + ret = BUFFER_E; + } /* if we found valid configs */ if (ret == 0 && configList != NULL) { @@ -659,7 +733,6 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, } workingConfig = configList; - while (workingConfig != NULL) { lastConfig = workingConfig; workingConfig = workingConfig->next; @@ -667,7 +740,6 @@ int SetEchConfigsEx(WOLFSSL_EchConfig** outputConfigs, void* heap, XFREE(lastConfig->cipherSuites, heap, DYNAMIC_TYPE_TMP_BUFFER); XFREE(lastConfig->publicName, heap, DYNAMIC_TYPE_TMP_BUFFER); XFREE(lastConfig->raw, heap, DYNAMIC_TYPE_TMP_BUFFER); - XFREE(lastConfig, heap, DYNAMIC_TYPE_TMP_BUFFER); } diff --git a/tests/api.c b/tests/api.c index 2fb5478495..f0b7c88b5d 100644 --- a/tests/api.c +++ b/tests/api.c @@ -14069,9 +14069,6 @@ static int test_wolfSSL_Tls13_ECH_params(void) EXPECT_DECLS; #if !defined(NO_WOLFSSL_CLIENT) byte testBuf[256]; - /* base64 ech configs from cloudflare-ech.com */ - const char* b64Configs = - "AEX+DQBBFAAgACBuAoQI8+liEVYQbXKBDeVgTmF2rfXuKO2knhwrN7jgTgAEAAEAAQASY2xvdWRmbGFyZS1lY2guY29tAAA="; word32 outputLen = sizeof(testBuf); word16 tmpLen = 0; WOLFSSL_CTX* ctx = wolfSSL_CTX_new(wolfTLSv1_3_client_method()); @@ -14080,7 +14077,7 @@ static int test_wolfSSL_Tls13_ECH_params(void) ExpectNotNull(ctx); ExpectNotNull(ssl); - /* CTX NULL errors */ + /* generation errors */ /* invalid ctx */ ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_GenerateEchConfig(NULL, @@ -14096,36 +14093,22 @@ static int test_wolfSSL_Tls13_ECH_params(void) ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_GenerateEchConfig(ctx, "ech-public-name.com", 0, 0, 1000)); - /* invalid base64 configs: NULL ctx, NULL configs, 0 length */ - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(NULL, - b64Configs, (word32)XSTRLEN(b64Configs))); - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, - NULL, (word32)XSTRLEN(b64Configs))); - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, - b64Configs, 0)); + /* bad function calls */ - /* invalid configs: NULL ctx, NULL configs, 0 length */ + /* NULL ctx */ ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigs(NULL, testBuf, sizeof(testBuf))); - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigs(ctx, NULL, - sizeof(testBuf))); - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigs(ctx, testBuf, 0)); - - /* SSL NULL errors */ - - /* invalid base64 configs: NULL ssl, NULL configs, 0 length */ - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(NULL, b64Configs, - (word32)XSTRLEN(b64Configs))); - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, NULL, - (word32)XSTRLEN(b64Configs))); - ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, b64Configs, - 0)); - - /* invalid configs: NULL ssl, NULL configs, 0 length */ ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigs(NULL, testBuf, sizeof(testBuf))); + + /* NULL configs */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigs(ctx, NULL, + sizeof(testBuf))); ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigs(ssl, NULL, sizeof(testBuf))); + + /* 0 length */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigs(ctx, testBuf, 0)); ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigs(ssl, testBuf, 0)); /* stateful errors */ @@ -14213,15 +14196,103 @@ static int test_wolfSSL_Tls13_ECH_params(void) ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_GetEchConfigs(ssl, testBuf, &outputLen)); + wolfSSL_free(ssl); + wolfSSL_CTX_free(ctx); +#endif /* !NO_WOLFSSL_CLIENT */ + + return EXPECT_RESULT(); +} + +static int test_wolfSSL_Tls13_ECH_params_b64(void) +{ + EXPECT_DECLS; +#if !defined(NO_WOLFSSL_CLIENT) + /* base64 ech configs from cloudflare-ech.com (these are good configs) */ + const char* b64Valid = "AEX+DQBBFAAgACBuAoQI8+liEVYQbXKBDeVgTmF2rfXuKO2knhwrN7jgTgAEAAEAAQASY2xvdWRmbGFyZS1lY2guY29tAAA="; + /* ech configs with bad/unsupported algorithm */ + const char* b64BadAlgo = "AEX+DQBBFP//ACBuAoQI8+liEVYQbXKBDeVgTmF2rfXuKO2knhwrN7jgTgAEAAEAAQASY2xvdWRmbGFyZS1lY2guY29tAAA="; + /* ech configs with bad/unsupported ciphersuite */ + const char* b64BadCiph = "AEX+DQBBFAAgACBuAoQI8+liEVYQbXKBDeVgTmF2rfXuKO2knhwrN7jgTgAE//8AAQASY2xvdWRmbGFyZS1lY2guY29tAAA="; + /* ech configs with bad version first */ + const char* b64BadVers1 = "AIz+HQBCAQAgACCjR6+Qn9UYkMaWdXZzsby88vXFhPHJ2tWCDHQJLvMkEgAEAAEAAQATZWNoLXB1YmxpYy1uYW1lLmNvbQAA/g0AQgIAIAAgMM6vLrTbOfsfA6fTbJY/Iu0Lj2xeHEPGUJeUwQGAYF4ABAABAAEAE2VjaC1wdWJsaWMtbmFtZS5jb20AAA=="; + /* ech configs with bad version second */ + const char* b64BadVers2 = "AIz+DQBCAQAgACCjR6+Qn9UYkMaWdXZzsby88vXFhPHJ2tWCDHQJLvMkEgAEAAEAAQATZWNoLXB1YmxpYy1uYW1lLmNvbQAA/h0AQgIAIAAgMM6vLrTbOfsfA6fTbJY/Iu0Lj2xeHEPGUJeUwQGAYF4ABAABAAEAE2VjaC1wdWJsaWMtbmFtZS5jb20AAA=="; + byte testBuf[256]; + word32 outputLen; + + WOLFSSL_CTX* ctx = wolfSSL_CTX_new(wolfTLSv1_3_client_method()); + WOLFSSL* ssl = wolfSSL_new(ctx); + + ExpectNotNull(ctx); + ExpectNotNull(ssl); + + /* NULL ctx/ssl, short public key */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(NULL, + b64Valid, (word32)XSTRLEN(b64Valid))); + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(NULL, + b64Valid, (word32)XSTRLEN(b64Valid))); + + /* NULL configs */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, + NULL, (word32)XSTRLEN(b64Valid))); + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, + NULL, (word32)XSTRLEN(b64Valid))); + + /* 0 length */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, + b64Valid, 0)); + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, + b64Valid, 0)); + + /* bad algorithm */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, + b64BadAlgo, (word32)XSTRLEN(b64BadAlgo))); + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, + b64BadAlgo, (word32)XSTRLEN(b64BadAlgo))); + + /* bad ciphersuite */ + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, + b64BadCiph, (word32)XSTRLEN(b64BadCiph))); + ExpectIntNE(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, + b64BadCiph, (word32)XSTRLEN(b64BadCiph))); + + /* bad version first, should only have config 2 set */ + ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, + b64BadVers1, (word32)XSTRLEN(b64BadVers1))); + ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, + b64BadVers1, (word32)XSTRLEN(b64BadVers1))); + ExpectIntEQ(2, ctx->echConfigs->configId); + ExpectIntEQ(2, ssl->echConfigs->configId); + + /* clear configs */ + wolfSSL_CTX_SetEchEnable(ctx, 0); + wolfSSL_CTX_SetEchEnable(ctx, 1); + wolfSSL_SetEchEnable(ssl, 0); + wolfSSL_SetEchEnable(ssl, 1); + + /* bad version second, should only have config 1 set */ + ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, + b64BadVers2, (word32)XSTRLEN(b64BadVers2))); + ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, + b64BadVers2, (word32)XSTRLEN(b64BadVers2))); + ExpectIntEQ(1, ctx->echConfigs->configId); + ExpectIntEQ(1, ssl->echConfigs->configId); + + /* clear configs */ + wolfSSL_CTX_SetEchEnable(ctx, 0); + wolfSSL_CTX_SetEchEnable(ctx, 1); + wolfSSL_SetEchEnable(ssl, 0); + wolfSSL_SetEchEnable(ssl, 1); + /* base64 tests */ /* set base64 configs */ ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_CTX_SetEchConfigsBase64(ctx, - b64Configs, (word32)XSTRLEN(b64Configs))); + b64Valid, (word32)XSTRLEN(b64Valid))); ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_SetEchConfigsBase64(ssl, - b64Configs, (word32)XSTRLEN(b64Configs))); + b64Valid, (word32)XSTRLEN(b64Valid))); - /* disable and check ctx has no configs as well */ + /* disable and check ctx has no configs */ wolfSSL_CTX_SetEchEnable(ctx, 0); outputLen = sizeof(testBuf); ExpectIntNE(WOLFSSL_SUCCESS, @@ -34136,6 +34207,7 @@ TEST_CASE testCases[] = { #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) #if defined(HAVE_IO_TESTS_DEPENDENCIES) TEST_DECL(test_wolfSSL_Tls13_ECH_params), + TEST_DECL(test_wolfSSL_Tls13_ECH_params_b64), /* Uses Assert in handshake callback. */ TEST_DECL(test_wolfSSL_Tls13_ECH), TEST_DECL(test_wolfSSL_Tls13_ECH_HRR),