fix(tls): validate cert/key pair and guard wildcard SAN matcher#13
Open
andypost wants to merge 1 commit into
Open
fix(tls): validate cert/key pair and guard wildcard SAN matcher#13andypost wants to merge 1 commit into
andypost wants to merge 1 commit into
Conversation
Two TLS hygiene fixes from the security audit (V3, PR-D slot in security-audit.md): * Missing SSL_CTX_check_private_key() — `src/nxt_openssl.c:522`. After SSL_CTX_use_PrivateKey() succeeds, also verify the loaded private key actually corresponds to the public key in the cert. Previously, a mismatched key/cert pair was accepted at config time and only surfaced as a TLS handshake failure at runtime, which presents to operators as a client-side problem. * 1-byte OOB read in wildcard SAN matcher — `src/nxt_openssl.c:986`. The check `item->name.start[0] == '*'` read byte 0 without first confirming `item->name.length > 0`. A cert with a zero-length SAN entry would dereference one byte past the buffer. Guard with `length > 0 &&` before the start[0] read. Two other V3 findings audited and reclassified as false positives (not addressed here): * "Cert chain mutated on active SSL_CTX during reload" (`nxt_openssl.c:504-510`) — on closer reading, the SSL_CTX in question is freshly created in nxt_openssl_server_init() and only reaches the listener / SNI dispatch table after nxt_openssl_chain_file() completes; SSL_CTX_add0_chain_cert() runs against a private context. No concurrent handshakes can hit it. * "realloc failure leaves chain count inconsistent" (`nxt_cert.c:278-283`) — cert->count is incremented only on the success path (line 288), and the just-parsed X509 is not yet in cert->chain[] when realloc fails; POSIX realloc leaves the original block intact on failure, so cert and chain[0..count-1] remain consistent for nxt_cert_destroy(). No protocol or config-surface changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request addresses two bugs in the OpenSSL integration. It adds a validation step using SSL_CTX_check_private_key to ensure that a loaded TLS private key matches its certificate during configuration, and it introduces a length check in the SAN matcher to prevent a 1-byte out-of-bounds read when processing empty names. I have no feedback to provide as there were no review comments.
This was referenced May 8, 2026
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
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
First audit-driven fix PR (PR-D slot from `security-audit.md` / PR #10). Two TLS hygiene fixes from V3; two other V3 findings reclassified as false positives after closer reading and documented in the commit message rather than fixed. Total diff: 11 lines across 2 files; no protocol or config-surface changes.
Findings addressed
Findings reclassified as false positives (not fixed)
Re-reading the source revealed two of the V3 mediums were misanalysed by the auditor:
Both are documented in the commit message body so a future re-auditor doesn't re-file them.
Files changed
Test plan
Audit linkage
Tracker rows in `security-audit.md` (PR #10) flip to `fixed (andypost/unit#NN)` once this merges.
Upstream
Both fix lines apply equally to `freeunitorg/freeunit`; will forward after merge here.
Generated by Claude Code