Add RFC8773(bis) cert_with_extern_psk support#10085
Add RFC8773(bis) cert_with_extern_psk support#10085Frauschi wants to merge 1 commit intowolfSSL:masterfrom
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10085
Scan targets checked: src, src-bugs, src-compliance
Findings: 3
Medium (2)
DoPreSharedKeys returns fatal error instead of skipping session ticket identity when cert_with_extern_psk is offered
File: src/tls13.c:6193-6198
Function: DoPreSharedKeys
Category: Logic errors
When the server processes PSK identities in DoPreSharedKeys, it iterates through all client-offered identities trying each as a session ticket first, then as an external PSK via FindPsk. The new code returns PSK_KEY_ERROR immediately when a session ticket identity matches and certWithExternOffered is true. This terminates the entire function, preventing the server from trying subsequent identities in the list. A legitimate client could offer a session ticket identity (for resumption with servers that don't support cert_with_extern_psk) followed by an external PSK identity (for cert_with_extern_psk). The fatal return prevents the server from ever reaching the external PSK identity, causing a handshake failure that could be avoided by simply skipping the ticket identity.
#if defined(WOLFSSL_CERT_WITH_EXTERN_PSK) && defined(HAVE_SESSION_TICKET)
if (certWithExternOffered) {
WOLFSSL_ERROR_VERBOSE(PSK_KEY_ERROR);
return PSK_KEY_ERROR;
}
#endifRecommendation: Replace return PSK_KEY_ERROR with continue so that the server skips the session ticket identity and proceeds to try the next identity in the list, which may be a valid external PSK identity suitable for cert_with_extern_psk.
DoPreSharedKeys fatally rejects session ticket match when cert_with_extern_psk is offered, violating RFC 8773bis fallback behavior
File: src/tls13.c:6196-6200
Function: DoPreSharedKeys
Category: Session resumption violations
When the client offers the tls_cert_with_extern_psk extension and also includes a session ticket identity in pre_shared_key, the server returns PSK_KEY_ERROR if the session ticket successfully decrypts. This is a hard return, not a continue, so it aborts the entire PSK identity search loop — even if a valid external PSK identity appears later in the list.
RFC 8773bis Section 3 explicitly permits the server to select a resumption PSK when the client offers cert_with_extern_psk: "If the client includes the 'tls_cert_with_extern_psk' extension in the ClientHello, and the server selects a resumption PSK, the server MUST NOT echo the extension." The spec allows normal ticket resumption as a fallback — the server simply omits the extension from ServerHello.
The current code prevents two valid scenarios:
- A client offering both a session ticket and an external PSK identity (ticket listed first) — the server should skip the ticket and try the external PSK, or fall back to normal resumption.
- A client offering
cert_with_extern_pskwith only a session ticket that matches — the server should fall back to normal ticket resumption without echoing the extension.
#if defined(WOLFSSL_CERT_WITH_EXTERN_PSK) && defined(HAVE_SESSION_TICKET)
if (certWithExternOffered) {
WOLFSSL_ERROR_VERBOSE(PSK_KEY_ERROR);
return PSK_KEY_ERROR;
}
#endif
/* SERVER: using secret in session ticket for peer auth. */
ssl->options.peerAuthGood = 1;Recommendation: Replace return PSK_KEY_ERROR with continue so the server skips the session ticket identity and continues searching for an external PSK identity. If no external PSK matches after the loop completes, the server can fall back to normal behavior (either retry with the ticket without echoing cert_with_extern_psk, or proceed without PSK).
Low (1)
TLSX_Cert_With_Extern_Psk_Parse overwrites actual error code with MEMORY_E
File: src/tls.c:12389-12390
Function: TLSX_Cert_With_Extern_Psk_Parse
Category: Incorrect error handling
In the client_hello handler of TLSX_Cert_With_Extern_Psk_Parse, the return value of TLSX_Cert_With_Extern_Psk_Use(ssl) is checked for non-zero but then hardcoded MEMORY_E is returned instead of the actual error code. TLSX_Cert_With_Extern_Psk_Use internally calls TLSX_Push, which may return various error codes. The actual error is lost and replaced with MEMORY_E, making debugging harder and potentially reporting the wrong error to the caller.
if (msgType == client_hello) {
if (!ssl->options.certWithExternPsk)
return 0;
if (TLSX_Cert_With_Extern_Psk_Use(ssl) != 0)
return MEMORY_E;Recommendation: Propagate the actual error code: int ret = TLSX_Cert_With_Extern_Psk_Use(ssl); if (ret != 0) return ret;
This review was generated automatically by Fenrir. Findings are non-blocking.
2ee0302 to
7a7ba81
Compare
|
@wolfSSL-Fenrir-bot Findings 1 and 2 identify a real bug, RFC 8773bis §5.1 (draft-ietf-tls-8773bis-13) is unambiguous: ▎ "All of the listed PSKs MUST be external PSKs. If a resumption PSK is There is no "skip and continue" option — the presence of any resumption The actual bug was that the check fired too late: only after The fix moves the certWithExternOffered abort to fire immediately upon |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10085
Scan targets checked: src, src-bugs, src-compliance
No new issues found in the changed files. ✅
|
Jenkins retest this please |
7a7ba81 to
9976db0
Compare
Implement RFC8773bis (draft-ietf-tls-8773bis-13) cert_with_extern_psk for TLS 1.3, including protocol checks and API support. Includes unit tests for API and handshake behavior as well as tests in the testsuite using extended examples.
9976db0 to
615ac8d
Compare
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 7 total — 5 posted, 2 skipped
Posted findings
- [High] Early return in DoPreSharedKeys skips ForceZero cleanup of sensitive key material —
src/tls13.c:6247 - [Medium] FindPsk sets peerAuthGood=1 on server without considering certWithExternPsk —
src/tls13.c:4316 - [Medium] No negative test for client receiving resumption ticket with cert_with_extern_psk —
tests/api/test_tls13.c:951-1059 - [Medium] test_tls13_cert_with_extern_psk_handshake may silently skip cert verification —
tests/api/test_tls13.c:970-975 - [Medium] Early data extension validation runs only when WOLFSSL_EARLY_DATA is defined —
src/tls.c:17903-17907
Skipped findings
- [Low] Preprocessor indent inconsistency in server.c cert_with_extern_psk block
- [Info] WOLFSSL_ENTER missing from new API functions
Review generated by Skoll via openclaw
| sizeof(psk_sess_free_cb_ctx)); | ||
| } | ||
| WOLFSSL_ERROR_VERBOSE(PSK_KEY_ERROR); | ||
| return PSK_KEY_ERROR; |
There was a problem hiding this comment.
🟠 [High] Early return in DoPreSharedKeys skips ForceZero cleanup of sensitive key material
🚫 BLOCK bug
When certWithExternOffered is true and a session ticket is successfully decrypted (resumption PSK detected), the new code at line 6247 does return PSK_KEY_ERROR; instead of goto cleanup;. The function's cleanup label at line 6368-6370 performs ForceZero(binderKey, ...) and ForceZero(binder, ...) to scrub sensitive cryptographic material from the stack. The early return bypasses this cleanup, leaving binder key material on the stack. Every other error path in this function uses goto cleanup.
Suggestion:
| return PSK_KEY_ERROR; | |
| WOLFSSL_ERROR_VERBOSE(PSK_KEY_ERROR); | |
| ret = PSK_KEY_ERROR; | |
| goto cleanup; |
— Skoll automated review via openclaw
| #ifdef WOLFSSL_CERT_WITH_EXTERN_PSK | ||
| if (ssl->options.certWithExternPsk) { | ||
| /* Certificate authentication is still required. */ | ||
| ssl->options.peerAuthGood = 0; |
There was a problem hiding this comment.
🟡 [Medium] FindPsk sets peerAuthGood=1 on server without considering certWithExternPsk
💡 SUGGEST bug
The FindPsk function (server-side PSK matching) unconditionally sets ssl->options.peerAuthGood = 1 at line 6134 when an external PSK identity is matched. In cert_with_extern_psk mode, the client (the server's peer) must also authenticate via certificate. While CheckPreSharedKeys subsequently sets ssl->options.sendVerify = SEND_CERT and verifyPeer is preserved, peerAuthGood is already set to 1 before the client certificate is verified. The client-side SetupPskKey at line 4314 correctly gates peerAuthGood on certWithExternPsk, but the server-side FindPsk does not. In practice this may work because the server later validates the client cert via DoTls13CertificateVerify, and peerAuthGood is set again there. However, there's a window where peerAuthGood=1 before the client has authenticated if verifyPeer is configured to not fail on missing cert.
Suggestion:
| ssl->options.peerAuthGood = 0; | |
| if (ret == 0) { | |
| /* PSK negotiation has succeeded */ | |
| ssl->options.isPSK = 1; | |
| #ifdef WOLFSSL_CERT_WITH_EXTERN_PSK | |
| if (!ssl->options.certWithExternPsk) | |
| #endif | |
| { | |
| /* SERVER: using PSK for peer authentication. */ | |
| ssl->options.peerAuthGood = 1; | |
| } | |
| } |
— Skoll automated review via openclaw
| } | ||
| #endif | ||
|
|
||
| int test_tls13_cert_with_extern_psk_handshake(void) |
There was a problem hiding this comment.
🟡 [Medium] No negative test for client receiving resumption ticket with cert_with_extern_psk
💡 SUGGEST test
The tests cover the happy path (successful handshake) and the HRR path, but there are no negative tests for important error scenarios introduced by this PR: (1) Server receives a resumption ticket PSK alongside cert_with_extern_psk extension (should fail with PSK_KEY_ERROR per line 6238-6248 in tls13.c), (2) Client receives ServerHello with cert_with_extern_psk but no key_share (should fail with EXT_MISSING per line 5753-5758 in tls13.c), (3) Client receives ServerHello confirming cert_with_extern_psk for a resumption PSK (should fail with PSK_KEY_ERROR per line 5748-5752 in tls13.c).
Recommendation: Add unit tests for the error paths: (1) test that resumption ticket identity with cert_with_extern_psk is rejected on the server, (2) test that missing key_share in ServerHello with cert_with_extern_psk is rejected on the client. These are critical validation paths.
— Skoll automated review via openclaw
|
|
||
| wolfSSL_set_verify(ssl_c, WOLFSSL_VERIFY_NONE, NULL); | ||
| wolfSSL_set_verify(ssl_s, WOLFSSL_VERIFY_NONE, NULL); | ||
| #if defined(HAVE_ECC) && !defined(NO_CERTS) && !defined(NO_FILESYSTEM) |
There was a problem hiding this comment.
🟡 [Medium] test_tls13_cert_with_extern_psk_handshake may silently skip cert verification
💡 SUGGEST test
The handshake test loads ECC certs only under #if defined(HAVE_ECC) && !defined(NO_CERTS) && !defined(NO_FILESYSTEM). If ECC is not available (e.g. RSA-only build), no server certificate is loaded, and the test may either fail the handshake (making it not a valid positive test) or succeed without actually sending a certificate. The test sets WOLFSSL_VERIFY_NONE on both sides, so no cert verification occurs even in the happy path. Consider also loading RSA certs as a fallback to ensure broader test coverage.
Suggestion:
| #if defined(HAVE_ECC) && !defined(NO_CERTS) && !defined(NO_FILESYSTEM) | |
| #if !defined(NO_CERTS) && !defined(NO_FILESYSTEM) | |
| #if defined(HAVE_ECC) | |
| ExpectTrue(wolfSSL_use_certificate_file(ssl_s, eccCertFile, | |
| CERT_FILETYPE) == WOLFSSL_SUCCESS); | |
| ExpectTrue(wolfSSL_use_PrivateKey_file(ssl_s, eccKeyFile, | |
| CERT_FILETYPE) == WOLFSSL_SUCCESS); | |
| #elif !defined(NO_RSA) | |
| ExpectTrue(wolfSSL_use_certificate_file(ssl_s, svrCertFile, | |
| CERT_FILETYPE) == WOLFSSL_SUCCESS); | |
| ExpectTrue(wolfSSL_use_PrivateKey_file(ssl_s, svrKeyFile, | |
| CERT_FILETYPE) == WOLFSSL_SUCCESS); | |
| #endif | |
| #endif |
— Skoll automated review via openclaw
| WOLFSSL_ERROR_VERBOSE(EXT_MISSING); | ||
| return EXT_MISSING; | ||
| } | ||
| #ifdef WOLFSSL_EARLY_DATA |
There was a problem hiding this comment.
🟡 [Medium] Early data extension validation runs only when WOLFSSL_EARLY_DATA is defined
💡 SUGGEST question
The RFC 8773bis validation that rejects early_data alongside cert_with_extern_psk is wrapped in #ifdef WOLFSSL_EARLY_DATA. If a build has WOLFSSL_CERT_WITH_EXTERN_PSK but not WOLFSSL_EARLY_DATA, a peer that sends an early_data extension would not be rejected by this check. However, this may be safe because without WOLFSSL_EARLY_DATA the extension wouldn't be processed at all (it would be treated as unknown). Flagging for consideration.
Recommendation: Verify that builds without WOLFSSL_EARLY_DATA still reject or ignore early_data extensions from peers. If unknown extensions are simply ignored (as per TLS 1.3 spec), this is safe.
— Skoll automated review via openclaw
Implement RFC 8773(bis) cert_with_extern_psk for TLS 1.3.
Includes unit tests for API and handshake behavior as well as tests in the testsuite using extended examples.