Skip to content

Commit ee12a27

Browse files
committed
Close three ways past the public suffix check
The check landed in 2.16.1 with three gaps, none of them a regression but none closed either. The list algorithm ends by applying a default rule of "*", which was not implemented, so a single label matching no rule was not a suffix. That accepted a Domain naming any of the eight top level domains carrying only a wildcard rule, ck and jm among them. The check lowercased with Locale.ROOT but its caller folded the request host and the Domain with the default locale, so an uppercase pair walked past it under a Turkish locale. Section 5.3 step 5.1.1 keeps a cookie whose Domain equals the request host but makes it host-only. Keeping it as a domain cookie let a host that is itself a public suffix set a cookie every host beneath it receives.
1 parent d1f0cce commit ee12a27

3 files changed

Lines changed: 59 additions & 5 deletions

File tree

‎client/src/main/java/org/asynchttpclient/cookie/PublicSuffixList.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,13 @@ public static boolean isPublicSuffix(String domain) {
119119
// A wildcard rule such as *.ck makes every direct child of ck a suffix, so the candidate is one
120120
// when its parent carries the rule.
121121
int dot = candidate.indexOf('.');
122-
return dot > 0 && WILDCARD.contains(candidate.substring(dot + 1));
122+
if (dot > 0) {
123+
return WILDCARD.contains(candidate.substring(dot + 1));
124+
}
125+
// No rule matched a single label. The algorithm in the list specification ends by applying a default
126+
// rule of "*", which makes any such label a public suffix. That covers the top level domains carrying
127+
// only a wildcard rule and no rule of their own, ck and jm among them, and any delegated after this
128+
// copy of the list was taken.
129+
return true;
123130
}
124131
}

‎client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ public Map<String, Map<CookieKey, StoredCookie>> getUnderlying() {
105105
}
106106

107107
private String requestDomain(Uri requestUri) {
108-
return requestUri.getHost().toLowerCase();
108+
return requestUri.getHost().toLowerCase(Locale.ROOT);
109109
}
110110

111111
private String requestPath(Uri requestUri) {
@@ -116,7 +116,7 @@ private String requestPath(Uri requestUri) {
116116
// Let cookie-domain be the attribute-value without the leading %x2E (".") character.
117117
private AbstractMap.SimpleEntry<String, Boolean> cookieDomain(String cookieDomain, String requestDomain) {
118118
if (cookieDomain != null) {
119-
String normalizedCookieDomain = cookieDomain.toLowerCase();
119+
String normalizedCookieDomain = cookieDomain.toLowerCase(Locale.ROOT);
120120
return new AbstractMap.SimpleEntry<>(
121121
(!cookieDomain.isEmpty() && cookieDomain.charAt(0) == '.') ?
122122
normalizedCookieDomain.substring(1) :
@@ -185,8 +185,14 @@ private void add(String requestDomain, String requestPath, Cookie cookie) {
185185
// The step also keeps a cookie whose Domain equals the request host, as a host-only cookie. Dropping
186186
// it would break ordinary single-label hosts: dev, app, box and cloud are ICANN suffixes as well as
187187
// the short names Docker Compose and Kubernetes hand out.
188-
if (!hostOnly && PublicSuffixList.isPublicSuffix(keyDomain) && !keyDomain.equals(requestDomain)) {
189-
return;
188+
if (!hostOnly && PublicSuffixList.isPublicSuffix(keyDomain)) {
189+
if (!keyDomain.equals(requestDomain)) {
190+
return;
191+
}
192+
// Step 5.1.1: when the Domain names the request host itself the cookie is kept, but as a host-only
193+
// cookie. Keeping it as a domain cookie would let a host that is itself a public suffix, which the
194+
// single-label gTLDs in container and corporate DNS are, set a cookie every host beneath it receives.
195+
hostOnly = true;
190196
}
191197

192198
String keyPath = cookiePath(cookie.path(), requestPath);

‎client/src/test/java/org/asynchttpclient/cookie/PublicSuffixCookieTest.java‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,47 @@ public void matchingDoesNotDependOnTheDefaultLocale() {
110110
}
111111
}
112112

113+
/**
114+
* The list algorithm ends by applying a default rule of {@code *}, so a single label that matches
115+
* nothing is still a public suffix. Without it the eight top-level domains carrying only a wildcard
116+
* rule, ck and jm among them, were accepted as a Domain.
117+
*/
118+
@Test
119+
public void aTopLevelDomainWithOnlyAWildcardRuleIsStillASuffix() {
120+
for (String tld : new String[]{"ck", "er", "fk", "jm", "kh", "mm", "np", "pg"}) {
121+
assertTrue(PublicSuffixList.isPublicSuffix(tld), tld + " must be treated as a public suffix");
122+
}
123+
assertFalse(reaches("evil.foo.ck", "ck", "bank.foo.ck"),
124+
"a host under a wildcard-only TLD must not set a cookie for the TLD");
125+
}
126+
127+
/**
128+
* The uppercase form must be caught too. The check itself lowercases with Locale.ROOT, but the store
129+
* folds the host and the Domain before calling it, and doing that with the default locale let an
130+
* uppercase pair through under Turkish.
131+
*/
132+
@Test
133+
public void anUppercaseDomainIsCaughtWhateverTheLocale() {
134+
Locale original = Locale.getDefault();
135+
try {
136+
Locale.setDefault(new Locale("tr", "TR"));
137+
assertFalse(reaches("EVIL.CO.IN", "CO.IN", "BANK.CO.IN"),
138+
"an uppercase host and Domain must not bypass the check under a Turkish locale");
139+
} finally {
140+
Locale.setDefault(original);
141+
}
142+
}
143+
144+
/**
145+
* Step 5.1.1 keeps such a cookie as host-only, so it must not reach hosts beneath the suffix.
146+
*/
147+
@Test
148+
public void aHostNamedAfterASuffixSetsAHostOnlyCookie() {
149+
assertTrue(reaches("dev", "dev", "dev"), "the host itself must still receive its own cookie");
150+
assertFalse(reaches("dev", "dev", "victim.dev"),
151+
"a host that is itself a public suffix must not set a cookie for everything beneath it");
152+
}
153+
113154
@Test
114155
public void theListRecognisesSuffixesAndRegistrableDomains() {
115156
assertTrue(PublicSuffixList.isPublicSuffix("co.uk"));

0 commit comments

Comments
 (0)