From 5a7fb1644ae7e2160dca0e11ecfd1f8ec9b316c9 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sat, 25 Jan 2020 15:24:19 +0100 Subject: [PATCH 1/2] Bug fix: fixed handling of private domains by PublicSuffixMatcher --- .../http/conn/util/PublicSuffixMatcher.java | 31 +++++++++++-------- .../conn/util/TestPublicSuffixMatcher.java | 18 ++++++----- .../src/test/resources/suffixlistmatcher.txt | 1 + 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/httpclient/src/main/java/org/apache/http/conn/util/PublicSuffixMatcher.java b/httpclient/src/main/java/org/apache/http/conn/util/PublicSuffixMatcher.java index 2f63c38fc6..4c246e7a18 100644 --- a/httpclient/src/main/java/org/apache/http/conn/util/PublicSuffixMatcher.java +++ b/httpclient/src/main/java/org/apache/http/conn/util/PublicSuffixMatcher.java @@ -97,20 +97,15 @@ public PublicSuffixMatcher(final Collection lists) { } } - private static boolean hasEntry(final Map map, final String rule, final DomainType expectedType) { + private static DomainType findEntry(final Map map, final String rule) { if (map == null) { - return false; + return null; } - final DomainType domainType = map.get(rule); - return domainType == null ? false : expectedType == null || domainType.equals(expectedType); - } - - private boolean hasRule(final String rule, final DomainType expectedType) { - return hasEntry(this.rules, rule, expectedType); + return map.get(rule); } - private boolean hasException(final String exception, final DomainType expectedType) { - return hasEntry(this.exceptions, exception, expectedType); + private static boolean match(final DomainType domainType, final DomainType expectedType) { + return domainType != null && (expectedType == null || domainType.equals(expectedType)); } /** @@ -147,10 +142,15 @@ public String getDomainRoot(final String domain, final DomainType expectedType) while (segment != null) { // An exception rule takes priority over any other matching rule. final String key = IDN.toUnicode(segment); - if (hasException(key, expectedType)) { + final DomainType exceptionRule = findEntry(exceptions, key); + if (match(exceptionRule, expectedType)) { return segment; } - if (hasRule(key, expectedType)) { + final DomainType domainRule = findEntry(rules, key); + if (match(domainRule, expectedType)) { + if (domainRule == DomainType.PRIVATE) { + return segment; + } return result; } @@ -158,7 +158,11 @@ public String getDomainRoot(final String domain, final DomainType expectedType) final String nextSegment = nextdot != -1 ? segment.substring(nextdot + 1) : null; if (nextSegment != null) { - if (hasRule("*." + IDN.toUnicode(nextSegment), expectedType)) { + final DomainType wildcardDomainRule = findEntry(rules, "*." + IDN.toUnicode(nextSegment)); + if (match(wildcardDomainRule, expectedType)) { + if (wildcardDomainRule == DomainType.PRIVATE) { + return segment; + } return result; } } @@ -174,6 +178,7 @@ public String getDomainRoot(final String domain, final DomainType expectedType) // If we did have expectations apparently there was no match return null; } + /** * Tests whether the given domain matches any of entry from the public suffix list. */ diff --git a/httpclient/src/test/java/org/apache/http/conn/util/TestPublicSuffixMatcher.java b/httpclient/src/test/java/org/apache/http/conn/util/TestPublicSuffixMatcher.java index 6640384e67..4823164629 100644 --- a/httpclient/src/test/java/org/apache/http/conn/util/TestPublicSuffixMatcher.java +++ b/httpclient/src/test/java/org/apache/http/conn/util/TestPublicSuffixMatcher.java @@ -57,11 +57,11 @@ public void setUp() throws Exception { @Test public void testGetDomainRootAnyType() { // Private - Assert.assertEquals("example.xx", matcher.getDomainRoot("example.XX")); - Assert.assertEquals("example.xx", matcher.getDomainRoot("www.example.XX")); - Assert.assertEquals("example.xx", matcher.getDomainRoot("www.blah.blah.example.XX")); + Assert.assertEquals("xx", matcher.getDomainRoot("example.XX")); + Assert.assertEquals("xx", matcher.getDomainRoot("www.example.XX")); + Assert.assertEquals("xx", matcher.getDomainRoot("www.blah.blah.example.XX")); + Assert.assertEquals("appspot.com", matcher.getDomainRoot("example.appspot.com")); // Too short - Assert.assertEquals(null, matcher.getDomainRoot("xx")); Assert.assertEquals(null, matcher.getDomainRoot("jp")); Assert.assertEquals(null, matcher.getDomainRoot("ac.jp")); Assert.assertEquals(null, matcher.getDomainRoot("any.tokyo.jp")); @@ -79,11 +79,11 @@ public void testGetDomainRootAnyType() { @Test public void testGetDomainRootOnlyPRIVATE() { // Private - Assert.assertEquals("example.xx", matcher.getDomainRoot("example.XX", DomainType.PRIVATE)); - Assert.assertEquals("example.xx", matcher.getDomainRoot("www.example.XX", DomainType.PRIVATE)); - Assert.assertEquals("example.xx", matcher.getDomainRoot("www.blah.blah.example.XX", DomainType.PRIVATE)); + Assert.assertEquals("xx", matcher.getDomainRoot("example.XX", DomainType.PRIVATE)); + Assert.assertEquals("xx", matcher.getDomainRoot("www.example.XX", DomainType.PRIVATE)); + Assert.assertEquals("xx", matcher.getDomainRoot("www.blah.blah.example.XX", DomainType.PRIVATE)); + Assert.assertEquals("appspot.com", matcher.getDomainRoot("example.appspot.com")); // Too short - Assert.assertEquals(null, matcher.getDomainRoot("xx", DomainType.PRIVATE)); Assert.assertEquals(null, matcher.getDomainRoot("jp", DomainType.PRIVATE)); Assert.assertEquals(null, matcher.getDomainRoot("ac.jp", DomainType.PRIVATE)); Assert.assertEquals(null, matcher.getDomainRoot("any.tokyo.jp", DomainType.PRIVATE)); @@ -128,6 +128,8 @@ public void testMatch() { Assert.assertTrue(matcher.matches(".any.tokyo.jp")); // exception Assert.assertFalse(matcher.matches(".metro.tokyo.jp")); + Assert.assertFalse(matcher.matches(".xx")); + Assert.assertFalse(matcher.matches(".appspot.com")); } @Test diff --git a/httpclient/src/test/resources/suffixlistmatcher.txt b/httpclient/src/test/resources/suffixlistmatcher.txt index 463c8b9afb..b027fe4458 100644 --- a/httpclient/src/test/resources/suffixlistmatcher.txt +++ b/httpclient/src/test/resources/suffixlistmatcher.txt @@ -26,6 +26,7 @@ // ===BEGIN PRIVATE DOMAINS=== xx lan +appspot.com // ===END PRIVATE DOMAINS=== // ===BEGIN ICANN DOMAINS=== From 67b5e2242d9caa5f9c331a8a683fd1c935ddcab0 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sat, 25 Jan 2020 15:49:44 +0100 Subject: [PATCH 2/2] HTTPCLIENT-2047: fixed regression in DefaultHostnameVerifier causing rejection of certs with non-standard domains. This reverts commit e0416f07 --- .../conn/ssl/DefaultHostnameVerifier.java | 4 +-- .../conn/ssl/TestDefaultHostnameVerifier.java | 26 +++++++++++++++++++ .../src/test/resources/suffixlistmatcher.txt | 1 + 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/httpclient/src/main/java/org/apache/http/conn/ssl/DefaultHostnameVerifier.java b/httpclient/src/main/java/org/apache/http/conn/ssl/DefaultHostnameVerifier.java index 4a0ae1fbeb..18dd5dc9ee 100644 --- a/httpclient/src/main/java/org/apache/http/conn/ssl/DefaultHostnameVerifier.java +++ b/httpclient/src/main/java/org/apache/http/conn/ssl/DefaultHostnameVerifier.java @@ -169,7 +169,7 @@ static void matchDNSName(final String host, final List subjectAlts, final SubjectName subjectAlt = subjectAlts.get(i); if (subjectAlt.getType() == SubjectName.DNS) { final String normalizedSubjectAlt = DnsUtils.normalize(subjectAlt.getValue()); - if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt, publicSuffixMatcher, DomainType.ICANN)) { + if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt, publicSuffixMatcher)) { return; } } @@ -182,7 +182,7 @@ static void matchCN(final String host, final String cn, final PublicSuffixMatcher publicSuffixMatcher) throws SSLException { final String normalizedHost = DnsUtils.normalize(host); final String normalizedCn = DnsUtils.normalize(cn); - if (!matchIdentityStrict(normalizedHost, normalizedCn, publicSuffixMatcher, DomainType.ICANN)) { + if (!matchIdentityStrict(normalizedHost, normalizedCn, publicSuffixMatcher)) { throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match " + "common name of the certificate subject: " + cn); } diff --git a/httpclient/src/test/java/org/apache/http/conn/ssl/TestDefaultHostnameVerifier.java b/httpclient/src/test/java/org/apache/http/conn/ssl/TestDefaultHostnameVerifier.java index ec6f2a9071..71bf7e0aa7 100644 --- a/httpclient/src/test/java/org/apache/http/conn/ssl/TestDefaultHostnameVerifier.java +++ b/httpclient/src/test/java/org/apache/http/conn/ssl/TestDefaultHostnameVerifier.java @@ -35,6 +35,7 @@ import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.util.Arrays; +import java.util.Collections; import java.util.List; import javax.net.ssl.SSLException; @@ -375,6 +376,7 @@ public void testHTTPCLIENT_1997_UNKNOWN() { // Only True on all domains (same as Assert.assertTrue(DefaultHostnameVerifier.matchIdentity( "service.apps." + domain, "*.apps." + domain, publicSuffixMatcher, DomainType.UNKNOWN)); Assert.assertTrue(DefaultHostnameVerifier.matchIdentityStrict( "service.apps." + domain, "*.apps." + domain, publicSuffixMatcher, DomainType.UNKNOWN)); } + @Test // Check compressed IPv6 hostname matching public void testHTTPCLIENT_1316() throws Exception{ final String host1 = "2001:0db8:aaaa:bbbb:cccc:0:0:0001"; @@ -417,4 +419,28 @@ public void testExtractCN() throws Exception { } } + @Test + public void testMatchDNSName() throws Exception { + DefaultHostnameVerifier.matchDNSName( + "host.domain.com", + Collections.singletonList(SubjectName.DNS("*.domain.com")), + publicSuffixMatcher); + DefaultHostnameVerifier.matchDNSName( + "host.xx", + Collections.singletonList(SubjectName.DNS("*.xx")), + publicSuffixMatcher); + DefaultHostnameVerifier.matchDNSName( + "host.appspot.com", + Collections.singletonList(SubjectName.DNS("*.appspot.com")), + publicSuffixMatcher); + DefaultHostnameVerifier.matchDNSName( + "demo-s3-bucket.s3.eu-central-1.amazonaws.com", + Collections.singletonList(SubjectName.DNS("*.s3.eu-central-1.amazonaws.com")), + publicSuffixMatcher); + DefaultHostnameVerifier.matchDNSName( + "hostname-workspace-1.local", + Collections.singletonList(SubjectName.DNS("hostname-workspace-1.local")), + publicSuffixMatcher); + } + } diff --git a/httpclient/src/test/resources/suffixlistmatcher.txt b/httpclient/src/test/resources/suffixlistmatcher.txt index b027fe4458..e9377cb5f9 100644 --- a/httpclient/src/test/resources/suffixlistmatcher.txt +++ b/httpclient/src/test/resources/suffixlistmatcher.txt @@ -27,6 +27,7 @@ xx lan appspot.com +s3.eu-central-1.amazonaws.com // ===END PRIVATE DOMAINS=== // ===BEGIN ICANN DOMAINS===