diff --git a/src/tls.c b/src/tls.c index 8fcfce21f0..1649badd81 100644 --- a/src/tls.c +++ b/src/tls.c @@ -13485,6 +13485,74 @@ static int TLSX_ECH_GetSize(WOLFSSL_ECH* ech, byte msgType) return (int)size; } +/* rough check that inner hello fields do not exceed length of decrypted + * information. Additionally, this function will check that all padding bytes + * are zero and decrease the innerHelloLen accordingly if so. + * returns 0 on success and otherwise failure */ +static int TLSX_ECH_CheckInnerPadding(WOLFSSL* ssl, WOLFSSL_ECH* ech) +{ + int headerSz; + const byte* innerCh; + word32 innerChLen; + word32 idx; + byte sessionIdLen; + word16 cipherSuitesLen; + byte compressionLen; + word16 extLen; + byte acc = 0; + word32 i; + +#ifdef WOLFSSL_DTLS13 + headerSz = ssl->options.dtls ? DTLS13_HANDSHAKE_HEADER_SZ : + HANDSHAKE_HEADER_SZ; +#else + headerSz = HANDSHAKE_HEADER_SZ; +#endif + + innerCh = ech->innerClientHello + headerSz; + innerChLen = ech->innerClientHelloLen; + + idx = OPAQUE16_LEN + RAN_LEN; + if (idx >= innerChLen) + return BUFFER_ERROR; + + sessionIdLen = innerCh[idx++]; + /* innerHello sessionID must initially be empty */ + if (sessionIdLen != 0) + return INVALID_PARAMETER; + idx += sessionIdLen; + if (idx + OPAQUE16_LEN > innerChLen) + return BUFFER_ERROR; + + ato16(innerCh + idx, &cipherSuitesLen); + idx += OPAQUE16_LEN + cipherSuitesLen; + if (idx >= innerChLen) + return BUFFER_ERROR; + + compressionLen = innerCh[idx++]; + idx += compressionLen; + if (idx + OPAQUE16_LEN > innerChLen) + return BUFFER_ERROR; + + ato16(innerCh + idx, &extLen); + idx += OPAQUE16_LEN + extLen; + if (idx > innerChLen) + return BUFFER_ERROR; + + /* should now be at the end of the innerHello + * Per ECH spec all padding bytes MUST be 0 */ + for (i = idx; i < innerChLen; i++) { + acc |= innerCh[i]; + } + if (acc != 0) { + SendAlert(ssl, alert_fatal, illegal_parameter); + return INVALID_PARAMETER; + } + + ech->innerClientHelloLen -= i - idx; + return 0; +} + /* Locate the given extension type, use the extOffset to start off after where a * previous call to this function ended * @@ -13536,7 +13604,7 @@ static const byte* TLSX_ECH_FindOuterExtension(const byte* outerCh, return NULL; } - while (idx < chLen && (idx - *extensionsStart) < *extensionsLen) { + while (idx - *extensionsStart < *extensionsLen) { if (idx + OPAQUE16_LEN + OPAQUE16_LEN > chLen) return NULL; @@ -13545,7 +13613,7 @@ static const byte* TLSX_ECH_FindOuterExtension(const byte* outerCh, ato16(outerCh + idx, &len); idx += OPAQUE16_LEN; - if (idx + len > chLen) + if (idx + len - *extensionsStart > *extensionsLen) return NULL; if (type == extType) { @@ -13688,33 +13756,22 @@ static int TLSX_ECH_ExpandOuterExtensions(WOLFSSL* ssl, WOLFSSL_ECH* ech, outerCh = ech->aad; outerChLen = ech->aadLen; + /* don't need to check for buffer overflows here since they are caught by + * TLSX_ECH_CheckInnerPadding */ idx = OPAQUE16_LEN + RAN_LEN; - if (idx >= innerChLen) - return BUFFER_ERROR; sessionIdLen = innerCh[idx++]; idx += sessionIdLen; - /* the ECH spec details that innerhello sessionID must initially be empty */ - if (sessionIdLen != 0) - return INVALID_PARAMETER; - if (idx + OPAQUE16_LEN > innerChLen) - return BUFFER_ERROR; ato16(innerCh + idx, &cipherSuitesLen); idx += OPAQUE16_LEN + cipherSuitesLen; - if (idx >= innerChLen) - return BUFFER_ERROR; compressionLen = innerCh[idx++]; idx += compressionLen; - if (idx + OPAQUE16_LEN > innerChLen) - return BUFFER_ERROR; ato16(innerCh + idx, &innerExtLen); idx += OPAQUE16_LEN; innerExtIdx = idx; - if (idx + innerExtLen > innerChLen) - return BUFFER_ERROR; /* validate ech_outer_extensions and calculate extra size */ while (idx < innerChLen && (idx - innerExtIdx) < innerExtLen) { @@ -13957,7 +14014,6 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size, byte msgType) { int ret = 0; - int i; TLSX* echX; WOLFSSL_ECH* ech; WOLFSSL_EchConfig* echConfig; @@ -13965,6 +14021,7 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size, byte* readBuf_p = (byte*)readBuf; word32 offset = 0; word16 len; + word16 tmpVal16; WOLFSSL_MSG("TLSX_ECH_Parse"); if (ssl->options.disableECH) { @@ -14007,39 +14064,74 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size, ech->state = ECH_PARSED_INTERNAL; return 0; } + else if (ech->type != ECH_TYPE_OUTER) { + /* type MUST be INNER or OUTER */ + return BAD_FUNC_ARG; + } /* Must have kdfId, aeadId, configId, enc len and payload len. */ if (size < offset + 2 + 2 + 1 + 2 + 2) { return BUFFER_ERROR; } - /* read kdfId */ - ato16(readBuf_p, &ech->cipherSuite.kdfId); - readBuf_p += 2; - offset += 2; - /* read aeadId */ - ato16(readBuf_p, &ech->cipherSuite.aeadId); - readBuf_p += 2; - offset += 2; - /* read configId */ - ech->configId = *readBuf_p; - readBuf_p++; - offset++; - /* read encLen */ - ato16(readBuf_p, &len); - readBuf_p += 2; - offset += 2; - /* Check encLen isn't more than remaining bytes minus payload length. */ - if (len > size - offset - 2) { - return BAD_FUNC_ARG; - } - if (len > HPKE_Npk_MAX) { - return BAD_FUNC_ARG; - } /* only get enc if we don't already have the hpke context */ if (ech->hpkeContext == NULL) { + /* kdfId */ + ato16(readBuf_p, &ech->cipherSuite.kdfId); + readBuf_p += 2; + offset += 2; + /* aeadId */ + ato16(readBuf_p, &ech->cipherSuite.aeadId); + readBuf_p += 2; + offset += 2; + /* configId */ + ech->configId = *readBuf_p; + readBuf_p++; + offset++; + /* encLen */ + ato16(readBuf_p, &len); + readBuf_p += 2; + offset += 2; + /* Check encLen isn't more than remaining bytes minus + * payload length. */ + if (len > size - offset - 2) { + return BAD_FUNC_ARG; + } + if (len > HPKE_Npk_MAX) { + return BAD_FUNC_ARG; + } /* read enc */ XMEMCPY(ech->enc, readBuf_p, len); ech->encLen = len; } + else { + /* kdfId, aeadId, and configId must be the same as last time */ + /* kdfId */ + ato16(readBuf_p, &tmpVal16); + if (tmpVal16 != ech->cipherSuite.kdfId) { + return BAD_FUNC_ARG; + } + readBuf_p += 2; + offset += 2; + /* aeadId */ + ato16(readBuf_p, &tmpVal16); + if (tmpVal16 != ech->cipherSuite.aeadId) { + return BAD_FUNC_ARG; + } + readBuf_p += 2; + offset += 2; + /* configId */ + if (*readBuf_p != ech->configId) { + return BAD_FUNC_ARG; + } + readBuf_p++; + offset++; + /* on an HRR the enc value MUST be empty */ + ato16(readBuf_p, &len); + if (len != 0) { + return BAD_FUNC_ARG; + } + readBuf_p += 2; + offset += 2; + } readBuf_p += len; offset += len; /* read hello inner len */ @@ -14098,19 +14190,12 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size, } } if (ret == 0) { - i = 0; - /* decrement until before the padding */ - /* TODO: verify padding is 0, abort with illegal_parameter */ - while (ech->innerClientHello[ech->innerClientHelloLen + - HANDSHAKE_HEADER_SZ - i - 1] != ECH_TYPE_INNER) { - i++; + ret = TLSX_ECH_CheckInnerPadding(ssl, ech); + if (ret == 0) { + /* expand EchOuterExtensions if present. + * Also, if it exists, copy sessionID from outer hello */ + ret = TLSX_ECH_ExpandOuterExtensions(ssl, ech, ssl->heap); } - /* subtract the length of the padding from the length */ - ech->innerClientHelloLen -= i; - - /* expand EchOuterExtensions if present - * and, if it exists, copy sessionID from outer hello */ - ret = TLSX_ECH_ExpandOuterExtensions(ssl, ech, ssl->heap); } /* if we failed to extract/expand, set state to retry configs */ if (ret != 0) { diff --git a/tests/api.c b/tests/api.c index 32213f57ec..1db9a34d07 100644 --- a/tests/api.c +++ b/tests/api.c @@ -14074,7 +14074,7 @@ static int test_wolfSSL_Tls13_ECH_params(void) const char* b64Configs = "AEX+DQBBFAAgACBuAoQI8+liEVYQbXKBDeVgTmF2rfXuKO2knhwrN7jgTgAEAAEAAQASY2xvdWRmbGFyZS1lY2guY29tAAA="; word32 outputLen = sizeof(testBuf); - word16 tmpLen; + word16 tmpLen = 0; WOLFSSL_CTX* ctx = wolfSSL_CTX_new(wolfTLSv1_3_client_method()); WOLFSSL* ssl = wolfSSL_new(ctx); @@ -14619,9 +14619,9 @@ static int test_wolfSSL_Tls13_ECH_new_config(void) word32 altConfigLen = sizeof(altConfig); byte combinedConfigs[512]; word32 combinedConfigsLen = sizeof(combinedConfigs); - word16 firstConfigLen; - word16 secondConfigOffset; - word16 secondConfigLen; + word16 firstConfigLen = 0; + word16 secondConfigOffset = 0; + word16 secondConfigLen = 0; XMEMSET(&test_ctx, 0, sizeof(test_ctx));