COR-170 - Verify TLS certificate hostnames on non-Apple platforms

[dbezverkhnii]

Note

High Risk
Changes TLS certificate acceptance for all mail sessions on OpenSSL/non-Apple platforms; misconfiguration could break legitimate servers or, if wrong, reintroduce hostname bypass.

Overview
Fixes COR-170: on non-Apple builds, OpenSSL verification previously checked chain trust only and could accept a valid certificate for a different host than the session hostname (CWE-297). The OpenSSL path now sets expected DNS or IP identity on the verify parameters before X509_verify_cert(), aligning with Apple’s SecPolicyCreateSSL behavior.

Verification logic is refactored into checkCertificateChain() (with optional test-only trust anchors and pinned verify time); production still uses the stream wrapper unchanged. A CCertificateUtils_checkCertificateChain C entry point and CertificateUtilsTests exercise the same path Swift clients rely on after TLS.

On non-Apple platforms, IMAP/SMTP/POP/NNTP TLS/StartTLS now use libetpan *_with_callback helpers to set the SSL server name for SNI. Build/header wiring and Android/Linux tweaks for IMAPInterruptCurrentCommandTests are included so the new tests run in CI.

^{Reviewed by Cursor Bugbot for commit c87ff7d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

🟡 Changes recommended

There is a confirmed memory leak on the OpenSSL path when sk_X509_push() fails, and at least one misleading test comment that should be corrected to match actual verification behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates MailCore2’s non-Apple TLS handling to verify certificate identity (hostname/IP) in addition to chain trust (COR-170), and ensures SNI server name is set for non-Apple TLS/StartTLS connections before post-handshake verification runs.

Changes:

• Add hostname/IP identity checks to non-Apple OpenSSL verification by setting the expected peer name on X509_VERIFY_PARAM, and refactor verification into checkCertificateChain().
• Add non-Apple libetpan *_with_callback hooks to set SSL server name (SNI) for IMAP/SMTP/POP/NNTP TLS and StartTLS paths.
• Add a Swift unit test suite with pinned time and DER fixtures, and wire up new public headers/build scripts.

File summaries

File	Description
unittest/CertificateUtilsTests.swift	Adds Swift tests exercising certificate chain + hostname/IP identity verification using DER fixtures and pinned verification time.
src/include/MailCore/MCCertificateUtils.h	Publishes the C++ checkCertificateChain() API in installed headers.
src/include/MailCore/CCertificateUtils.h	Publishes the C entry point used by Swift tests to invoke chain+identity verification.
src/core/security/MCCertificateUtils.h	Extends internal certificate utils header with checkCertificateChain() and test-only parameters (anchors/time).
src/core/security/MCCertificateUtils.cpp	Refactors verification to checkCertificateChain() and adds OpenSSL expected-identity verification (DNS/IP) plus better error reporting.
src/core/smtp/MCSMTPSession.cpp	Sets SNI server name via libetpan callback for non-Apple SMTP StartTLS (and uses callback path for TLS connect in file).
src/core/imap/MCIMAPSession.cpp	Sets SNI server name via libetpan callback for non-Apple IMAP StartTLS.
src/core/pop/MCPOPSession.cpp	Adds SNI callback usage for non-Apple POP TLS connect and StartTLS.
src/core/nntp/MCNNTPSession.cpp	Adds SNI callback usage for non-Apple NNTP TLS connect.
src/c/utils/CCertificateUtils.h	Adds C wrapper declaration for checkCertificateChain() (used from Swift).
src/c/utils/CCertificateUtils.cpp	Implements C wrapper converting Swift/C arrays into libetpan’s carray chain representation.
src/cmake/public-headers.cmake	Exposes core/security/MCCertificateUtils.h as a public installed header.
src/CMakeLists.txt	Adds libetpan to CMailCore link on Windows to satisfy new usages.
Package.swift	Wires the new Swift test file into the SPM test target.
configure-headers.sh	Ensures generated installed headers include the new certificate utility headers.

Review details

Suppressed comments (1)

src/core/security/MCCertificateUtils.cpp:269

• On OpenSSL builds, if sk_X509_push(certificates, certificate) fails, the newly-created certificate is leaked because it was never pushed onto the stack (so sk_X509_pop_free() won’t free it). Free it before jumping to free_certs.

        if (!sk_X509_push(certificates, certificate)) {
            MCLog("MCCertificateUtils error: can't sk_X509_push");
            goto free_certs;
        }

• Files reviewed: 15/18 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

It changes TLS acceptance semantics across all non-Apple platforms and needs careful human validation of interoperability and failure modes despite the added unit coverage.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/core/security/MCCertificateUtils.cpp:86

• checkCertificateChain() can be called with an empty (but non-NULL) certificate chain (e.g. the Swift C wrapper builds an empty carray when the input array is empty). In the OpenSSL path this leads to sk_X509_value(certificates, 0) == NULL being passed into X509_STORE_CTX_init(), which relies on OpenSSL’s handling of a NULL leaf and makes failure mode/diagnostics platform-dependent. Add an explicit early return for NULL/empty chains so the function rejects them deterministically.

• Files reviewed: 16/19 changed files
• Comments generated: 0 new
• Review effort level: Lite