Security: Fix cookie leakage to wrong origins and cookie accept criteria

Previously cookies of foo.bar.example.com were leaked to foo.bar. Additionally, any site could set cookies for any other site. Artax follows newer browser implementations now. Cookies can only be set on domains higher or equal to the current domain, but not on any public suffixes.
amphp · May 9, 2017 · accdada · accdada
1 parent 99b72e4
commit accdada
Show file tree

Hide file tree

Showing 9 changed files with 12,349 additions and 13 deletions.
diff --git a/lib/Client.php b/lib/Client.php
@@ -2,6 +2,7 @@
 
 namespace Amp\Artax;
 
+use Amp\Artax\Cookie\PublicSuffixList;
 use Amp\Deferred,
     Amp\Socket as socket,
     Amp\Artax\Cookie\Cookie,
@@ -642,6 +643,7 @@ private function inflateGzipBody(Response $response) {
     private function storeResponseCookie($requestDomain, $rawCookieStr) {
         try {
             $cookie = CookieParser::parse($rawCookieStr);
+
             if (!$cookie->getDomain()) {
                 $cookie = new Cookie(
                     $cookie->getName(),
@@ -652,7 +654,36 @@ private function storeResponseCookie($requestDomain, $rawCookieStr) {
                     $cookie->getSecure(),
                     $cookie->getHttpOnly()
                 );
+            } else {
+                // https://tools.ietf.org/html/rfc6265#section-4.1.2.3
+                $cookieDomain = $cookie->getDomain();
+
+                // If a domain is set, left dots are ignored and it's always a wildcard
+                $cookieDomain = \ltrim($cookieDomain, ".");
+
+                if ($cookieDomain !== $requestDomain) {
+                    // ignore cookies on domains that are public suffixes
+                    if (PublicSuffixList::isPublicSuffix($cookieDomain)) {
+                        return;
+                    }
+
+                    // cookie origin would not be included when sending the cookie
+                    if (\substr($requestDomain, 0, -\strlen($cookieDomain) - 1) . "." . $cookieDomain !== $requestDomain) {
+                        return;
+                    }
+                }
+
+                $cookie = new Cookie(
+                    $cookie->getName(),
+                    $cookie->getValue(),
+                    $cookie->getExpirationTime(),
+                    $cookie->getPath(),
+                    "." . $cookieDomain, // always add the dot, it's used internally for wildcard matching
+                    $cookie->getSecure(),
+                    $cookie->getHttpOnly()
+                );
             }
+
             $this->cookieJar->store($cookie);
         } catch (\InvalidArgumentException $e) {
             // Ignore malformed Set-Cookie headers

diff --git a/lib/Cookie/ArrayCookieJar.php b/lib/Cookie/ArrayCookieJar.php
@@ -101,25 +101,26 @@ private function clearExpiredCookies() {
 
     /**
      * @link http://tools.ietf.org/html/rfc6265#section-5.1.3
+     * @link http://erik.io/blog/2014/03/04/definitive-guide-to-cookie-domains/
      */
     private function matchesDomain($requestDomain, $cookieDomain) {
-        if ($requestDomain === $cookieDomain) {
-            return $isMatch = true;
+        if ($requestDomain === \ltrim($cookieDomain, ".")) {
+            return true;
         }
 
-        if (!($isWildcardCookieDomain = ($cookieDomain[0] === '.'))) {
-            $isMatch = false;
-        } elseif (filter_var($requestDomain, FILTER_VALIDATE_IP)) {
-            $isMatch = false;
-        } elseif (substr($requestDomain, 0, -\strlen($cookieDomain)) . $cookieDomain === $requestDomain) {
-            $isMatch = true;
-        } elseif (strrpos($cookieDomain, $requestDomain) !== false) {
-            $isMatch = true;
-        } else {
-            $isMatch = false;
+        if (!isset($cookieDomain[0]) || $cookieDomain[0] !== '.') {
+            return false;
         }
 
-        return $isMatch;
+        if (\filter_var($requestDomain, FILTER_VALIDATE_IP)) {
+            return false;
+        }
+
+        if (\substr($requestDomain, 0, -\strlen($cookieDomain)) . $cookieDomain === $requestDomain) {
+            return true;
+        }
+
+        return false;
     }
 
     /**

diff --git a/lib/Cookie/CookieJar.php b/lib/Cookie/CookieJar.php
@@ -5,6 +5,12 @@
 interface CookieJar {
     public function get($domain, $path = '', $name = null);
     public function getAll();
+
+    /**
+     * Note: Implicit domains MUST NOT start with a dot, explicitly set domains MUST start with a dot.
+     *
+     * @param Cookie $cookie
+     */
     public function store(Cookie $cookie);
     public function remove(Cookie $cookie);
     public function removeAll();

diff --git a/lib/Cookie/PublicSuffixList.php b/lib/Cookie/PublicSuffixList.php
@@ -0,0 +1,113 @@
+<?php
+
+namespace Amp\Artax\Cookie;
+
+/** @internal */
+final class PublicSuffixList {
+    private static $initialized = false;
+    private static $suffixPatterns;
+    private static $exceptionPatterns;
+
+    public static function isPublicSuffix($domain) {
+        if (!self::$initialized) {
+            self::readList();
+        }
+
+        if (!self::isValidHostName($domain)) {
+            $domain = \idn_to_ascii($domain);
+
+            if (!self::isValidHostName($domain)) {
+                throw new \InvalidArgumentException("Invalid host name: " . $domain);
+            }
+        }
+
+        $domain = \implode(".", \array_reverse(\explode(".", \trim($domain, "."))));
+
+        foreach (self::$exceptionPatterns as $pattern) {
+            if (\preg_match($pattern, $domain)) {
+                return false;
+            }
+        }
+
+        foreach (self::$suffixPatterns as $pattern) {
+            if (\preg_match($pattern, $domain)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    private static function readList() {
+        $lines = \file(__DIR__ . "/../../res/public_suffix_list.dat", \FILE_IGNORE_NEW_LINES | \FILE_SKIP_EMPTY_LINES);
+
+        $exceptions = [];
+        $rules = [];
+
+        foreach ($lines as $line) {
+            if (\trim($line) === "") {
+                continue;
+            }
+
+            if (\substr($line, 0, 2) === "//") {
+                continue;
+            }
+
+            $rule = \strtok($line, " \t");
+
+            if ($rule[0] === "!") {
+                $exceptions[] = self::toRegex(\substr($rule, 1), true);
+            } else {
+                $rules[] = self::toRegex($rule, false);
+            }
+        }
+
+        self::$exceptionPatterns = \array_map(function ($list) {
+            return "(^(?:" . \implode("|", $list) . ")$)i";
+        }, \array_chunk($exceptions, 256));
+
+        self::$suffixPatterns = \array_map(function ($list) {
+            return "(^(?:" . \implode("|", $list) . ")$)i";
+        }, \array_chunk($rules, 256));
+    }
+
+    private static function toRegex($rule, $exception) {
+        if (!self::isValidHostName(\strtr($rule, "*", "x"))) {
+            $rule = \idn_to_ascii($rule);
+
+            if (!self::isValidHostName(\strtr($rule, "*", "x"))) {
+                \trigger_error("Invalid public suffix rule: " . $rule, \E_USER_DEPRECATED);
+
+                return "";
+            }
+        }
+
+        $regexParts = [];
+
+        foreach (\explode(".", $rule) as $part) {
+            if ($part === "*") {
+                $regexParts[] = "[^.]+";
+            } else {
+                $regexParts[] = \preg_quote($part);
+            }
+        }
+
+        $regex = \array_reduce($regexParts, function ($carry, $item) use ($exception) {
+            if ($carry === "") {
+                return $item;
+            }
+
+            return $item . "(?:\\." . $carry . ")" . ($exception ? "" : "?");
+        }, "");
+
+        return $regex;
+    }
+
+    private static function isValidHostName($name) {
+        $pattern = <<<'REGEX'
+/^(?<name>[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\.(?&name))*$/i
+REGEX;
+
+        return !isset($name[253]) && \preg_match($pattern, $name);
+    }
+}