Send decrypt_error alert for TLS 1.3 PSK binder failures - #11105
Conversation
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11105
Scan targets checked: wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
a3a1d6a to
01d1262
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11105
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
RFC 8446 Section 6.2 lists failure to validate a PSK binder under decrypt_error, but TranslateErrorToAlert() mapped BAD_BINDER to illegal_parameter. That told the peer its ClientHello was malformed, when in fact the message was well formed and only the binder MAC did not match. BAD_BINDER is also returned when no offered PSK matched and the server has no certificate to fall back to. That reuse is deliberate, because it keeps an unknown PSK identity indistinguishable from a failed binder, so both conditions still map to a single alert after this change. A comment at the return site in CheckPreSharedKeys() now records the invariant at both ends. Splitting BAD_BINDER off illegal_parameter exposed a second path that had to move with it. FindPsk() raised PSK_KEY_ERROR, which stays mapped to illegal_parameter, when the server callback recognised an identity carrying a non-zero obfuscated_ticket_age. That check ran before any binder was derived, so on a certificate-less server the two alerts would have let an unauthenticated peer enumerate valid PSK identities without holding a key. The check is removed rather than made to fail differently: RFC 8446 Section 4.2.11 says that for an externally established identity an obfuscated_ticket_age of 0 SHOULD be used and servers MUST ignore the value. Ignoring it satisfies that requirement, closes the oracle, and lets a conformant client that sends a non-zero age complete a handshake that was previously rejected. test_tls13_psk_age_no_identity_oracle() asserts a known and an unknown identity produce the same alert, with a positive control that the known run really took the matched path. Also removes an unreachable branch in CheckPreSharedKeys(). Since commit 089f1f7 added the earlier !*usingPSK certificate check, the later certificate check and its BAD_BINDER return can no longer be reached. A build with certificates returns BAD_BINDER earlier when none is loaded, and a NO_CERTS build returns it unconditionally. The remaining branch now falls through to the shared exit so WOLFSSL_LEAVE() is emitted on the certificate fallback path as well. Adds an os-check-linux configuration, tls13-psk-no-certs. psk.yml already covers NO_CERTS with TLS 1.3 PSK through static-psk-lowresource-tls13, but that config disables DH, so it only reaches the psk_ke branch. This one leaves DH enabled and covers psk_dhe_ke under NO_CERTS. Applications that inspect wolfSSL_get_alert_history() will observe alert 51 instead of 47 for these conditions. The affected tests are updated.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11105
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Description
TranslateErrorToAlert()mappedBAD_BINDERtoillegal_parameter(47), which tells the peer it sent an invalid ClientHello. RFC 8446 Section 6.2 lists PSK binder validation failure underdecrypt_error(51), and that is what the condition actually is: the message was well formed and only the binder MAC failed. This PR changes the mapping.CheckPreSharedKeys()also returnsBAD_BINDERwhen no offered PSK matched and there is no certificate to fall back to. That reuse is deliberate (commit089f1f7c9): it keeps an unknown PSK identity indistinguishable from a failed binder. Both conditions still share one alert, so the change is indistinguishability preserving, and a comment at the return site now records the invariant at both ends.Finally, the later certificate check in
CheckPreSharedKeys()has been unreachable since089f1f7c9added the earlier one, so it is removed. The remaining branch falls through to the shared exit, which also restores a missingWOLFSSL_LEAVE()on the certificate fallback path.Behaviour change
Applications inspecting
wolfSSL_get_alert_history()will see alert 51 instead of 47 when a binder fails to verify, and when no PSK matched on a server with no certificate. The returned error code (BAD_BINDER) and the fatal level are unchanged.Identity protection
Splitting
BAD_BINDERoffillegal_parameterexposed a second path that had to move with it.FindPsk()raisedPSK_KEY_ERROR, which stays mapped toillegal_parameter, when the callback recognised an identity carrying a non-zeroobfuscated_ticket_age. That ran before any binder was derived, so on a certificate-less server the two alerts would have let an unauthenticated peer enumerate valid PSK identities without holding a key.The check is removed rather than made to fail differently. RFC 8446 Section 4.2.11: "For identities established externally, an obfuscated_ticket_age of 0 SHOULD be used, and servers MUST ignore the value." Ignoring it satisfies that requirement, closes the oracle, and lets a conformant client that sends a non-zero age complete a handshake that wolfSSL previously rejected.
test_tls13_psk_age_no_identity_oracle()guards this. It rewrites the age on the wire, since wolfSSL's own client always sends zero, and asserts a known and an unknown identity produce the same alert. A control run with an untouched ClientHello completes the handshake first, and each run assertsssl_s->options.isPSK, which is set only after the ciphersuite match, so neither rejecting run can pass by way of a fixture that never negotiated a PSK. The accept side for a non-zero age is not covered: any on-wire rewrite invalidates the binder the client already computed, so driving it would need a hand-built ClientHello with an offline-precomputed binder.New CI coverage
.github/workflows/psk.ymlalready coversNO_CERTSwith TLS 1.3 PSK viastatic-psk-lowresource-tls13, but that config disables DH, so it only reaches thepsk_kebranch. The newtls13-psk-no-certsentry in.github/configs/os-check-linux.jsonleaves DH enabled and coverspsk_dhe_keunderNO_CERTS. There is no--disable-certs;ENABLED_CERTSis derived, so both reach it via--disable-asn.Testing
./tests/unit.teston--enable-psk --enable-tls13:0/1664/483/2147.illegal_parameterexpectation is restored, so they are not vacuous.wolfSSL_require_psk()still sendshandshake_failure(40)and accepting controls still complete.FindPsk()change and both give 51 after.-333 != -423) and the alert comparison. The control run still passes there, which is what localises the failure to the age handling.illegal_parameter(47)before the change and completes the handshake after it, confirming the RFC 8446 Section 4.2.11 behaviour.WOLFSSL_LEAVE()on the certificate fallback path: 0 before, 1 after.tls13-psk-no-certspasses throughparallel-make-check.pywith the workflow's own flags. Itsmake checkis a genuineNO_CERTSrun (0/1876/235/2111), and bothtest_tls13_psk_no_cert_bad_binderand the new test execute rather than skipping.Related
Found while analysing #11077, which asked for
access_denied(49)when a server PSK callback declines a valid identity. That is not actionable and #11077 should be closed as working as intended: the callback runs before the binder is verified, so nothing is authenticated when the application decides, andwc_psk_server_callbackcannot distinguish an unknown identity from a denied one. A distinct alert would also reopen the enumeration oracle089f1f7c9closed. The mapping fixed here is the real defect that analysis surfaced.