Skip to content

reject hosts that fail IP canonicalization in verifyIpAddress - #9621

Merged
swankjesse merged 1 commit into
lysine-dev:mainfrom
basavaraj-sm05:hostname-ip-canonicalize
Jul 30, 2026
Merged

reject hosts that fail IP canonicalization in verifyIpAddress#9621
swankjesse merged 1 commit into
lysine-dev:mainfrom
basavaraj-sm05:hostname-ip-canonicalize

Conversation

@basavaraj-sm05

Copy link
Copy Markdown
Contributor

OkHostnameVerifier sends any host that looks like an IP address to verifyIpAddress, which canonicalizes the host and compares it against the canonicalized form of each iPAddress SAN in the peer certificate. Both sides of that comparison come from toCanonicalHost, which returns null for anything it can't parse, and a null on the left is never treated as a failure. A host like 1::2::3 gets past the loose canParseAsIpAddress regex but has two :: so it canonicalizes to null, and an iPAddress SAN that carries an address plus a netmask comes back from the JDK as 1.2.3.4/255.255.255.0, which also canonicalizes to null because of the slash. The two nulls compare equal, so verification succeeds against a certificate that has nothing to do with the host. I found it while reading the other toCanonicalHost call sites: Cookie, CertificatePinner, Dns and HttpUrl all reject null right where they get it, and this was the only one that let it through. Bailing out when the host doesn't canonicalize keeps the comparison to addresses we actually understand, and the hosts OkHttp passes in itself already come from HttpUrl in canonical form, so nothing changes for them.

@yschimke

Copy link
Copy Markdown
Collaborator

So is the threat that if you can get a certificate with an invalid ip address, and construct a url with an invalid host ip address, then that would match as null == null?

It seems like a bug, but I can't work out how to exploit it in practice as you only get the certificate back once you have connected, and presumably that IP address isn't usable to connect as it doesn't parse?

@swankjesse swankjesse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test

@swankjesse
swankjesse merged commit f258f7a into lysine-dev:main Jul 30, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants