TLS Fenrir fixes - #11094
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11094
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.
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11094
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.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11094
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
DoClientHello has four exits that fail with VERSION_ERROR when runtime version restrictions leave nothing acceptable at or below the version the client offered. Three of them sent no alert at all, and the fourth sent one only when WOLFSSL_EXTRA_ALERTS was defined, so a default build simply dropped the connection. The client could not tell a version mismatch from a network failure. The generic fallback did not help. SendFatalAlertOnly is a no-op unless WOLFSSL_EXTRA_ALERTS is defined, and where it is defined it grouped VERSION_ERROR with MATCH_SUITE_ERROR and sent handshake_failure. That also disagreed with the TLS 1.3 mapping, which already resolves VERSION_ERROR to protocol_version. Send a fatal protocol_version alert from all four branches regardless of WOLFSSL_EXTRA_ALERTS, and give VERSION_ERROR its own case in SendFatalAlertOnly so the generic path agrees. Note that this is only observable on the TLS 1.2 message path. A TLS 1.3 capable server routes the ClientHello through DoTls13HandShakeMsgType, which already translates the error into the right alert. Fixes F-7568.
The function freed the extension's dynamically allocated ASN.1 string buffer but left value.data and value.isDynamic pointing at it. The subsequent wolfSSL_ASN1_STRING_copy() call snapshots those fields before copying and frees the old buffer once the copy is complete, so the stale pointer was freed a second time. Any second call to wolfSSL_X509_EXTENSION_set_data() on an extension holding a value of at least CTC_NAME_SIZE bytes hit this, and passing the extension its own value made the copy read freed memory as well. wolfSSL_ASN1_STRING_set() already performs an alias safe replacement and disposes of the previous buffer itself, so drop the manual free. Add a regression test that replaces a dynamically allocated extension value and then sets the value from itself. Fixes F-7340.
wolfSSL_X509_STORE_CTX_set_verify_cb stored the application callback in ctx->verify_cb, but every verification site read ctx->store->verify_cb instead, so the field was never consulted. An application installing a restrictive callback on the store context, which is the OpenSSL documented way to enforce extra policy during verification, had it silently ignored, and wolfSSL_X509_verify_cert could report success on a chain the callback would have rejected. Add X509StoreGetVerifyCb, which prefers the context callback and falls back to the store one, and use it at all four call sites in X509StoreVerifyCert, X509StoreCheckPathLen and wolfSSL_X509_verify_cert. The store fallback keeps its OPENSSL_ALL or WOLFSSL_QT guard because the store field only exists there, while the call sites now follow the OPENSSL_EXTRA guard of the setter. Clear ctx->verify_cb in wolfSSL_X509_STORE_CTX_init along with the other per-verification state so a reused context does not carry a stale callback. A rejection also has to be reportable. When the certificate manager accepts a chain, ctx->error is X509_V_OK, so a callback that rejects it without recording an error of its own left wolfSSL_X509_verify_cert returning failure while X509_STORE_CTX_get_error still said the chain was fine. Record WOLFSSL_X509_V_ERR_UNSPECIFIED in that case, matching what OpenSSL reports, and only when the callback set no error itself. Add that value to the X509 error enum, where the openssl compatibility header already had the define. Feeding the rejection marker to SetupStoreCtxError is not an option there, since GetX509Error has no X509_V_ error for it and would pass the negative value through as the reported error. The OPENSSL_ALL date recheck did exactly that after a rejection, so skip that block once the callback has rejected, which also stops it from consulting the callback a second time. Add a regression test that verifies a good chain twice, once bare and once with a rejecting context callback, requires the second attempt to fail, and checks the reported error both when the callback records one and when it does not. Fixes F-7341.
| WOLFSSL_MSG("Client trying to connect with lesser version"); | ||
| ret = VERSION_ERROR; | ||
| /* propagate socket errors to avoid re-calling send alert */ | ||
| if (SendAlert(ssl, alert_fatal, |
There was a problem hiding this comment.
This can produce a duplicate fatal alert.
Recommendation: Add the ssl->alert_history.last_tx.level != alert_fatal guard — preferably at src/tls13.c:14896, since that fixes the whole class of problem for every error DoClientHello alerts on directly (it already double-sends for decode_error, illegal_parameter and inappropriate_fallback via the same route); alternatively guard the four new SendAlert() calls in DoClientHello.
| */ | ||
| enum { | ||
| WOLFSSL_X509_V_OK = 0, | ||
| WOLFSSL_X509_V_ERR_UNSPECIFIED = 1, |
There was a problem hiding this comment.
Consider fixing GetX509Error() -> 'case 1: return 0' success mapping
| * the application the chain was fine. Fill in the generic error OpenSSL | ||
| * uses for this, without touching an error the callback did set. */ | ||
| if (ret != WOLFSSL_SUCCESS && ctx->error == WOLFSSL_X509_V_OK) { | ||
| ctx->error = WOLFSSL_X509_V_ERR_UNSPECIFIED; |
There was a problem hiding this comment.
Blanket X509_V_ERR_UNSPECIFIED fallback applies to every failure path, including allocation failures.
Recommendation: Decide which behavior is intended. If the blanket fallback is deliberate, widen the comment so it does not read as callback-specific. If not, record a specific error (OpenSSL's X509_V_ERR_OUT_OF_MEM) at the allocation-failure goto exit sites and keep the fallback scoped to the veto case.
| WOLFSSL_SUCCESS : ret; | ||
| #endif | ||
| if (verifyCb != NULL) { | ||
| if (verifyCb(ret >= 0 ? 1 : 0, ctx) == 1) { |
There was a problem hiding this comment.
Recommendation: Snapshot ctx->error before calling the callback and compare afterwards, so only an error the callback genuinely recorded is preserved.
| * (WOLFSSL_FAILURE / WOLFSSL_FATAL_ERROR) because chain building must stop on | ||
| * it: a veto must not be turned into a retry with another issuer, and must not | ||
| * be cleared by the partial-chain fallback in wolfSSL_X509_verify_cert(). */ | ||
| #define X509_STORE_CB_REJECTED (-2) |
There was a problem hiding this comment.
Recommendation: Pick a value that cannot collide with wolfSSL_ErrorCodes/WOLFSSL_CBIO_ERR_*, or return the veto through an out-parameter instead of in band. Either way, move the #define up with the file's other constants.
| XFREE(current->data, NULL, DYNAMIC_TYPE_OPENSSL); | ||
| } | ||
|
|
||
| /* wolfSSL_ASN1_STRING_copy() frees any existing dynamic buffer itself, |
There was a problem hiding this comment.
Recommendation: Reword to name wolfSSL_ASN1_STRING_set() as the owner of the free.
| WOLFSSL_MSG("\tversion below minimum allowed, fatal error"); | ||
| ret = VERSION_ERROR; | ||
| /* propagate socket errors to avoid re-calling send alert */ | ||
| if (SendAlert(ssl, alert_fatal, |
There was a problem hiding this comment.
Recommendation: Consider restoring ssl->version to the client's offered version (or the server's own maximum) around the SendAlert() call at exits 3 and 4, so the alert record carries a version the peer will accept. At minimum, note the intent in a comment at the source rather than only in the test.
Three independent Fenrir fixes, each with a regression test that fails without its source change.
1. Honor the per-context X509_STORE_CTX verify callback (F-7341)
wolfSSL_X509_STORE_CTX_set_verify_cb()stored the callback inctx->verify_cb, but every verification site readctx->store->verify_cb, so the field was never consulted - a restrictive callback installed on the store context was silently ignored, andwolfSSL_X509_verify_cert()could succeed on a chain the callback would have rejected.New
X509StoreGetVerifyCb()prefers the context callback and falls back to the store's, matching OpenSSL precedence; it is used at all four call sites.wolfSSL_X509_STORE_CTX_init()now clearsctx->verify_cbso a reused context carries no stale callback.Two notes: the pathLen and
INVALID_CAoverrides become reachable inOPENSSL_EXTRAbuilds, not justOPENSSL_ALL/WOLFSSL_QT, since the setter is guarded that way - this is the documented OpenSSL semantic. Rejection (callback returns 0) is honored only for the per-context callback; widening the store callback would be a separate behavior change.2. Fix double free in
wolfSSL_X509_EXTENSION_set_data()(F-7340)The function freed the extension's dynamic ASN.1 string buffer but left
value.dataandvalue.isDynamicpointing at it. The followingwolfSSL_ASN1_STRING_copy()snapshots those fields and frees the old buffer itself once the copy completes, so the stale pointer was freed twice. Any second call on a value of at leastCTC_NAME_SIZEbytes hit this; passing the extension its own value also read freed memory.wolfSSL_ASN1_STRING_copy()is alias-safe, so the manual free is dropped.3. Send a protocol_version alert when no version can be negotiated (F-7568)
DoClientHello()has four exits that fail withVERSION_ERRORwhen runtime version restrictions leave nothing acceptable at or below the offered version. Three sent no alert, the fourth only underWOLFSSL_EXTRA_ALERTS, so a default build just dropped the connection - the client could not tell a version mismatch from a network failure. RFC 5246 7.2.2 and RFC 8446 4.1.3 require a fatalprotocol_versionalert.The generic fallback did not help:
SendFatalAlertOnly()is a no-op withoutWOLFSSL_EXTRA_ALERTS, and where defined it groupedVERSION_ERRORwithMATCH_SUITE_ERRORand senthandshake_failure, disagreeing with the TLS 1.3 mapping. All four branches now alert regardless ofWOLFSSL_EXTRA_ALERTS(propagatingSOCKET_ERROR_E), andVERSION_ERRORgets its own case inSendFatalAlertOnly(). Only observable on the TLS 1.2 path; TLS 1.3 already translates the error correctly.Tests
test_wolfSSL_X509_STORE_CTX_verify_cbossl_x509_storeX509_STORE_CTX_init()test_wolfSSL_X509_EXTENSION_set_dataossl_x509_exttest_tls_no_acceptable_version_alerttlsprotocol_versionalert on the wire (noWOLFSSL_EXTRA_ALERTS)test_tls_version_error_alert_mappingtlsSendFatalAlertOnly()mapsVERSION_ERRORtoprotocol_version(withWOLFSSL_EXTRA_ALERTS)The two TLS tests are complementary: with
WOLFSSL_EXTRA_ALERTSthe fallback emits a byte-identical alert, so the first would pass either way.