Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -38894,6 +38894,13 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
}

#ifdef HAVE_TLS_EXTENSIONS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] peerNoUncompPF reset skipped when ClientHello has no extensions block

The defensive reset ssl->options.peerNoUncompPF = 0; is placed inside the if ((i - begin) < helloSz) extensions-present block (and inside the TLSX_SupportExtensions(ssl) block), immediately before TLSX_Parse. If a subsequent ClientHello on the same WOLFSSL object carries no extensions block at all, the reset is not executed and a stale peerNoUncompPF=1 from an earlier handshake persists; if an ECC suite is then negotiated, the new RFC 8422 check would fire a spurious illegal_parameter abort. In practice this is not reachable for secure renegotiation (RFC 5746 requires the renegotiation_info extension, so the extensions block is always present and the reset always runs), so the stated motivation in the comment is satisfied. It is only theoretically reachable with unsafe legacy renegotiation enabled and a no-extensions ClientHello that still negotiates ECC. Flagged for robustness only.

Fix: Optionally move the reset earlier to guarantee it runs for every ClientHello regardless of whether an extensions block is present. Low priority since secure renegotiation always carries an extensions block.

#ifdef HAVE_SUPPORTED_CURVES
/* Reset per-ClientHello extension state before (re)parsing so a
* stale value from an earlier handshake on this object (e.g.
* secure renegotiation, where Options is not zeroed) cannot
* trigger a spurious RFC 8422 abort below. */
ssl->options.peerNoUncompPF = 0;
#endif
/* tls extensions */
if ((ret = TLSX_Parse(ssl, input + i, totalExtSz, client_hello,
ssl->clSuites)))
Expand Down Expand Up @@ -39073,6 +39080,25 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
if (ret == 0)
ret = MatchSuite(ssl, ssl->clSuites);

#if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_SUPPORTED_CURVES)
/* RFC 8422 Section 5.1.2: abort only when an ECC suite was actually
* negotiated and the client's ec_point_formats omitted the uncompressed
* (0) format (peerNoUncompPF, set in TLSX_PointFormat_Parse). Checked
* after MatchSuite so it keys off the chosen suite, not advertised
* groups. */
if (ret == 0 && ssl->options.peerNoUncompPF &&
(ssl->specs.kea == ecc_diffie_hellman_kea ||
ssl->specs.kea == ecc_static_diffie_hellman_kea ||
ssl->specs.kea == ecdhe_psk_kea)) {
WOLFSSL_MSG("Client ec_point_formats extension missing "
"uncompressed format for negotiated ECC suite");
SendAlert(ssl, alert_fatal, illegal_parameter);
ret = INVALID_PARAMETER;
WOLFSSL_ERROR_VERBOSE(ret);
goto out;
}
#endif

#if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_ENCRYPT_THEN_MAC) && \
!defined(WOLFSSL_AEAD_ONLY)
if (ret == 0 && ssl->options.encThenMac &&
Expand Down
37 changes: 35 additions & 2 deletions src/tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -5222,8 +5222,13 @@ int TLSX_SupportedCurve_Parse(const WOLFSSL* ssl, const byte* input,
if (length != OPAQUE16_LEN + offset)
return BUFFER_ERROR;
offset = OPAQUE16_LEN;
if (offset == length)
return 0;
if (offset == length) {
/* An empty named group list is malformed (named_group_list<2..2^16-1>,
* RFC 8422 / RFC 8446). BUFFER_ERROR yields a decode_error alert (see
* TranslateErrorToAlert()). Accepting it would also make an explicit
* empty extension look absent and impose no group restriction. */
return BUFFER_ERROR;
}

extension = TLSX_Find(*extensions, TLSX_SUPPORTED_GROUPS);
if (extension == NULL) {
Expand All @@ -5240,6 +5245,14 @@ int TLSX_SupportedCurve_Parse(const WOLFSSL* ssl, const byte* input,
break;
ret = 0;
}
/* All advertised groups are unsupported, so no node was added above.
* Record an empty node so suite selection still sees the restriction
* (e.g. ECC/ECDHE must not be chosen) instead of treating the
* extension as absent. */
if (ret == 0 && isRequest &&
TLSX_Find(*extensions, TLSX_SUPPORTED_GROUPS) == NULL) {
ret = TLSX_Push(extensions, TLSX_SUPPORTED_GROUPS, NULL, ssl->heap);
}
}
else {
/* Find the intersection with what the user has set */
Expand Down Expand Up @@ -5644,6 +5657,26 @@ static int TLSX_PointFormat_Parse(WOLFSSL* ssl, const byte* input,
return BUFFER_ERROR;

if (isRequest) {
#if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_SUPPORTED_CURVES)
/* RFC 8422 Section 5.1.2: a client that sends the ec_point_formats
* extension MUST include the uncompressed (0) format. Record whether
* it is missing so DoClientHello() can abort with an illegal_parameter
* alert if the client also advertised ECC named groups. The decision
* is deferred to after all extensions are parsed so it does not depend
* on the relative order of the supported_groups and ec_point_formats
* extensions in the ClientHello. */
word16 i;
int found = 0;

for (i = 0; i < input[0]; i++) {
if (input[ENUM_LEN + i] == WOLFSSL_EC_PF_UNCOMPRESSED) {
found = 1;
break;
}
}
ssl->options.peerNoUncompPF = (found == 0);
#endif

/* adding uncompressed point format to response */
ret = TLSX_UsePointFormat(&ssl->extensions, WOLFSSL_EC_PF_UNCOMPRESSED,
ssl->heap);
Expand Down
2 changes: 2 additions & 0 deletions tests/api.c
Original file line number Diff line number Diff line change
Expand Up @@ -35137,6 +35137,8 @@ TEST_CASE testCases[] = {
TEST_DECL(test_TLSX_SNI_GetSize_overflow),
TEST_DECL(test_TLSX_ECH_msg_type_validation),
TEST_DECL(test_TLSX_SRTP_msg_type_validation),
TEST_DECL(test_TLSX_SupportedCurve_empty_or_unsupported),
TEST_DECL(test_TLSX_PointFormat_uncompressed_required),
TEST_DECL(test_wolfSSL_wolfSSL_UseSecureRenegotiation),
TEST_DECL(test_wolfSSL_clear_secure_renegotiation),
TEST_DECL(test_wolfSSL_SCR_Reconnect),
Expand Down
37 changes: 37 additions & 0 deletions tests/api/test_arc4.c
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,43 @@ int test_wc_Arc4Process(void)

} /* END test_wc_Arc4Process */

/*
* Regression test for issue 5378: wc_Arc4Process must refuse to run unless a
* key has been configured with wc_Arc4SetKey(), both before SetKey and after
* Free. Otherwise the keystream is all-zero and ARC4 silently copies the
* plaintext into the ciphertext.
*/
int test_wc_Arc4Process_no_key(void)
{
EXPECT_DECLS;
#ifndef NO_RC4
Arc4 arc4;
const char* key = "\x01\x23\x45\x67\x89\xab\xcd\xef";
int keyLen = 8;
const char* input = "\x01\x23\x45\x67\x89\xab\xcd\xef";
byte cipher[8];

XMEMSET(&arc4, 0, sizeof(arc4));
XMEMSET(cipher, 0, sizeof(cipher));

/* Processing without a key after init must fail, not silently copy. */
ExpectIntEQ(wc_Arc4Init(&arc4, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_Arc4Process(&arc4, cipher, (byte*)input, (word32)keyLen),
WC_NO_ERR_TRACE(MISSING_KEY));

/* After a key is set, processing succeeds. */
ExpectIntEQ(wc_Arc4SetKey(&arc4, (byte*)key, (word32)keyLen), 0);
ExpectIntEQ(wc_Arc4Process(&arc4, cipher, (byte*)input, (word32)keyLen), 0);

/* After free, the keyed state is cleared and processing must fail again. */
wc_Arc4Free(&arc4);
ExpectIntEQ(wc_Arc4Process(&arc4, cipher, (byte*)input, (word32)keyLen),
WC_NO_ERR_TRACE(MISSING_KEY));
#endif
return EXPECT_RESULT();

} /* END test_wc_Arc4Process_no_key */


#include <wolfssl/wolfcrypt/random.h>

Expand Down
2 changes: 2 additions & 0 deletions tests/api/test_arc4.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@

int test_wc_Arc4SetKey(void);
int test_wc_Arc4Process(void);
int test_wc_Arc4Process_no_key(void);
int test_wc_Arc4_MonteCarlo(void);

#define TEST_ARC4_DECLS \
TEST_DECL_GROUP("arc4", test_wc_Arc4SetKey), \
TEST_DECL_GROUP("arc4", test_wc_Arc4Process), \
TEST_DECL_GROUP("arc4", test_wc_Arc4Process_no_key), \
TEST_DECL_GROUP("arc4", test_wc_Arc4_MonteCarlo)

#endif /* WOLFCRYPT_TEST_ARC4_H */
61 changes: 61 additions & 0 deletions tests/api/test_des3.c
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,67 @@ int test_wc_Des3_CbcEncryptDecrypt(void)

} /* END wc_Des3_CbcEncrypt */

/*
* Regression test for issue 5379: wc_Des3_CbcEncrypt/Decrypt must refuse to
* run unless a key has been configured with wc_Des3_SetKey(), both before
* SetKey and after Free. Otherwise the operation would silently run with
* uninitialized or zeroed key material and return success.
*
* FIPS builds use the FIPS-certified DES3 implementation which does not track
* key state, so skip the test for FIPS.
*/
int test_wc_Des3_CbcEncryptDecrypt_no_key(void)
{
EXPECT_DECLS;
#if !defined(NO_DES3) && !defined(HAVE_FIPS)
Des3 des;
byte cipher[24];
byte plain[24];
const byte key[] = {
0x01,0x23,0x45,0x67,0x89,0xab,0xcd,0xef,
0xfe,0xde,0xba,0x98,0x76,0x54,0x32,0x10,
0x89,0xab,0xcd,0xef,0x01,0x23,0x45,0x67
};
const byte iv[] = {
0x12,0x34,0x56,0x78,0x90,0xab,0xcd,0xef,
0x01,0x01,0x01,0x01,0x01,0x01,0x01,0x01,
0x11,0x21,0x31,0x41,0x51,0x61,0x71,0x81
};
const byte vector[] = { /* "Now is the time for all " w/o trailing 0 */
0x4e,0x6f,0x77,0x20,0x69,0x73,0x20,0x74,
0x68,0x65,0x20,0x74,0x69,0x6d,0x65,0x20,
0x66,0x6f,0x72,0x20,0x61,0x6c,0x6c,0x20
};

XMEMSET(&des, 0, sizeof(Des3));
XMEMSET(cipher, 0, sizeof(cipher));
XMEMSET(plain, 0, sizeof(plain));

/* Encrypt/decrypt without a key after init must fail, not silently run. */
ExpectIntEQ(wc_Des3Init(&des, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_Des3_CbcEncrypt(&des, cipher, vector, 24),
WC_NO_ERR_TRACE(MISSING_KEY));
ExpectIntEQ(wc_Des3_CbcDecrypt(&des, plain, vector, 24),
WC_NO_ERR_TRACE(MISSING_KEY));

/* After a key is set, the operations succeed. */
ExpectIntEQ(wc_Des3_SetKey(&des, key, iv, DES_ENCRYPTION), 0);
ExpectIntEQ(wc_Des3_CbcEncrypt(&des, cipher, vector, 24), 0);
ExpectIntEQ(wc_Des3_SetKey(&des, key, iv, DES_DECRYPTION), 0);
ExpectIntEQ(wc_Des3_CbcDecrypt(&des, plain, cipher, 24), 0);
ExpectIntEQ(XMEMCMP(plain, vector, 24), 0);

/* After free, the keyed state is cleared and operations must fail again. */
wc_Des3Free(&des);
ExpectIntEQ(wc_Des3_CbcEncrypt(&des, cipher, vector, 24),
WC_NO_ERR_TRACE(MISSING_KEY));
ExpectIntEQ(wc_Des3_CbcDecrypt(&des, plain, vector, 24),
WC_NO_ERR_TRACE(MISSING_KEY));
#endif
return EXPECT_RESULT();

} /* END test_wc_Des3_CbcEncryptDecrypt_no_key */

/*
* Unit test for wc_Des3_EcbEncrypt
*/
Expand Down
12 changes: 7 additions & 5 deletions tests/api/test_des3.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,16 @@
int test_wc_Des3_SetIV(void);
int test_wc_Des3_SetKey(void);
int test_wc_Des3_CbcEncryptDecrypt(void);
int test_wc_Des3_CbcEncryptDecrypt_no_key(void);
int test_wc_Des3_EcbEncrypt(void);
int test_wc_Des3Cbc_MonteCarlo(void);

#define TEST_DES3_DECLS \
TEST_DECL_GROUP("des3", test_wc_Des3_SetIV), \
TEST_DECL_GROUP("des3", test_wc_Des3_SetKey), \
TEST_DECL_GROUP("des3", test_wc_Des3_CbcEncryptDecrypt), \
TEST_DECL_GROUP("des3", test_wc_Des3_EcbEncrypt), \
#define TEST_DES3_DECLS \
TEST_DECL_GROUP("des3", test_wc_Des3_SetIV), \
TEST_DECL_GROUP("des3", test_wc_Des3_SetKey), \
TEST_DECL_GROUP("des3", test_wc_Des3_CbcEncryptDecrypt), \
TEST_DECL_GROUP("des3", test_wc_Des3_CbcEncryptDecrypt_no_key), \
TEST_DECL_GROUP("des3", test_wc_Des3_EcbEncrypt), \
TEST_DECL_GROUP("des3", test_wc_Des3Cbc_MonteCarlo)

#endif /* WOLFCRYPT_TEST_DES3_H */
4 changes: 3 additions & 1 deletion tests/api/test_ossl_cipher.c
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,9 @@ int test_wolfSSL_RC4(void)
wolfSSL_RC4(NULL, 0, data, enc);

ExpectIntEQ(1, 1);
for (i = 0; EXPECT_SUCCESS() && (i <= sizeof(key)); i++) {
/* Start at a key length of 1: a zero-length key is invalid and leaves the
* Arc4 object without a key set, so encrypt/decrypt is a no-op. */
for (i = 1; EXPECT_SUCCESS() && (i <= sizeof(key)); i++) {
for (j = 0; EXPECT_SUCCESS() && (j <= sizeof(data)); j++) {
XMEMSET(enc, 0, sizeof(enc));
XMEMSET(dec, 0, sizeof(dec));
Expand Down
103 changes: 103 additions & 0 deletions tests/api/test_tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,109 @@ int test_tls12_no_null_compression(void)
return EXPECT_RESULT();
}

/* RFC 8422 Section 5.1.2: a client that sends an ec_point_formats extension
* omitting the uncompressed (0) format while negotiating an ECC suite must be
* rejected by the server with a fatal illegal_parameter alert. This drives a
* real handshake all the way through DoClientHello so the abort path (not just
* the parse-time detection) is exercised.
*
* Rather than hand-craft a ClientHello (which would pin the cipher suite, named
* group and exact byte offsets, making the test fragile as extension handling
* evolves), the client builds its own ClientHello and we only suppress the
* uncompressed point format: TLSX_PopulateExtensions() adds the default
* uncompressed format only when no ec_point_formats extension already exists,
* so pre-seeding the client with a compressed-only list makes it advertise
* exactly that. The curve is negotiated normally, so the test is independent of
* which named groups are enabled. */
int test_tls12_ec_point_formats_no_uncompressed(void)
{
EXPECT_DECLS;
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_NO_TLS12) \
&& defined(HAVE_ECC) && defined(HAVE_SUPPORTED_CURVES) \
&& defined(BUILD_TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA)
/* Pin an ECDHE (ECC) suite so the server negotiates an ECC key exchange;
* gating on the BUILD_ macro skips the test in builds where the suite is
* unavailable (e.g. --disable-aescbc) instead of failing with
* MATCH_SUITE_ERROR. */
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
struct test_memio_ctx test_ctx;

XMEMSET(&test_ctx, 0, sizeof(test_ctx));
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, "ECDHE-RSA-AES128-SHA"),
WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_s, "ECDHE-RSA-AES128-SHA"),
WOLFSSL_SUCCESS);
/* Make the client advertise only the compressed point format (1 ==
* ansiX962_compressed_prime), i.e. omit the uncompressed (0) format. */
ExpectIntEQ(TLSX_UsePointFormat(&ssl_c->extensions, 1, ssl_c->heap),
WOLFSSL_SUCCESS);
/* The server must reject the handshake with a fatal illegal_parameter
* alert (surfaced as INVALID_PARAMETER), not complete it. */
ExpectIntNE(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
ExpectIntEQ(wolfSSL_get_error(ssl_s, WOLFSSL_FATAL_ERROR),
WC_NO_ERR_TRACE(INVALID_PARAMETER));

wolfSSL_free(ssl_c);
wolfSSL_free(ssl_s);
wolfSSL_CTX_free(ctx_c);
wolfSSL_CTX_free(ctx_s);
#endif
return EXPECT_RESULT();
}

/* RFC 8422 Section 5.1.2 ties the missing-uncompressed-format abort to the
* server actually negotiating an ECC cipher suite. A client that omits the
* uncompressed point format but negotiates a NON-ECC suite (here DHE_RSA) must
* NOT be rejected - the handshake completes. This is the complement of
* test_tls12_ec_point_formats_no_uncompressed and guards against regressing
* back to an advertised-groups (parse-time) abort.
*
* As in that test the client builds a real ClientHello and we only suppress the
* uncompressed point format (see the comment there); the suite is pinned to a
* DHE (non-ECC) suite. */
int test_tls12_ec_point_formats_no_uncompressed_non_ecc(void)
{
EXPECT_DECLS;
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_NO_TLS12) \
&& defined(HAVE_SUPPORTED_CURVES) && !defined(NO_DH) && defined(HAVE_FFDHE) \
&& !defined(NO_RSA) && defined(BUILD_TLS_DHE_RSA_WITH_AES_128_CBC_SHA)
/* The negotiated suite must be non-ECC for the missing format to be
* irrelevant. RFC 9325 / WOLFSSL_HARDEN_TLS disables all TLS_DHE_* suites
* (NO_TLS_DH); gating on the BUILD_ macro skips the test there rather than
* failing with MATCH_SUITE_ERROR. */
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
struct test_memio_ctx test_ctx;

XMEMSET(&test_ctx, 0, sizeof(test_ctx));
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, "DHE-RSA-AES128-SHA"),
WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_s, "DHE-RSA-AES128-SHA"),
WOLFSSL_SUCCESS);
/* Make the client advertise only the compressed point format (1 ==
* ansiX962_compressed_prime), i.e. omit the uncompressed (0) format. */
ExpectIntEQ(TLSX_UsePointFormat(&ssl_c->extensions, 1, ssl_c->heap),
WOLFSSL_SUCCESS);
/* The handshake must complete: the missing uncompressed format is
* irrelevant for a non-ECC (DHE) suite. */
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
/* Sanity: the server really did observe a point-format list without the
* uncompressed format, yet proceeded. */
ExpectIntEQ(ssl_s->options.peerNoUncompPF, 1);

wolfSSL_free(ssl_c);
wolfSSL_free(ssl_s);
wolfSSL_CTX_free(ctx_c);
wolfSSL_CTX_free(ctx_s);
#endif
return EXPECT_RESULT();
}

/* Test that set_curves_list correctly resolves ECC curve names that fall
* through the kNistCurves table and reach the wc_ecc_get_curve_idx_from_name
* fallback path. The kNistCurves lookup uses a case-sensitive XSTRNCMP, so
Expand Down
5 changes: 5 additions & 0 deletions tests/api/test_tls.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ int test_tls_certreq_order(void);
int test_tls12_certreq_odd_sigalgs(void);
int test_tls12_bad_cv_sig_alg(void);
int test_tls12_no_null_compression(void);
int test_tls12_ec_point_formats_no_uncompressed(void);
int test_tls12_ec_point_formats_no_uncompressed_non_ecc(void);
int test_tls12_etm_failed_resumption(void);
int test_tls_set_session_min_downgrade(void);
int test_tls12_session_id_resumption_sni_mismatch(void);
Expand Down Expand Up @@ -60,6 +62,9 @@ int test_record_size_cache_invalidated_on_renegotiation(void);
TEST_DECL_GROUP("tls", test_tls12_certreq_odd_sigalgs), \
TEST_DECL_GROUP("tls", test_tls12_bad_cv_sig_alg), \
TEST_DECL_GROUP("tls", test_tls12_no_null_compression), \
TEST_DECL_GROUP("tls", test_tls12_ec_point_formats_no_uncompressed), \
TEST_DECL_GROUP("tls", \
test_tls12_ec_point_formats_no_uncompressed_non_ecc), \
TEST_DECL_GROUP("tls", test_tls12_etm_failed_resumption), \
TEST_DECL_GROUP("tls", test_tls_set_session_min_downgrade), \
TEST_DECL_GROUP("tls", test_tls12_session_id_resumption_sni_mismatch), \
Expand Down
Loading
Loading