From 8cb0367b3b4bf684309cb6e849afefa473d7ebe0 Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Wed, 25 Mar 2020 16:07:42 +0000 Subject: OWNERS: tobiast -> ngeoffray. This CL topic was created via: find . -name OWNERS | xargs grep -l tobiast | xargs sed \ -i 's/tobiast/ngeoffray/g' Plus appropriate repo start, git commit, repo upload commands: while read proj; do croot $proj ; repo start OWNERS-tobiast-ngeoffray ; \ git commit -F ~/commit-message.txt; croot ; done \ < ~/owners-projects.txt repo upload -t --br=OWNERS-tobiast-ngeoffray No attempt was made to sort the files after the replacement. Test: Manually inspected the result. Bug: 152399425 Change-Id: I817d23c1f5497fee70ea3ab2e9d5626ae51cca2b --- OWNERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OWNERS b/OWNERS index 9cd1768..dc99a81 100644 --- a/OWNERS +++ b/OWNERS @@ -1,3 +1,3 @@ # Bug component: 24949 -tobiast@google.com +ngeoffray@google.com include platform/libcore:/OWNERS -- cgit v1.2.3 From b1824d35c961cec61c3e6812b629d6858cf6312e Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Fri, 3 Apr 2020 14:31:35 +0100 Subject: Remove testdex version. Unused anymore. Test: m Change-Id: I0d1de5338dc3fe4c241bf52af451a0db022483c4 --- Android.bp | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/Android.bp b/Android.bp index 18fbd41..b18fe91 100644 --- a/Android.bp +++ b/Android.bp @@ -88,29 +88,6 @@ java_library { ], } -// A guaranteed unstripped version of okhttp. -// The build system may or may not strip the okhttp jar, but this one will -// not be stripped. See b/24535627. -java_library { - name: "okhttp-testdex", - visibility: [ - "//art:__subpackages__", - ], - static_libs: ["okhttp"], - - installable: true, - - sdk_version: "none", - system_modules: "core-all-system-modules", - libs: [ - "conscrypt.module.intra.core.api.stubs", - ], - dex_preopt: { - enabled: false, - }, - java_version: "1.7", -} - java_library { name: "okhttp-tests-nojarjar", visibility: nojarjar_visibility, -- cgit v1.2.3 From 30474ef7d467ebf21b35af2d2183a1a2acc253a7 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 8 Apr 2020 13:55:30 +0100 Subject: Depend on conscrypt.module.intra.core.api not stubs Changed the dependency from the stubs module created by the conscrypt.module.intra.core.api java_sdk_library to the conscrypt.module.intra.core.api java_sdk_library itself. Needed to allow the conscrypt.module.intra.core.api to be exported from conscrypt-module-sdk instead of the stubs module. Bug: 153443117 Test: m okhttp-nojarjar Manually verified that the classpath was the same before and after the change. Verified that while Soong rebuilt the ninja file after this change it did not need to rebuild the okhttp-nojarjar which indicates that ninja determined that the command line to build it had not changed. Change-Id: I877ff8b0447069f7dffb56cdf3150cf241514bf2 --- Android.bp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Android.bp b/Android.bp index b18fe91..011ba4e 100644 --- a/Android.bp +++ b/Android.bp @@ -50,7 +50,7 @@ java_library { sdk_version: "none", system_modules: "core-all-system-modules", libs: [ - "conscrypt.module.intra.core.api.stubs", + "conscrypt.module.intra.core.api", ], java_version: "1.7", } @@ -79,7 +79,7 @@ java_library { sdk_version: "none", system_modules: "core-all-system-modules", libs: [ - "conscrypt.module.intra.core.api.stubs", + "conscrypt.module.intra.core.api", ], java_version: "1.7", apex_available: [ @@ -111,7 +111,7 @@ java_library { libs: [ "okhttp-nojarjar", "junit", - "conscrypt.module.intra.core.api.stubs", + "conscrypt.module.intra.core.api", "bouncycastle-unbundled", ], -- cgit v1.2.3 From 1f0cf7d86b4c03b8bd1978f2d2ea4164cbf9c59a Mon Sep 17 00:00:00 2001 From: Bob Badour Date: Tue, 28 Apr 2020 09:10:37 -0700 Subject: Add METADATA to okhttp: Apache2=NOTICE Bug: 68860345 Bug: 69058154 Bug: 151953481 Test: no code changes Change-Id: If935d149db451e5f8f4aa76f0ce310a2a30a8b22 --- METADATA | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 METADATA diff --git a/METADATA b/METADATA new file mode 100644 index 0000000..d97975c --- /dev/null +++ b/METADATA @@ -0,0 +1,3 @@ +third_party { + license_type: NOTICE +} -- cgit v1.2.3 From a006ad85ace0355c59776ea863b2591f98bef5c8 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 26 Jun 2020 09:14:49 -0700 Subject: Compile okhttp for host Bug: 148404241 Test: mma Change-Id: Ibb7a1168b198777a21b98c2a347f8c5adca23570 --- Android.bp | 66 +++++++++++++++++----- .../src/main/java/android/util/Log.java | 26 +++++++++ 2 files changed, 78 insertions(+), 14 deletions(-) create mode 100644 okhttp-android-util-log/src/main/java/android/util/Log.java diff --git a/Android.bp b/Android.bp index 011ba4e..4538995 100644 --- a/Android.bp +++ b/Android.bp @@ -57,30 +57,46 @@ java_library { java_library { name: "okhttp", + host_supported: true, visibility: [ "//art/build/apex", "//external/robolectric-shadows", "//libcore", ], - srcs: [ - // Although some of the classes in the android/ directory are already in the correct - // package and do not need to be moved to another package they are transformed as they - // reference other classes that do require repackaging. - "repackaged/android/src/main/java/**/*.java", - "repackaged/okhttp/src/main/java/**/*.java", - "repackaged/okhttp-urlconnection/src/main/java/**/*.java", - "repackaged/okhttp-android-support/src/main/java/**/*.java", - "repackaged/okio/okio/src/main/java/**/*.java", - ], + target: { + android: { + srcs: [ + // Although some of the classes in the android/ directory are already in the correct + // package and do not need to be moved to another package they are transformed as they + // reference other classes that do require repackaging. + "repackaged/android/src/main/java/**/*.java", + "repackaged/okhttp/src/main/java/**/*.java", + "repackaged/okhttp-urlconnection/src/main/java/**/*.java", + "repackaged/okhttp-android-support/src/main/java/**/*.java", + "repackaged/okio/okio/src/main/java/**/*.java", + ], + libs: [ + "conscrypt.module.intra.core.api", + ], + }, + host: { + srcs: [ + "okhttp/src/main/java/**/*.java", + "okhttp-urlconnection/src/main/java/**/*.java", + "okio/okio/src/main/java/**/*.java", + ":okhttp_version.java", + ], + libs: [ + "okhttp-android-util-log", + ], + }, + }, hostdex: true, installable: true, - sdk_version: "none", system_modules: "core-all-system-modules", - libs: [ - "conscrypt.module.intra.core.api", - ], + sdk_version: "none", java_version: "1.7", apex_available: [ "com.android.art.debug", @@ -88,6 +104,28 @@ java_library { ], } +// Generate Version.java based on the version number from pom.xml. +genrule { + name: "okhttp_version.java", + srcs: [ + "okhttp/src/main/java-templates/com/squareup/okhttp/internal/Version.java", + "okhttp/pom.xml", + ], + out: ["com/squareup/okhttp/internal/Version.java"], + cmd: "grep \"\" $(location okhttp/pom.xml) | head -1 |" + + " sed -e \"s/\\s*\\(.*\\)<\\/version>/\\1/\" > $(genDir)/version && " + + "sed -e \"s/\\$${project.version}/$$(cat $(genDir)/version)/\" " + + " $(location okhttp/src/main/java-templates/com/squareup/okhttp/internal/Version.java) " + + "> $(out)", +} + +// A library to provide a stub android.util.Log symbol for +// okhttp/src/main/java/com/squareup/okhttp/internal/Platform.java +java_library_host { + name: "okhttp-android-util-log", + srcs: ["okhttp-android-util-log/src/main/java/**/*.java"], +} + java_library { name: "okhttp-tests-nojarjar", visibility: nojarjar_visibility, diff --git a/okhttp-android-util-log/src/main/java/android/util/Log.java b/okhttp-android-util-log/src/main/java/android/util/Log.java new file mode 100644 index 0000000..d2d4f55 --- /dev/null +++ b/okhttp-android-util-log/src/main/java/android/util/Log.java @@ -0,0 +1,26 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +package android.util; + +public final class Log { + +private Log() { throw new RuntimeException("Stub!"); } + +public static int d(java.lang.String tag, java.lang.String msg) { throw new RuntimeException("Stub!"); } +} + -- cgit v1.2.3 From 8a32d9f43b58e4974aeca9e470ce7ee147da6d0f Mon Sep 17 00:00:00 2001 From: Hadrien Zalek Date: Mon, 29 Jun 2020 09:39:00 -0700 Subject: Expand the visibility of the OkHttp build module Expand the visibility of the OkHttp build module which is required for compiling the corresponding gRPC Java transport library. Test: m okhttp Bug: 148404241 Change-Id: If9059ce6f89978d65843e9d9b260fc902de094e8 --- Android.bp | 1 + 1 file changed, 1 insertion(+) diff --git a/Android.bp b/Android.bp index 4538995..1b54ffe 100644 --- a/Android.bp +++ b/Android.bp @@ -60,6 +60,7 @@ java_library { host_supported: true, visibility: [ "//art/build/apex", + "//external/grpc-grpc-java/okhttp", "//external/robolectric-shadows", "//libcore", ], -- cgit v1.2.3 From 72ac99d371b9794bd4947ae2794e3ef322e36673 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Fri, 4 Dec 2020 09:25:28 +0000 Subject: Reject non-ASCII hostnames and SANs. Removes the possibility of certificates spoofing DNS names by exploiting name collisions when lowercasing Unicode characters. Note that the relevant RFCs mandate that domain names in certificates should be stored using IDNA 2008 rules, i.e. as ASCII punycode. Bug: 171980069 Test: atest CtsLibcoreTestCases CtsLibcoreOkHttpTestCases Change-Id: I96d52609ce4966ff11f649ca940de3b02a43b0b2 --- .../okhttp/internal/tls/HostnameVerifierTest.java | 67 +++++++++++----------- .../okhttp/internal/tls/OkHostnameVerifier.java | 31 ++++++++++ .../okhttp/internal/tls/OkHostnameVerifier.java | 31 ++++++++++ 3 files changed, 95 insertions(+), 34 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java index 76897fc..0c3d16d 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java @@ -26,7 +26,6 @@ import java.util.Collection; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -168,12 +167,7 @@ public final class HostnameVerifierTest { assertFalse(verifier.verify("a.bar.com", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void verifyNonAsciiSubjectAlt() throws Exception { + @Test public void verifyNonAsciiSubjectAlt() throws Exception { // CN=foo.com, subjectAlt=bar.com, subjectAlt=花子.co.jp // (hanako.co.jp in kanji) SSLSession session = session("" @@ -203,16 +197,15 @@ public final class HostnameVerifierTest { + "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n" + "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); assertFalse(verifier.verify("a.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertTrue(verifier.verify("bar.com", session)); - // assertFalse(verifier.verify("a.bar.com", session)); - // assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + assertTrue(verifier.verify("bar.com", session)); + assertFalse(verifier.verify("a.bar.com", session)); + assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + // Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } @Test public void verifySubjectAltOnly() throws Exception { @@ -358,17 +351,12 @@ public final class HostnameVerifierTest { // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("foo.co.jp", session)); assertFalse(verifier.verify("foo.co.jp", session)); - // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void testWilcardNonAsciiSubjectAlt() throws Exception { + @Test public void testWilcardNonAsciiSubjectAlt() throws Exception { // CN=*.foo.com, subjectAlt=*.bar.com, subjectAlt=*.花子.co.jp // (*.hanako.co.jp in kanji) SSLSession session = session("" @@ -399,19 +387,22 @@ public final class HostnameVerifierTest { + "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n" + "-----END CERTIFICATE-----\n"); // try the foo.com variations - assertTrue(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + // BEGIN Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + // assertTrue(verifier.verify("www.foo.com", session)); + // assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); + // END Android-changed: Ignore common name in hostname verification. http://b/70278814 assertFalse(verifier.verify("a.b.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertFalse(verifier.verify("bar.com", session)); - // assertTrue(verifier.verify("www.bar.com", session)); + // these checks test alternative subjects. + assertFalse(verifier.verify("bar.com", session)); + assertTrue(verifier.verify("www.bar.com", session)); + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.bar.com", session)); - // assertTrue(verifier.verify("a.b.bar.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.bar.com", session)); + assertFalse(verifier.verify("a.b.bar.com", session)); } @Test public void subjectAltUsesLocalDomainAndIp() throws Exception { @@ -605,6 +596,14 @@ public final class HostnameVerifierTest { assertFalse(OkHostnameVerifier.verifyAsIpAddress("www.nintendo.co.jp")); } + @Test public void isPrintableAscii() { + assertTrue(OkHostnameVerifier.isPrintableAscii("foo-bar_baz.com")); + assertTrue(OkHostnameVerifier.isPrintableAscii("FoO-bAr_BaZ.cOm")); + assertFalse(OkHostnameVerifier.isPrintableAscii("Føø-bAr_BaZ.cøm")); + // Char 0xc0 (capital A with grave accent in ISO 8859-1) fits in 8 bits but not 7. + assertFalse(OkHostnameVerifier.isPrintableAscii("\u00c0.com")); + } + private X509Certificate certificate(String certificate) throws Exception { return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate( new ByteArrayInputStream(certificate.getBytes(Util.UTF_8))); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java index d560c62..71d2f8e 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java @@ -107,6 +107,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { * Returns true if {@code certificate} matches {@code hostName}. */ private boolean verifyHostName(String hostName, X509Certificate certificate) { + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(hostName)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 hostName = hostName.toLowerCase(Locale.US); boolean hasDns = false; List altNames = getSubjectAltNames(certificate, ALT_DNS_NAME); @@ -209,6 +214,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { } // hostName and pattern are now absolute domain names. + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(pattern)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 pattern = pattern.toLowerCase(Locale.US); // hostName and pattern are now in lower case -- domain names are case-insensitive. @@ -279,4 +289,25 @@ public final class OkHostnameVerifier implements HostnameVerifier { // hostName matches pattern return true; } + + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + /** + * Returns true if the input string contains only printable 7-bit ASCII + * characters, otherwise false. + */ + private static final char DEL = 127; + static boolean isPrintableAscii(String input) { + if (input == null) { + return false; + } + for (char c : input.toCharArray()) { + // Space is illegal in a DNS name. DEL and anything less than space is non-printing so + // also illegal. Anything greater than DEL is not 7-bit. + if (c <= ' ' || c >= DEL) { + return false; + } + } + return true; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 } diff --git a/repackaged/okhttp/src/main/java/com/android/okhttp/internal/tls/OkHostnameVerifier.java b/repackaged/okhttp/src/main/java/com/android/okhttp/internal/tls/OkHostnameVerifier.java index d37902f..22daecd 100644 --- a/repackaged/okhttp/src/main/java/com/android/okhttp/internal/tls/OkHostnameVerifier.java +++ b/repackaged/okhttp/src/main/java/com/android/okhttp/internal/tls/OkHostnameVerifier.java @@ -109,6 +109,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { * Returns true if {@code certificate} matches {@code hostName}. */ private boolean verifyHostName(String hostName, X509Certificate certificate) { + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(hostName)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 hostName = hostName.toLowerCase(Locale.US); boolean hasDns = false; List altNames = getSubjectAltNames(certificate, ALT_DNS_NAME); @@ -211,6 +216,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { } // hostName and pattern are now absolute domain names. + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(pattern)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 pattern = pattern.toLowerCase(Locale.US); // hostName and pattern are now in lower case -- domain names are case-insensitive. @@ -281,4 +291,25 @@ public final class OkHostnameVerifier implements HostnameVerifier { // hostName matches pattern return true; } + + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + /** + * Returns true if the input string contains only printable 7-bit ASCII + * characters, otherwise false. + */ + private static final char DEL = 127; + static boolean isPrintableAscii(String input) { + if (input == null) { + return false; + } + for (char c : input.toCharArray()) { + // Space is illegal in a DNS name. DEL and anything less than space is non-printing so + // also illegal. Anything greater than DEL is not 7-bit. + if (c <= ' ' || c >= DEL) { + return false; + } + } + return true; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 } -- cgit v1.2.3 From ef3cc00da34dcef024e39bcabcca098e49499b3e Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Thu, 26 Nov 2020 22:33:30 +0000 Subject: [DO NOT MERGE] Reject non-ASCII hostnames and SANs, Ignore DNS names in certificate CN. Removes the possibility of certificates spoofing DNS names by exploiting name collisions when lowercasing Unicode characters. Note that the relevant RFCs mandate that domain names in certificates should be stored using IDNA 2008 rules, i.e. as ASCII punycode. Ignoring DNS names in the certificate CN is a backport of a change made in Android P as this behaviour has been deprecated for many years. The P change was not gated on targetSdkVersion, so all apps on P or later ignore the CN field when verifiying hostnames, this change bring O MR1 into line with best practices without introducing significant fragmentation. The change makes hostname verification consistent across all releases with security patch support. Bug: 171980069 Test: cts-tradefed cts run -m CtsLibcoreTestCases Test: cts-tradefed cts run -m CtsLibcoreOkHttpTestCases Change-Id: I6be13c94b2db22229093d2f0403337798c3ad9ad Merged-In: I96d52609ce4966ff11f649ca940de3b02a43b0b2 --- .../okhttp/internal/tls/HostnameVerifierTest.java | 99 +++++++++++++--------- .../okhttp/internal/tls/OkHostnameVerifier.java | 36 +++++++- 2 files changed, 92 insertions(+), 43 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java index d7f1c78..d2b4a83 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java @@ -24,7 +24,6 @@ import java.security.cert.X509Certificate; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; -import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertEquals; @@ -36,7 +35,7 @@ import static org.junit.Assert.assertTrue; * itself includes tests from the Apache HTTP Client test suite. */ public final class HostnameVerifierTest { - private HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; + private final HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; @Test public void verify() throws Exception { FakeSSLSession session = new FakeSSLSession(); @@ -71,7 +70,9 @@ public final class HostnameVerifierTest { + "HwlNrAu8jlZ2UqSgskSWlhYdMTAP9CPHiUv9N7FcT58Itv/I4fKREINQYjDpvQcx\n" + "SaTYb9dr5sB4WLNglk7zxDtM80H518VvihTcP7FHL+Gn6g4j5fkI98+S\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); assertFalse(verifier.verify("a.foo.com", session)); assertFalse(verifier.verify("bar.com", session)); } @@ -104,7 +105,9 @@ public final class HostnameVerifierTest { + "9BsO7qe46hidgn39hKh1WjKK2VcL/3YRsC4wUi0PBtFW6ScMCuMhgIRXSPU55Rae\n" + "UIlOdPjjr1SUNWGId1rD7W16Scpwnknn310FNxFMHVI0GTGFkNdkilNCFJcIoRA=\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); } @@ -143,12 +146,7 @@ public final class HostnameVerifierTest { assertFalse(verifier.verify("a.bar.com", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void verifyNonAsciiSubjectAlt() throws Exception { + @Test public void verifyNonAsciiSubjectAlt() throws Exception { // CN=foo.com, subjectAlt=bar.com, subjectAlt=花子.co.jp // (hanako.co.jp in kanji) SSLSession session = session("" @@ -178,16 +176,15 @@ public final class HostnameVerifierTest { + "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n" + "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); assertFalse(verifier.verify("a.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertTrue(verifier.verify("bar.com", session)); - // assertFalse(verifier.verify("a.bar.com", session)); - // assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + assertTrue(verifier.verify("bar.com", session)); + assertFalse(verifier.verify("a.bar.com", session)); + assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + // Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } @Test public void verifySubjectAltOnly() throws Exception { @@ -257,7 +254,9 @@ public final class HostnameVerifierTest { assertFalse(verifier.verify("a.foo.com", session)); assertFalse(verifier.verify("bar.com", session)); assertFalse(verifier.verify("a.bar.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); } @@ -290,8 +289,12 @@ public final class HostnameVerifierTest { + "l3Q/RK95bnA6cuRClGusLad0e6bjkBzx/VQ3VarDEpAkTLUGVAa0CLXtnyc=\n" + "-----END CERTIFICATE-----\n"); assertFalse(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify("www.foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); assertFalse(verifier.verify("a.b.foo.com", session)); } @@ -324,16 +327,15 @@ public final class HostnameVerifierTest { + "UGPLEUDzRHMPHLnSqT1n5UU5UDRytbjJPXzF+l/+WZIsanefWLsxnkgAuZe/oMMF\n" + "EJMryEzOjg4Tfuc5qM0EXoPcQ/JlheaxZ40p2IyHqbsWV4MRYuFH4bkM\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.co.jp", session)); - assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.co.jp", session)); + assertFalse(verifier.verify("foo.co.jp", session)); + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 + // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void testWilcardNonAsciiSubjectAlt() throws Exception { + @Test public void testWilcardNonAsciiSubjectAlt() throws Exception { // CN=*.foo.com, subjectAlt=*.bar.com, subjectAlt=*.花子.co.jp // (*.hanako.co.jp in kanji) SSLSession session = session("" @@ -364,19 +366,22 @@ public final class HostnameVerifierTest { + "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n" + "-----END CERTIFICATE-----\n"); // try the foo.com variations - assertTrue(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + // BEGIN Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + // assertTrue(verifier.verify("www.foo.com", session)); + // assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); + // END Android-changed: Ignore common name in hostname verification. http://b/70278814 assertFalse(verifier.verify("a.b.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertFalse(verifier.verify("bar.com", session)); - // assertTrue(verifier.verify("www.bar.com", session)); + // these checks test alternative subjects. + assertFalse(verifier.verify("bar.com", session)); + assertTrue(verifier.verify("www.bar.com", session)); + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.bar.com", session)); - // assertTrue(verifier.verify("a.b.bar.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.bar.com", session)); + assertFalse(verifier.verify("a.b.bar.com", session)); } @Test public void subjectAltUsesLocalDomainAndIp() throws Exception { @@ -451,7 +456,9 @@ public final class HostnameVerifierTest { + "U6LFxmZr31lFyis2/T68PpjAppc0DpNQuA2m/Y7oTHBDi55Fw6HVHCw3lucuWZ5d\n" + "qUYo4ES548JdpQtcLrW2sA==\n" + "-----END CERTIFICATE-----"); - assertTrue(verifier.verify("google.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("google.com", session)); + assertFalse(verifier.verify("google.com", session)); } @Test public void subjectAltName() throws Exception { @@ -542,6 +549,14 @@ public final class HostnameVerifierTest { assertFalse(OkHostnameVerifier.verifyAsIpAddress("www.nintendo.co.jp")); } + @Test public void isPrintableAscii() { + assertTrue(OkHostnameVerifier.isPrintableAscii("foo-bar_baz.com")); + assertTrue(OkHostnameVerifier.isPrintableAscii("FoO-bAr_BaZ.cOm")); + assertFalse(OkHostnameVerifier.isPrintableAscii("Føø-bAr_BaZ.cøm")); + // Char 0xc0 (capital A with grave accent in ISO 8859-1) fits in 8 bits but not 7. + assertFalse(OkHostnameVerifier.isPrintableAscii("\u00c0.com")); + } + private X509Certificate certificate(String certificate) throws Exception { return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate( new ByteArrayInputStream(certificate.getBytes(Util.UTF_8))); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java index 740de1b..399614b 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java @@ -29,7 +29,6 @@ import java.util.regex.Pattern; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLException; import javax.net.ssl.SSLSession; -import javax.security.auth.x500.X500Principal; /** * A HostnameVerifier consistent with altNames = getSubjectAltNames(certificate, ALT_DNS_NAME); @@ -105,6 +109,8 @@ public final class OkHostnameVerifier implements HostnameVerifier { } } + // BEGIN Android-removed: Ignore common name in hostname verification. http://b/70278814 + /* if (!hasDns) { X500Principal principal = certificate.getSubjectX500Principal(); // RFC 2818 advises using the most specific name for matching. @@ -113,6 +119,8 @@ public final class OkHostnameVerifier implements HostnameVerifier { return verifyHostName(hostName, cn); } } + */ + // END Android-removed: Ignore common name in hostname verification. http://b/70278814 return false; } @@ -193,6 +201,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { } // hostName and pattern are now absolute domain names. + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(pattern)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 pattern = pattern.toLowerCase(Locale.US); // hostName and pattern are now in lower case -- domain names are case-insensitive. @@ -249,4 +262,25 @@ public final class OkHostnameVerifier implements HostnameVerifier { // hostName matches pattern return true; } + + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + /** + * Returns true if the input string contains only printable 7-bit ASCII + * characters, otherwise false. + */ + private static final char DEL = 127; + static boolean isPrintableAscii(String input) { + if (input == null) { + return false; + } + for (char c : input.toCharArray()) { + // Space is illegal in a DNS name. DEL and anything less than space is non-printing so + // also illegal. Anything greater than DEL is not 7-bit. + if (c <= ' ' || c >= DEL) { + return false; + } + } + return true; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 } -- cgit v1.2.3 From 815aa54b4d36ededfe584cb28b2058fe2b70ebe0 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Sun, 29 Nov 2020 21:09:53 +0000 Subject: [DO NOT MERGE] Reject non-ASCII hostnames and SANs. Removes the possibility of certificates spoofing DNS names by exploiting name collisions when lowercasing Unicode characters. Note that the relevant RFCs mandate that domain names in certificates should be stored using IDNA 2008 rules, i.e. as ASCII punycode. Bug: 171980069 Test: atest CtsLibcoreTestCases CtsLibcoreOkHttpTestCases Change-Id: Idc6db2ce08eb9e8c6af7c587b0787bae9902a0c9 Merged-In: I96d52609ce4966ff11f649ca940de3b02a43b0b2 --- .../okhttp/internal/tls/HostnameVerifierTest.java | 69 +++++++++++----------- .../okhttp/internal/tls/OkHostnameVerifier.java | 31 ++++++++++ 2 files changed, 65 insertions(+), 35 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java index beb2b6c..d2b4a83 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java @@ -24,7 +24,6 @@ import java.security.cert.X509Certificate; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; -import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertEquals; @@ -36,7 +35,7 @@ import static org.junit.Assert.assertTrue; * itself includes tests from the Apache HTTP Client test suite. */ public final class HostnameVerifierTest { - private HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; + private final HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; @Test public void verify() throws Exception { FakeSSLSession session = new FakeSSLSession(); @@ -147,12 +146,7 @@ public final class HostnameVerifierTest { assertFalse(verifier.verify("a.bar.com", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void verifyNonAsciiSubjectAlt() throws Exception { + @Test public void verifyNonAsciiSubjectAlt() throws Exception { // CN=foo.com, subjectAlt=bar.com, subjectAlt=花子.co.jp // (hanako.co.jp in kanji) SSLSession session = session("" @@ -182,16 +176,15 @@ public final class HostnameVerifierTest { + "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n" + "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); assertFalse(verifier.verify("a.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertTrue(verifier.verify("bar.com", session)); - // assertFalse(verifier.verify("a.bar.com", session)); - // assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + assertTrue(verifier.verify("bar.com", session)); + assertFalse(verifier.verify("a.bar.com", session)); + assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + // Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } @Test public void verifySubjectAltOnly() throws Exception { @@ -337,17 +330,12 @@ public final class HostnameVerifierTest { // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("foo.co.jp", session)); assertFalse(verifier.verify("foo.co.jp", session)); - // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void testWilcardNonAsciiSubjectAlt() throws Exception { + @Test public void testWilcardNonAsciiSubjectAlt() throws Exception { // CN=*.foo.com, subjectAlt=*.bar.com, subjectAlt=*.花子.co.jp // (*.hanako.co.jp in kanji) SSLSession session = session("" @@ -378,19 +366,22 @@ public final class HostnameVerifierTest { + "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n" + "-----END CERTIFICATE-----\n"); // try the foo.com variations - assertTrue(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + // BEGIN Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + // assertTrue(verifier.verify("www.foo.com", session)); + // assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); + // END Android-changed: Ignore common name in hostname verification. http://b/70278814 assertFalse(verifier.verify("a.b.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertFalse(verifier.verify("bar.com", session)); - // assertTrue(verifier.verify("www.bar.com", session)); + // these checks test alternative subjects. + assertFalse(verifier.verify("bar.com", session)); + assertTrue(verifier.verify("www.bar.com", session)); + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.bar.com", session)); - // assertTrue(verifier.verify("a.b.bar.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.bar.com", session)); + assertFalse(verifier.verify("a.b.bar.com", session)); } @Test public void subjectAltUsesLocalDomainAndIp() throws Exception { @@ -558,6 +549,14 @@ public final class HostnameVerifierTest { assertFalse(OkHostnameVerifier.verifyAsIpAddress("www.nintendo.co.jp")); } + @Test public void isPrintableAscii() { + assertTrue(OkHostnameVerifier.isPrintableAscii("foo-bar_baz.com")); + assertTrue(OkHostnameVerifier.isPrintableAscii("FoO-bAr_BaZ.cOm")); + assertFalse(OkHostnameVerifier.isPrintableAscii("Føø-bAr_BaZ.cøm")); + // Char 0xc0 (capital A with grave accent in ISO 8859-1) fits in 8 bits but not 7. + assertFalse(OkHostnameVerifier.isPrintableAscii("\u00c0.com")); + } + private X509Certificate certificate(String certificate) throws Exception { return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate( new ByteArrayInputStream(certificate.getBytes(Util.UTF_8))); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java index c947d7d..399614b 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java @@ -94,6 +94,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { * Returns true if {@code certificate} matches {@code hostName}. */ private boolean verifyHostName(String hostName, X509Certificate certificate) { + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(hostName)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 hostName = hostName.toLowerCase(Locale.US); boolean hasDns = false; List altNames = getSubjectAltNames(certificate, ALT_DNS_NAME); @@ -196,6 +201,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { } // hostName and pattern are now absolute domain names. + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(pattern)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 pattern = pattern.toLowerCase(Locale.US); // hostName and pattern are now in lower case -- domain names are case-insensitive. @@ -252,4 +262,25 @@ public final class OkHostnameVerifier implements HostnameVerifier { // hostName matches pattern return true; } + + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + /** + * Returns true if the input string contains only printable 7-bit ASCII + * characters, otherwise false. + */ + private static final char DEL = 127; + static boolean isPrintableAscii(String input) { + if (input == null) { + return false; + } + for (char c : input.toCharArray()) { + // Space is illegal in a DNS name. DEL and anything less than space is non-printing so + // also illegal. Anything greater than DEL is not 7-bit. + if (c <= ' ' || c >= DEL) { + return false; + } + } + return true; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 } -- cgit v1.2.3 From 065c46754c030ce129760f8ed0ba0d5fc3a49082 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Thu, 3 Dec 2020 19:55:51 +0000 Subject: [DO NOT MERGE] Reject non-ASCII hostnames and SANs. Removes the possibility of certificates spoofing DNS names by exploiting name collisions when lowercasing Unicode characters. Note that the relevant RFCs mandate that domain names in certificates should be stored using IDNA 2008 rules, i.e. as ASCII punycode. Bug: 171980069 Test: atest CtsLibcoreTestCases CtsLibcoreOkHttpTestCases Change-Id: I5f03c040be89c5e6bcf92b120fea99156e4bf5ba Merged-In: I96d52609ce4966ff11f649ca940de3b02a43b0b2 --- .../okhttp/internal/tls/HostnameVerifierTest.java | 69 +++++++++++----------- .../okhttp/internal/tls/OkHostnameVerifier.java | 31 ++++++++++ 2 files changed, 65 insertions(+), 35 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java index beb2b6c..d2b4a83 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/tls/HostnameVerifierTest.java @@ -24,7 +24,6 @@ import java.security.cert.X509Certificate; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; -import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertEquals; @@ -36,7 +35,7 @@ import static org.junit.Assert.assertTrue; * itself includes tests from the Apache HTTP Client test suite. */ public final class HostnameVerifierTest { - private HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; + private final HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; @Test public void verify() throws Exception { FakeSSLSession session = new FakeSSLSession(); @@ -147,12 +146,7 @@ public final class HostnameVerifierTest { assertFalse(verifier.verify("a.bar.com", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void verifyNonAsciiSubjectAlt() throws Exception { + @Test public void verifyNonAsciiSubjectAlt() throws Exception { // CN=foo.com, subjectAlt=bar.com, subjectAlt=花子.co.jp // (hanako.co.jp in kanji) SSLSession session = session("" @@ -182,16 +176,15 @@ public final class HostnameVerifierTest { + "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n" + "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); + // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); assertFalse(verifier.verify("a.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertTrue(verifier.verify("bar.com", session)); - // assertFalse(verifier.verify("a.bar.com", session)); - // assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + assertTrue(verifier.verify("bar.com", session)); + assertFalse(verifier.verify("a.bar.com", session)); + assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + // Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } @Test public void verifySubjectAltOnly() throws Exception { @@ -337,17 +330,12 @@ public final class HostnameVerifierTest { // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("foo.co.jp", session)); assertFalse(verifier.verify("foo.co.jp", session)); - // Android-changed: Ignore common name in hostname verification. http://b/70278814 + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } - /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII - * subject alt names are parsed. Android fails to parse these, which means we - * fall back to the CN. The RI does parse them, so the CN is unused. - */ - @Test @Ignore public void testWilcardNonAsciiSubjectAlt() throws Exception { + @Test public void testWilcardNonAsciiSubjectAlt() throws Exception { // CN=*.foo.com, subjectAlt=*.bar.com, subjectAlt=*.花子.co.jp // (*.hanako.co.jp in kanji) SSLSession session = session("" @@ -378,19 +366,22 @@ public final class HostnameVerifierTest { + "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n" + "-----END CERTIFICATE-----\n"); // try the foo.com variations - assertTrue(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + // BEGIN Android-changed: Ignore common name in hostname verification. http://b/70278814 + // assertTrue(verifier.verify("foo.com", session)); + // assertTrue(verifier.verify("www.foo.com", session)); + // assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify("foo.com", session)); + assertFalse(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); + // END Android-changed: Ignore common name in hostname verification. http://b/70278814 assertFalse(verifier.verify("a.b.foo.com", session)); - // these checks test alternative subjects. The test data contains an - // alternative subject starting with a japanese kanji character. This is - // not supported by Android because the underlying implementation from - // harmony follows the definition from rfc 1034 page 10 for alternative - // subject names. This causes the code to drop all alternative subjects. - // assertFalse(verifier.verify("bar.com", session)); - // assertTrue(verifier.verify("www.bar.com", session)); + // these checks test alternative subjects. + assertFalse(verifier.verify("bar.com", session)); + assertTrue(verifier.verify("www.bar.com", session)); + // Android-changed: Reject non-ASCII hostnames and SANs. http://b/171980069 // assertTrue(verifier.verify("\u82b1\u5b50.bar.com", session)); - // assertTrue(verifier.verify("a.b.bar.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.bar.com", session)); + assertFalse(verifier.verify("a.b.bar.com", session)); } @Test public void subjectAltUsesLocalDomainAndIp() throws Exception { @@ -558,6 +549,14 @@ public final class HostnameVerifierTest { assertFalse(OkHostnameVerifier.verifyAsIpAddress("www.nintendo.co.jp")); } + @Test public void isPrintableAscii() { + assertTrue(OkHostnameVerifier.isPrintableAscii("foo-bar_baz.com")); + assertTrue(OkHostnameVerifier.isPrintableAscii("FoO-bAr_BaZ.cOm")); + assertFalse(OkHostnameVerifier.isPrintableAscii("Føø-bAr_BaZ.cøm")); + // Char 0xc0 (capital A with grave accent in ISO 8859-1) fits in 8 bits but not 7. + assertFalse(OkHostnameVerifier.isPrintableAscii("\u00c0.com")); + } + private X509Certificate certificate(String certificate) throws Exception { return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate( new ByteArrayInputStream(certificate.getBytes(Util.UTF_8))); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java index c947d7d..399614b 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/tls/OkHostnameVerifier.java @@ -94,6 +94,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { * Returns true if {@code certificate} matches {@code hostName}. */ private boolean verifyHostName(String hostName, X509Certificate certificate) { + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(hostName)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 hostName = hostName.toLowerCase(Locale.US); boolean hasDns = false; List altNames = getSubjectAltNames(certificate, ALT_DNS_NAME); @@ -196,6 +201,11 @@ public final class OkHostnameVerifier implements HostnameVerifier { } // hostName and pattern are now absolute domain names. + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + if (!isPrintableAscii(pattern)) { + return false; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 pattern = pattern.toLowerCase(Locale.US); // hostName and pattern are now in lower case -- domain names are case-insensitive. @@ -252,4 +262,25 @@ public final class OkHostnameVerifier implements HostnameVerifier { // hostName matches pattern return true; } + + // BEGIN Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 + /** + * Returns true if the input string contains only printable 7-bit ASCII + * characters, otherwise false. + */ + private static final char DEL = 127; + static boolean isPrintableAscii(String input) { + if (input == null) { + return false; + } + for (char c : input.toCharArray()) { + // Space is illegal in a DNS name. DEL and anything less than space is non-printing so + // also illegal. Anything greater than DEL is not 7-bit. + if (c <= ' ' || c >= DEL) { + return false; + } + } + return true; + } + // END Android-added: Reject non-ASCII hostnames and SANs. http://b/171980069 } -- cgit v1.2.3