Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit f574ea2

Browse filesBrowse files
yschimkesquarejesse
yschimke
and
squarejesse
authored
Cherry pick fix for CVE-2021-0341 onto 4.9.x (#6741)
* Use generated certificates in unit tests (#6346) * Use generated certificates in unit tests * Strict to ascii lowercase implementation Co-authored-by: Jesse Wilson <jwilson@squareup.com> * More restrictive behaviour of OkHostnameVerifier (#6353) * Test quirks of HostnameVerifier. * Restrict successful results to ascii input. Co-authored-by: Jesse Wilson <jwilson@squareup.com>
1 parent 1fd7c0a commit f574ea2
Copy full SHA for f574ea2

File tree

Expand file treeCollapse file tree

3 files changed

+207
-26
lines changed
Filter options
Expand file treeCollapse file tree

3 files changed

+207
-26
lines changed

‎okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt

Copy file name to clipboardExpand all lines: okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt
+28-7Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,15 @@
1616
*/
1717
package okhttp3.internal.tls
1818

19+
import okhttp3.internal.canParseAsIpAddress
20+
import okhttp3.internal.toCanonicalHost
21+
import okio.utf8Size
1922
import java.security.cert.CertificateParsingException
2023
import java.security.cert.X509Certificate
2124
import java.util.Locale
2225
import javax.net.ssl.HostnameVerifier
2326
import javax.net.ssl.SSLException
2427
import javax.net.ssl.SSLSession
25-
import okhttp3.internal.canParseAsIpAddress
26-
import okhttp3.internal.toCanonicalHost
2728

2829
/**
2930
* A HostnameVerifier consistent with [RFC 2818][rfc_2818].
@@ -36,11 +37,16 @@ object OkHostnameVerifier : HostnameVerifier {
3637
private const val ALT_IPA_NAME = 7
3738

3839
override fun verify(host: String, session: SSLSession): Boolean {
39-
return try {
40-
verify(host, session.peerCertificates[0] as X509Certificate)
41-
} catch (_: SSLException) {
40+
return if (!host.isAscii()) {
4241
false
42+
} else {
43+
try {
44+
verify(host, session.peerCertificates[0] as X509Certificate)
45+
} catch (_: SSLException) {
46+
false
47+
}
4348
}
49+
4450
}
4551

4652
fun verify(host: String, certificate: X509Certificate): Boolean {
@@ -61,12 +67,27 @@ object OkHostnameVerifier : HostnameVerifier {
6167

6268
/** Returns true if [certificate] matches [hostname]. */
6369
private fun verifyHostname(hostname: String, certificate: X509Certificate): Boolean {
64-
val hostname = hostname.toLowerCase(Locale.US)
70+
val hostname = hostname.asciiToLowercase()
6571
return getSubjectAltNames(certificate, ALT_DNS_NAME).any {
6672
verifyHostname(hostname, it)
6773
}
6874
}
6975

76+
/**
77+
* This is like [toLowerCase] except that it does nothing if this contains any non-ASCII
78+
* characters. We want to avoid lower casing special chars like U+212A (Kelvin symbol) because
79+
* they can return ASCII characters that match real hostnames.
80+
*/
81+
private fun String.asciiToLowercase(): String {
82+
return when {
83+
isAscii() -> toLowerCase(Locale.US) // This is an ASCII string.
84+
else -> this
85+
}
86+
}
87+
88+
/** Returns true if the [String] is ASCII encoded (0-127). */
89+
private fun String.isAscii() = length == utf8Size().toInt()
90+
7091
/**
7192
* Returns true if [hostname] matches the domain name [pattern].
7293
*
@@ -108,7 +129,7 @@ object OkHostnameVerifier : HostnameVerifier {
108129
}
109130
// Hostname and pattern are now absolute domain names.
110131

111-
pattern = pattern.toLowerCase(Locale.US)
132+
pattern = pattern.asciiToLowercase()
112133
// Hostname and pattern are now in lower case -- domain names are case-insensitive.
113134

114135
if ("*" !in pattern) {

‎okhttp/src/test/java/okhttp3/HttpUrlTest.java

Copy file name to clipboardExpand all lines: okhttp/src/test/java/okhttp3/HttpUrlTest.java
+11Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1773,6 +1773,17 @@ public void unparseableTopPrivateDomain() {
17731773
assertInvalid("http://../", "Invalid URL host: \"..\"");
17741774
}
17751775

1776+
@Test
1777+
public void hostnameTelephone() throws Exception {
1778+
// https://www.gosecure.net/blog/2020/10/27/weakness-in-java-tls-host-verification/
1779+
1780+
// Map the single character telephone symbol (℡) to the string "tel".
1781+
assertThat(parse("http://\u2121").host()).isEqualTo("tel");
1782+
1783+
// Map the Kelvin symbol (K) to the string "k".
1784+
assertThat(parse("http://\u212A").host()).isEqualTo("k");
1785+
}
1786+
17761787
private void assertInvalid(String string, String exceptionMessage) {
17771788
if (useGet) {
17781789
try {

‎okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java

Copy file name to clipboardExpand all lines: okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java
+168-19Lines changed: 168 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,17 @@
1919

2020
import java.io.ByteArrayInputStream;
2121
import java.security.cert.CertificateFactory;
22+
import java.security.cert.CertificateParsingException;
2223
import java.security.cert.X509Certificate;
24+
import java.util.stream.Stream;
2325
import javax.net.ssl.HostnameVerifier;
2426
import javax.net.ssl.SSLSession;
2527
import javax.security.auth.x500.X500Principal;
2628
import okhttp3.FakeSSLSession;
29+
import okhttp3.OkHttpClient;
2730
import okhttp3.internal.Util;
28-
import org.junit.Ignore;
31+
import okhttp3.tls.HeldCertificate;
32+
import okhttp3.tls.internal.TlsUtil;
2933
import org.junit.Test;
3034

3135
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -36,9 +40,9 @@
3640
* from the Apache HTTP Client test suite.
3741
*/
3842
public final class HostnameVerifierTest {
39-
private HostnameVerifier verifier = OkHostnameVerifier.INSTANCE;
43+
private OkHostnameVerifier verifier = OkHostnameVerifier.INSTANCE;
4044

41-
@Test public void verify() throws Exception {
45+
@Test public void verify() {
4246
FakeSSLSession session = new FakeSSLSession();
4347
assertThat(verifier.verify("localhost", session)).isFalse();
4448
}
@@ -148,7 +152,7 @@ public final class HostnameVerifierTest {
148152
* are parsed. Android fails to parse these, which means we fall back to the CN. The RI does parse
149153
* them, so the CN is unused.
150154
*/
151-
@Test @Ignore public void verifyNonAsciiSubjectAlt() throws Exception {
155+
@Test public void verifyNonAsciiSubjectAlt() throws Exception {
152156
// CN=foo.com, subjectAlt=bar.com, subjectAlt=&#x82b1;&#x5b50;.co.jp
153157
// (hanako.co.jp in kanji)
154158
SSLSession session = session(""
@@ -178,16 +182,20 @@ public final class HostnameVerifierTest {
178182
+ "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n"
179183
+ "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n"
180184
+ "-----END CERTIFICATE-----\n");
181-
assertThat(verifier.verify("foo.com", session)).isTrue();
185+
186+
X509Certificate peerCertificate = ((X509Certificate) session.getPeerCertificates()[0]);
187+
assertThat(certificateSANs(peerCertificate)).containsExactly("bar.com", "������.co.jp");
188+
189+
assertThat(verifier.verify("foo.com", session)).isFalse();
182190
assertThat(verifier.verify("a.foo.com", session)).isFalse();
183191
// these checks test alternative subjects. The test data contains an
184192
// alternative subject starting with a japanese kanji character. This is
185193
// not supported by Android because the underlying implementation from
186194
// harmony follows the definition from rfc 1034 page 10 for alternative
187195
// subject names. This causes the code to drop all alternative subjects.
188-
// assertTrue(verifier.verify("bar.com", session));
189-
// assertFalse(verifier.verify("a.bar.com", session));
190-
// assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session));
196+
assertThat(verifier.verify("bar.com", session)).isTrue();
197+
assertThat(verifier.verify("a.bar.com", session)).isFalse();
198+
assertThat(verifier.verify("a.\u82b1\u5b50.co.jp", session)).isFalse();
191199
}
192200

193201
@Test public void verifySubjectAltOnly() throws Exception {
@@ -329,11 +337,11 @@ public final class HostnameVerifierTest {
329337
}
330338

331339
/**
332-
* Ignored due to incompatibilities between Android and Java on how non-ASCII subject alt names
333-
* are parsed. Android fails to parse these, which means we fall back to the CN. The RI does parse
334-
* them, so the CN is unused.
340+
* Previously ignored due to incompatibilities between Android and Java on how non-ASCII subject
341+
* alt names are parsed. Android fails to parse these, which means we fall back to the CN.
342+
* The RI does parse them, so the CN is unused.
335343
*/
336-
@Test @Ignore public void testWilcardNonAsciiSubjectAlt() throws Exception {
344+
@Test public void testWilcardNonAsciiSubjectAlt() throws Exception {
337345
// CN=*.foo.com, subjectAlt=*.bar.com, subjectAlt=*.&#x82b1;&#x5b50;.co.jp
338346
// (*.hanako.co.jp in kanji)
339347
SSLSession session = session(""
@@ -363,20 +371,24 @@ public final class HostnameVerifierTest {
363371
+ "qFr0AIZKBlg6NZZFf/0dP9zcKhzSriW27bY0XfzA6GSiRDXrDjgXq6baRT6YwgIg\n"
364372
+ "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n"
365373
+ "-----END CERTIFICATE-----\n");
374+
375+
X509Certificate peerCertificate = ((X509Certificate) session.getPeerCertificates()[0]);
376+
assertThat(certificateSANs(peerCertificate)).containsExactly("*.bar.com", "*.������.co.jp");
377+
366378
// try the foo.com variations
367-
assertThat(verifier.verify("foo.com", session)).isTrue();
368-
assertThat(verifier.verify("www.foo.com", session)).isTrue();
369-
assertThat(verifier.verify("\u82b1\u5b50.foo.com", session)).isTrue();
379+
assertThat(verifier.verify("foo.com", session)).isFalse();
380+
assertThat(verifier.verify("www.foo.com", session)).isFalse();
381+
assertThat(verifier.verify("\u82b1\u5b50.foo.com", session)).isFalse();
370382
assertThat(verifier.verify("a.b.foo.com", session)).isFalse();
371383
// these checks test alternative subjects. The test data contains an
372384
// alternative subject starting with a japanese kanji character. This is
373385
// not supported by Android because the underlying implementation from
374386
// harmony follows the definition from rfc 1034 page 10 for alternative
375387
// subject names. This causes the code to drop all alternative subjects.
376-
// assertFalse(verifier.verify("bar.com", session));
377-
// assertTrue(verifier.verify("www.bar.com", session));
378-
// assertTrue(verifier.verify("\u82b1\u5b50.bar.com", session));
379-
// assertTrue(verifier.verify("a.b.bar.com", session));
388+
assertThat(verifier.verify("bar.com", session)).isFalse();
389+
assertThat(verifier.verify("www.bar.com", session)).isTrue();
390+
assertThat(verifier.verify("\u82b1\u5b50.bar.com", session)).isFalse();
391+
assertThat(verifier.verify("a.b.bar.com", session)).isFalse();
380392
}
381393

382394
@Test public void subjectAltUsesLocalDomainAndIp() throws Exception {
@@ -554,6 +566,143 @@ public final class HostnameVerifierTest {
554566
assertThat(verifier.verify("0:0:0:0:0:FFFF:C0A8:0101", session)).isTrue();
555567
}
556568

569+
@Test public void generatedCertificate() throws Exception {
570+
HeldCertificate heldCertificate = new HeldCertificate.Builder()
571+
.commonName("Foo Corp")
572+
.addSubjectAlternativeName("foo.com")
573+
.build();
574+
575+
SSLSession session = session(heldCertificate.certificatePem());
576+
assertThat(verifier.verify("foo.com", session)).isTrue();
577+
assertThat(verifier.verify("bar.com", session)).isFalse();
578+
}
579+
580+
@Test public void specialKInHostname() throws Exception {
581+
// https://github.com/apache/httpcomponents-client/commit/303e435d7949652ea77a6c50df1c548682476b6e
582+
// https://www.gosecure.net/blog/2020/10/27/weakness-in-java-tls-host-verification/
583+
584+
HeldCertificate heldCertificate = new HeldCertificate.Builder()
585+
.commonName("Foo Corp")
586+
.addSubjectAlternativeName("k.com")
587+
.addSubjectAlternativeName("tel.com")
588+
.build();
589+
590+
SSLSession session = session(heldCertificate.certificatePem());
591+
assertThat(verifier.verify("foo.com", session)).isFalse();
592+
assertThat(verifier.verify("bar.com", session)).isFalse();
593+
assertThat(verifier.verify("k.com", session)).isTrue();
594+
assertThat(verifier.verify("K.com", session)).isTrue();
595+
596+
assertThat(verifier.verify("\u2121.com", session)).isFalse();
597+
assertThat(verifier.verify("℡.com", session)).isFalse();
598+
599+
// These should ideally be false, but we know that hostname is usually already checked by us
600+
assertThat(verifier.verify("\u212A.com", session)).isFalse();
601+
// Kelvin character below
602+
assertThat(verifier.verify("K.com", session)).isFalse();
603+
}
604+
605+
@Test public void specialKInCert() throws Exception {
606+
// https://github.com/apache/httpcomponents-client/commit/303e435d7949652ea77a6c50df1c548682476b6e
607+
// https://www.gosecure.net/blog/2020/10/27/weakness-in-java-tls-host-verification/
608+
609+
HeldCertificate heldCertificate = new HeldCertificate.Builder()
610+
.commonName("Foo Corp")
611+
.addSubjectAlternativeName("\u2121.com")
612+
.addSubjectAlternativeName("\u212A.com")
613+
.build();
614+
615+
SSLSession session = session(heldCertificate.certificatePem());
616+
assertThat(verifier.verify("foo.com", session)).isFalse();
617+
assertThat(verifier.verify("bar.com", session)).isFalse();
618+
assertThat(verifier.verify("k.com", session)).isFalse();
619+
assertThat(verifier.verify("K.com", session)).isFalse();
620+
621+
assertThat(verifier.verify("tel.com", session)).isFalse();
622+
assertThat(verifier.verify("k.com", session)).isFalse();
623+
}
624+
625+
@Test public void specialKInExternalCert() throws Exception {
626+
// $ cat ./cert.cnf
627+
// [req]
628+
// distinguished_name=distinguished_name
629+
// req_extensions=req_extensions
630+
// x509_extensions=x509_extensions
631+
// [distinguished_name]
632+
// [req_extensions]
633+
// [x509_extensions]
634+
// subjectAltName=DNS:℡.com,DNS:K.com
635+
//
636+
// $ openssl req -x509 -nodes -days 36500 -subj '/CN=foo.com' -config ./cert.cnf \
637+
// -newkey rsa:512 -out cert.pem
638+
SSLSession session = session(""
639+
+ "-----BEGIN CERTIFICATE-----\n"
640+
+ "MIIBSDCB86ADAgECAhRLR4TGgXBegg0np90FZ1KPeWpDtjANBgkqhkiG9w0BAQsF\n"
641+
+ "ADASMRAwDgYDVQQDDAdmb28uY29tMCAXDTIwMTAyOTA2NTkwNVoYDzIxMjAxMDA1\n"
642+
+ "MDY1OTA1WjASMRAwDgYDVQQDDAdmb28uY29tMFwwDQYJKoZIhvcNAQEBBQADSwAw\n"
643+
+ "SAJBALQcTVW9aW++ClIV9/9iSzijsPvQGEu/FQOjIycSrSIheZyZmR8bluSNBq0C\n"
644+
+ "9fpalRKZb0S2tlCTi5WoX8d3K30CAwEAAaMfMB0wGwYDVR0RBBQwEoIH4oShLmNv\n"
645+
+ "bYIH4oSqLmNvbTANBgkqhkiG9w0BAQsFAANBAA1+/eDvSUGv78iEjNW+1w3OPAwt\n"
646+
+ "Ij1qLQ/YI8OogZPMk7YY46/ydWWp7UpD47zy/vKmm4pOc8Glc8MoDD6UADs=\n"
647+
+ "-----END CERTIFICATE-----\n");
648+
649+
X509Certificate peerCertificate = ((X509Certificate) session.getPeerCertificates()[0]);
650+
assertThat(certificateSANs(peerCertificate)).containsExactly("���.com", "���.com");
651+
652+
assertThat(verifier.verify("tel.com", session)).isFalse();
653+
assertThat(verifier.verify("k.com", session)).isFalse();
654+
655+
assertThat(verifier.verify("foo.com", session)).isFalse();
656+
assertThat(verifier.verify("bar.com", session)).isFalse();
657+
assertThat(verifier.verify("k.com", session)).isFalse();
658+
assertThat(verifier.verify("K.com", session)).isFalse();
659+
}
660+
661+
private Stream<String> certificateSANs(X509Certificate peerCertificate)
662+
throws CertificateParsingException {
663+
return peerCertificate.getSubjectAlternativeNames().stream().map(c -> (String) c.get(1));
664+
}
665+
666+
@Test public void replacementCharacter() throws Exception {
667+
// $ cat ./cert.cnf
668+
// [req]
669+
// distinguished_name=distinguished_name
670+
// req_extensions=req_extensions
671+
// x509_extensions=x509_extensions
672+
// [distinguished_name]
673+
// [req_extensions]
674+
// [x509_extensions]
675+
// subjectAltName=DNS:℡.com,DNS:K.com
676+
//
677+
// $ openssl req -x509 -nodes -days 36500 -subj '/CN=foo.com' -config ./cert.cnf \
678+
// -newkey rsa:512 -out cert.pem
679+
SSLSession session = session(""
680+
+ "-----BEGIN CERTIFICATE-----\n"
681+
+ "MIIBSDCB86ADAgECAhRLR4TGgXBegg0np90FZ1KPeWpDtjANBgkqhkiG9w0BAQsF\n"
682+
+ "ADASMRAwDgYDVQQDDAdmb28uY29tMCAXDTIwMTAyOTA2NTkwNVoYDzIxMjAxMDA1\n"
683+
+ "MDY1OTA1WjASMRAwDgYDVQQDDAdmb28uY29tMFwwDQYJKoZIhvcNAQEBBQADSwAw\n"
684+
+ "SAJBALQcTVW9aW++ClIV9/9iSzijsPvQGEu/FQOjIycSrSIheZyZmR8bluSNBq0C\n"
685+
+ "9fpalRKZb0S2tlCTi5WoX8d3K30CAwEAAaMfMB0wGwYDVR0RBBQwEoIH4oShLmNv\n"
686+
+ "bYIH4oSqLmNvbTANBgkqhkiG9w0BAQsFAANBAA1+/eDvSUGv78iEjNW+1w3OPAwt\n"
687+
+ "Ij1qLQ/YI8OogZPMk7YY46/ydWWp7UpD47zy/vKmm4pOc8Glc8MoDD6UADs=\n"
688+
+ "-----END CERTIFICATE-----\n");
689+
690+
// Replacement characters are deliberate, from certificate loading.
691+
assertThat(verifier.verify("���.com", session)).isFalse();
692+
assertThat(verifier.verify("℡.com", session)).isFalse();
693+
}
694+
695+
@Test
696+
public void thatCatchesErrorsWithBadSession() {
697+
HostnameVerifier localVerifier = new OkHttpClient().hostnameVerifier();
698+
699+
// Since this is public API, okhttp3.internal.tls.OkHostnameVerifier.verify is also
700+
assertThat(verifier).isInstanceOf(OkHostnameVerifier.class);
701+
702+
SSLSession session = TlsUtil.localhost().sslContext().createSSLEngine().getSession();
703+
assertThat(localVerifier.verify("\uD83D\uDCA9.com", session)).isFalse();
704+
}
705+
557706
@Test public void verifyAsIpAddress() {
558707
// IPv4
559708
assertThat(Util.canParseAsIpAddress("127.0.0.1")).isTrue();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.