Normalize trailing dot when matching hostnames against certificate pins - #9734
Open
adityaanikam wants to merge 1 commit into
Open
Normalize trailing dot when matching hostnames against certificate pins#9734adityaanikam wants to merge 1 commit into
adityaanikam wants to merge 1 commit into
Conversation
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.
Fixes #9724.
Credit to Joshua Rogers (MegaManSec) for finding and reporting this, with a clear write-up and working PoC in the issue.
CertificatePinner.Pin.matchesHostname() compares hostname against pattern with plain string equality (or region matching for wildcard patterns), with no normalization of either side. hostname comes from address.url.host in ConnectPlan, which explicitly preserves a trailing dot for an absolute DNS name. A pin's pattern is developer-authored config and never has one. So a pin for example.com provides no protection at all against a connection to example.com. -- same host at the DNS level, but the pin is silently skipped, and the connection is verified against an ordinary CA-issued certificate instead.
OkHostnameVerifier already treats both spellings as equivalent for exactly this reason (it normalizes both hostname and pattern to absolute form before comparing). CertificatePinner never got the same treatment.
Fix: strip a single trailing dot from hostname at the top of matchesHostname, before the wildcard/exact-match dispatch. All three branches (**., *., and exact) already reference the same hostname identifier, so this one change covers them uniformly. pattern is left untouched -- it is never legitimately going to carry a trailing dot in practice, and the reported issue is specifically about the hostname side.
Added testMatchesHostnameWithTrailingDot to CertificatePinnerKotlinTest, covering both the exact-match and wildcard branches directly on Pin.matchesHostname, plus the reporter's own findMatchingPins() shape.
Verified with a decisive negative control: reverting only the source fix (keeping the new test) reproduces the exact reported behavior -- the test fails with "expected: but was: " on the trailing-dot assertion, confirming the old code silently skips the pin for the dotted hostname.