From 719760c251dd30a8f87b7b7214227d6785e53cbc Mon Sep 17 00:00:00 2001 From: Syed Mohammed Nayyar Date: Thu, 20 Aug 2026 19:03:05 +0530 Subject: [PATCH] refuse secure cookies planted over a plaintext connection --- .../cookie/ThreadSafeCookieStore.java | 44 +++++++- .../cookie/SecureCookieSchemeTest.java | 103 ++++++++++++++++++ 2 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 client/src/test/java/org/asynchttpclient/cookie/SecureCookieSchemeTest.java diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 311e70a7c..107da2115 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -58,7 +58,7 @@ public void add(Uri uri, Cookie cookie) { String thisRequestDomain = requestDomain(uri); String thisRequestPath = requestPath(uri); - add(thisRequestDomain, thisRequestPath, cookie); + add(thisRequestDomain, thisRequestPath, uri.isSecured(), cookie); } @Override @@ -189,7 +189,14 @@ private static boolean pathsMatch(String cookiePath, String requestPath) { requestPath.startsWith(cookiePath) && (cookiePath.charAt(cookiePath.length() - 1) == '/' || requestPath.charAt(cookiePath.length()) == '/'); } - private void add(String requestDomain, String requestPath, Cookie cookie) { + private void add(String requestDomain, String requestPath, boolean requestSecure, Cookie cookie) { + // rfc6265bis#section-5.7 step 14: a Secure cookie is only honoured when it arrives over a secure + // scheme. Otherwise anyone on the plaintext path of http://example.com can plant or overwrite the + // cookie the site only ever sets inside TLS, and the next https request carries it back. + if (cookie.isSecure() && !requestSecure) { + return; + } + AbstractMap.SimpleEntry pair = cookieDomain(cookie.domain(), requestDomain); String keyDomain = pair.getKey(); boolean hostOnly = pair.getValue(); @@ -217,6 +224,13 @@ private void add(String requestDomain, String requestPath, Cookie cookie) { String keyPath = cookiePath(cookie.path(), requestPath); CookieKey key = new CookieKey(cookie.name().toLowerCase(), keyPath); + // rfc6265bis#section-5.7 step 22: a non-Secure cookie from a non-secure scheme must not overlay an + // existing Secure cookie either, or the step above is sidestepped by dropping the attribute. This + // sits before the expiry branch so a plaintext Max-Age=0 cannot delete a Secure cookie. + if (!requestSecure && shadowsSecureCookie(keyDomain, key)) { + return; + } + if (hasCookieExpired(cookie, 0)) { cookieJar.getOrDefault(keyDomain, Collections.emptyMap()).remove(key); } else { @@ -264,6 +278,32 @@ private static void evictExcessCookies(Map innerMap) { } } + /** + * Whether the store holds a live Secure cookie the new, non-Secure cookie would overlay: same name, + * a domain that domain-matches the new cookie's domain in either direction, and a path the new + * cookie's path path-matches. The path test is deliberately one-way, so a non-Secure cookie for /foo + * is still allowed next to a Secure one for /login (rfc6265bis#section-5.7 step 22). + */ + private boolean shadowsSecureCookie(String cookieDomain, CookieKey newKey) { + for (Map.Entry> domainEntry : cookieJar.entrySet()) { + String storedDomain = domainEntry.getKey(); + if (!domainsMatch(cookieDomain, storedDomain) && !domainsMatch(storedDomain, cookieDomain)) { + continue; + } + for (Map.Entry entry : domainEntry.getValue().entrySet()) { + CookieKey storedKey = entry.getKey(); + StoredCookie storedCookie = entry.getValue(); + if (storedCookie.cookie.isSecure() + && storedKey.name.equals(newKey.name) + && pathsMatch(storedKey.path, newKey.path) + && !hasCookieExpired(storedCookie.cookie, storedCookie.createdAt)) { + return true; + } + } + } + return false; + } + private List get(String domain, String path, boolean secure) { boolean exactDomainMatch = true; String subDomain = domain; diff --git a/client/src/test/java/org/asynchttpclient/cookie/SecureCookieSchemeTest.java b/client/src/test/java/org/asynchttpclient/cookie/SecureCookieSchemeTest.java new file mode 100644 index 000000000..3b430a430 --- /dev/null +++ b/client/src/test/java/org/asynchttpclient/cookie/SecureCookieSchemeTest.java @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. + * + * 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 org.asynchttpclient.cookie; + +import io.netty.handler.codec.http.cookie.ClientCookieDecoder; +import io.netty.handler.codec.http.cookie.Cookie; +import org.asynchttpclient.uri.Uri; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * RFC 6265bis Section 5.7 steps 14 and 22: a Secure cookie is ignored when it arrives over a non-secure + * scheme, and a non-Secure cookie arriving over a non-secure scheme must not overlay a Secure cookie the + * store already holds. Without either rule anyone on the plaintext path of http://example.com can plant or + * replace the cookie the site only ever sets inside TLS. + */ +public class SecureCookieSchemeTest { + + private static void set(ThreadSafeCookieStore store, String url, String setCookie) { + store.add(Uri.create(url), ClientCookieDecoder.LAX.decode(setCookie)); + } + + private static String valueOf(ThreadSafeCookieStore store, String url, String name) { + List cookies = store.get(Uri.create(url)); + return cookies.stream().filter(c -> name.equals(c.name())).map(Cookie::value).findFirst().orElse(null); + } + + @Test + public void aSecureCookieSetOverPlaintextIsIgnored() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + set(store, "http://example.com/", "SID=planted; Path=/; Secure"); + + assertNull(valueOf(store, "https://example.com/", "SID"), + "a Secure cookie received over http must not be served to https"); + assertNull(valueOf(store, "http://example.com/", "SID"), + "a Secure cookie received over http must not be stored at all"); + } + + @Test + public void aPlaintextCookieCannotOverlayASecureCookie() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + set(store, "https://example.com/", "SID=real; Path=/; Secure"); + + set(store, "http://example.com/", "SID=planted; Path=/"); + assertEquals("real", valueOf(store, "https://example.com/", "SID"), + "a non-Secure cookie received over http must not replace a Secure cookie"); + + set(store, "http://www.example.com/", "SID=planted; Domain=example.com; Path=/"); + assertEquals("real", valueOf(store, "https://example.com/", "SID"), + "a domain cookie received over http must not replace a Secure host cookie it domain-matches"); + + set(store, "http://example.com/", "SID=; Path=/; Max-Age=0"); + assertEquals("real", valueOf(store, "https://example.com/", "SID"), + "an expiring cookie received over http must not delete a Secure cookie"); + } + + @Test + public void thePathOverlayRuleIsOneWay() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + set(store, "https://example.com/login", "SID=real; Path=/login; Secure"); + + set(store, "http://example.com/foo", "SID=other; Path=/foo"); + assertEquals("other", valueOf(store, "http://example.com/foo", "SID"), + "a non-Secure cookie on an unrelated path is still stored"); + + set(store, "http://example.com/login/en", "SID=planted; Path=/login/en"); + assertNull(valueOf(store, "http://example.com/login/en", "SID"), + "a non-Secure cookie whose path sits under the Secure cookie's path is ignored"); + assertEquals("real", valueOf(store, "https://example.com/login/en", "SID")); + } + + @Test + public void cookiesSetOverASecureSchemeAreUnaffected() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + set(store, "https://example.com/", "SID=real; Path=/; Secure"); + assertEquals("real", valueOf(store, "https://example.com/", "SID")); + + set(store, "https://example.com/", "SID=renewed; Path=/"); + assertEquals("renewed", valueOf(store, "https://example.com/", "SID"), + "the origin itself may still replace its Secure cookie over https"); + + set(store, "http://example.com/", "lang=en; Path=/"); + assertEquals("en", valueOf(store, "http://example.com/", "lang"), + "an ordinary http cookie with no Secure counterpart is still stored"); + } +}