fix: real peer verification in SecureClient (hostname check + fail-closed CA load) - #65
Open
sfoulad wants to merge 1 commit into
Open
fix: real peer verification in SecureClient (hostname check + fail-closed CA load)#65sfoulad wants to merge 1 commit into
sfoulad wants to merge 1 commit into
Conversation
SecureClient: - wolfSSL_check_domain_name() is armed for every non-insecure connection. SNI names the host but never bound the peer certificate to it, so a setCACert() connection still accepted any certificate signed by a trusted CA for any hostname. - wolfSSL_CTX_load_verify_buffer() failure now fails the connect outright instead of proceeding to a handshake against an empty trust store whose verify error is indistinguishable from a bad peer. SecureHttpClient: - 1xx/204/304 responses carry no body (RFC 9110 s6.4.1); read one and a kept-alive 304 with no framing headers stalls in readUntilClose() until the timeout. Required for conditional GETs (If-None-Match) against the GitHub API.
Contributor
Author
|
Status update: this makes SecureClient actually verify the peer — hostname check plus fail-closed CA loading — instead of accepting any CA-signed certificate for any host; it's the SDK half of crosspoint-reader#3298. This repo runs no CI, so the evidence is downstream: it builds clean into an ESP32-C3 firmware and the chain/hostname negative tests pass at the HTTP layer against captured live GitHub chains. Downstream we're pinned to a fork commit for this one change; a review and merge lets our submodule return to an official upstream ref. Happy to split the hostname check and CA-load changes if that's easier to review. |
sfoulad
added a commit
to sfoulad/midad-by-foulad
that referenced
this pull request
Aug 30, 2026
The RC hardened CrossPoint PR #3298's OTA transport locally instead of upstream. That hardening now exists upstream on fix/3298-verified-entrypoint (850e0657, on top of #3298's head 108ea716), so the RC takes the reviewed form rather than carrying a Midad variation of a generic transport rule. - src/network/HttpVerifiedFetch.h: added byte-identical to 850e0657. urlIsHttps(), verifiedFetchRefusal() and kVerifiedFetchNeedsAnchors now have exactly one definition in the tree, upstream's. - HttpDownloader::fetchUrlVerified(DataCallback) is byte-identical to 850e0657's: the bespoke verifiedFetchAllowed() helper is gone and the precondition runs outside any #if, so the https-only half applies on non-wolfSSL builds too. - The Midad-only ConditionalGet& overload survives and routes through the same verifiedFetchRefusal(), not a second copy of the rules. - lib/UrlOrigin/UrlOrigin.h keeps urlOrigin() (the cross-origin Authorization latch) and drops its duplicate urlIsHttps(). - test/http_verified_fetch/ added byte-identical to 850e0657 (11 cases per TLS backend) and registered. test/http_url_origin keeps the extra malformed-scheme shapes upstream's suite does not enumerate. Deliberately NOT adopted from 850e0657: its corrected setCACert comment saying hostname checking is uncovered until Free-Ink/freeink-sdk#65 lands. That is true of upstream's SDK pin (f831c1e4) but false here -- the RC pins freeink-sdk 6d33e9fd, whose SecureClient calls wolfSSL_check_domain_name() on every non-insecure handshake. The gitlink is untouched. Host tests: 358/358 pass (336 baseline + 22 new).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SecureClient::connect()loads the caller's CA bundle viasetCACert()but never callswolfSSL_check_domain_name(), so hostname verification is silently skipped: any certificate signed by any anchor in the bundle is accepted for any host. A network attacker holding a valid certificate for their own domain (issued by the same public CA) passes verification when impersonating the target. This defeats the point ofsetCACertfor public-internet endpoints.Two further hardening items in the same small surface:
wolfSSL_CTX_load_verify_buffer()failure was not treated as fatal — a corrupt/truncated bundle degraded to an unverified connection. It now fails the connect (fail closed).SecureHttpClienttreated every response as having a body; per RFC 9110, 1xx/204/304 have none. A kept-alive304 Not Modified(conditional GET) previously stalled the read loop until timeout.Changes
SecureClient.cpp: armwolfSSL_check_domain_name(ssl, host)for every non-insecure connection; fail closed on CA-load errors. (+24/−2)SecureHttpClient.h: no-body status handling for 1xx/204/304. (+11)setInsecure()behaviour is unchanged — callers that explicitly opt out still can; this only makes the verified path actually verify.Verification
Happy to split the RFC 9110 hunk into its own PR if you prefer reviewing the security change in isolation.