Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify IPv6 hostnames #5889

Merged
merged 5 commits into from
Mar 22, 2020
Merged

Verify IPv6 hostnames #5889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d88070
Verify IPv6 hostnames
yschimke Mar 22, 2020
5bc03fe
Simpler check
yschimke Mar 22, 2020
c216cfa
Tests
yschimke Mar 22, 2020
e83ce01
Avoid exceptions
yschimke Mar 22, 2020
bca5b1f
Fixes
yschimke Mar 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import javax.net.ssl.HostnameVerifier
import javax.net.ssl.SSLException
import javax.net.ssl.SSLSession
import okhttp3.internal.canParseAsIpAddress
import okhttp3.internal.toCanonicalHost

/**
* A HostnameVerifier consistent with [RFC 2818][rfc_2818].
Expand Down Expand Up @@ -51,8 +52,10 @@ object OkHostnameVerifier : HostnameVerifier {

/** Returns true if [certificate] matches [ipAddress]. */
private fun verifyIpAddress(ipAddress: String, certificate: X509Certificate): Boolean {
val canonicalIpAddress = ipAddress.toCanonicalHost()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW I didn't consider IP Addresses a host path to optimise too much. But that's certainly debatable.


return getSubjectAltNames(certificate, ALT_IPA_NAME).any {
ipAddress.equals(it, ignoreCase = true)
canonicalIpAddress == it.toCanonicalHost()
}
}

Expand Down
36 changes: 36 additions & 0 deletions okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,42 @@ public final class HostnameVerifierTest {
assertThat(verifier.verify("quux.com", session)).isFalse();
}

@Test public void subjectAltNameWithIPAddresses() throws Exception {
// $ cat ./cert.cnf
// [req]
// distinguished_name=distinguished_name
// req_extensions=req_extensions
// x509_extensions=x509_extensions
// [distinguished_name]
// [req_extensions]
// [x509_extensions]
// subjectAltName=IP:0:0:0:0:0:0:0:1,IP:2a03:2880:f003:c07:face:b00c::2,IP:0::5,IP:192.168.1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wanna do an IPv4 mapped address also?
#4451

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you want the behaviour to be for ::ffff:192.168.1.1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will land that as a follow up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it’s the IPv4 address.

Copy link
Collaborator Author

@yschimke yschimke Mar 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What's the reasoning? Are you self signing certificates but don't want to specify both forms of IPv4 and IPV6 (mapped IPv4) and you expect it to still work for either?

I'm nervous about following Postel's law here to be nice. Maybe keeping it explicit is the better option for security?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The canonical form of certain IPv6 addresses (with colons!) is an IPv4 address (no colons!). It’s weird, but it’s how it’s specified and I’ve come to terms with it.

If we’re canonicalizing IP addresses, we should canonicalize all of ’em and test that it works. I expect the test already passes but I’d like coverage of that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ok? #5892

//
// $ openssl req -x509 -nodes -days 36500 -subj '/CN=foo.com' -config ./cert.cnf \
// -newkey rsa:512 -out cert.pem
SSLSession session = session(""
+ "-----BEGIN CERTIFICATE-----\n"
+ "MIIBaDCCARKgAwIBAgIJALxN+AOBVGwQMA0GCSqGSIb3DQEBCwUAMBIxEDAOBgNV\n"
+ "BAMMB2Zvby5jb20wIBcNMjAwMzIyMTEwNDI4WhgPMjEyMDAyMjcxMTA0MjhaMBIx\n"
+ "EDAOBgNVBAMMB2Zvby5jb20wXDANBgkqhkiG9w0BAQEFAANLADBIAkEAlnVbVfQ9\n"
+ "4aYjrPCcFuxOpjXuvyOc9Hcha4K7TfXyfsrjhAvCjCBIT/TiLOUVF3sx4yoCAtX8\n"
+ "wmt404tTbKD6UwIDAQABo0kwRzBFBgNVHREEPjA8hxAAAAAAAAAAAAAAAAAAAAAB\n"
+ "hxAqAyiA8AMMB/rOsAwAAAAChxAAAAAAAAAAAAAAAAAAAAAFhwTAqAEBMA0GCSqG\n"
+ "SIb3DQEBCwUAA0EAPSOYHJh7hB4ElBqTCAFW+T5Y7mXsv9nQjBJ7w0YIw83V2PEI\n"
+ "3KbBIyGTrqHD6lG8QGZy+yNkIcRlodG8OfQRUg==\n"
+ "-----END CERTIFICATE-----");
assertThat(verifier.verify("foo.com", session)).isFalse();
assertThat(verifier.verify("::1", session)).isTrue();
assertThat(verifier.verify("::2", session)).isFalse();
assertThat(verifier.verify("::5", session)).isTrue();
assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c::2", session)).isTrue();
assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c:0:2", session)).isTrue();
assertThat(verifier.verify("2a03:2880:f003:c07:FACE:B00C:0:2", session)).isTrue();
assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c:0:3", session)).isFalse();
assertThat(verifier.verify("127.0.0.1", session)).isFalse();
assertThat(verifier.verify("192.168.1.1", session)).isTrue();
}

@Test public void verifyAsIpAddress() {
// IPv4
assertThat(Util.canParseAsIpAddress("127.0.0.1")).isTrue();
Expand Down