Bug 1892748 Part 1 - Reject control characters in cookie attributes. r=dveditz,cookie-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D208155

netwerk/cookie/CookieService.cpp

@@ -1524,10 +1524,9 @@ bool CookieService::CanSetCookie(
     separators    = ";" | "="
     value-sep     = ";"
     cookie-sep    = CR | LF
-    allowed-chars = <any OCTET except NUL or cookie-sep>
+    allowed-chars = <any OCTET except cookie-sep>
     OCTET         = <any 8-bit sequence of data>
     LWS           = SP | HT
-    NUL           = <US-ASCII NUL, null control character (0)>
     CR            = <US-ASCII CR, carriage return (13)>
     LF            = <US-ASCII LF, linefeed (10)>
     SP            = <US-ASCII SP, space (32)>
@@ -1547,6 +1546,8 @@ bool CookieService::CanSetCookie(
                   | "Max-Age" "=" value
                   | "Comment" "=" value
                   | "Version" "=" value
+                  | "Partitioned"
+                  | "SameSite"
                   | "Secure"
                   | "HttpOnly"
 
@@ -1554,7 +1555,6 @@ bool CookieService::CanSetCookie(
 // clang-format on
 
 // helper functions for GetTokenValue
-static inline bool isnull(char c) { return c == 0; }
 static inline bool iswhitespace(char c) { return c == ' ' || c == '\t'; }
 static inline bool isterminator(char c) { return c == '\n' || c == '\r'; }
 static inline bool isvalueseparator(char c) {
@@ -1582,7 +1582,7 @@ bool CookieService::GetTokenValue(nsACString::const_char_iterator& aIter,
     ++aIter;
   }
   start = aIter;
-  while (aIter != aEndIter && !isnull(*aIter) && !istokenseparator(*aIter)) {
+  while (aIter != aEndIter && !istokenseparator(*aIter)) {
     ++aIter;
   }
 
@@ -1605,7 +1605,7 @@ bool CookieService::GetTokenValue(nsACString::const_char_iterator& aIter,
 
     // process <token>
     // just look for ';' to terminate ('=' allowed)
-    while (aIter != aEndIter && !isnull(*aIter) && !isvalueseparator(*aIter)) {
+    while (aIter != aEndIter && !isvalueseparator(*aIter)) {
       ++aIter;
     }
 
@@ -1645,6 +1645,17 @@ static inline void SetSameSiteAttribute(CookieStruct& aCookieData,
   aCookieData.rawSameSite() = aValue;
 }
 
+// Tests for control characters, defined by RFC 5234 to be %x00-1F / %x7F.
+// An exception is made for HTAB as the cookie spec treats that as whitespace.
+static bool ContainsControlChars(const nsACString& aString) {
+  const auto* start = aString.BeginReading();
+  const auto* end = aString.EndReading();
+
+  return std::find_if(start, end, [](unsigned char c) {
+           return (c <= 0x1F && c != 0x09) || c == 0x7F;
+         }) != end;
+}
+
 // Parses attributes from cookie header. expires/max-age attributes aren't
 // folded into the cookie struct here, because we don't know which one to use
 // until we've parsed the header.
@@ -1702,6 +1713,14 @@ bool CookieService::ParseAttributes(nsIConsoleReportCollector* aCRC,
     newCookie = GetTokenValue(cookieStart, cookieEnd, tokenString, tokenValue,
                               equalsFound);
 
+    if (ContainsControlChars(tokenString) || ContainsControlChars(tokenValue)) {
+      CookieLogging::LogMessageToConsole(
+          aCRC, aHostURI, nsIScriptError::errorFlag, CONSOLE_REJECTION_CATEGORY,
+          "CookieRejectedInvalidCharAttributes"_ns,
+          AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(aCookieData.name())});
+      return newCookie;
+    }
+
     // decide which attribute we have, and copy the string
     if (tokenString.LowerCaseEqualsLiteral(kPath)) {
       aCookieData.path() = tokenValue;

netwerk/locales/en-US/necko.properties

@@ -70,6 +70,8 @@ CookiePathOversize=Cookie “%1$S” is invalid because its path size is too big
 CookieRejectedByPermissionManager=Cookie “%1$S” has been rejected by user set permissions.
 # LOCALIZATION NOTE (CookieRejectedInvalidCharName): %1$S is the cookie name.
 CookieRejectedInvalidCharName=Cookie “%1$S” has been rejected for invalid characters in the name.
+# LOCALIZATION NOTE (CookieRejectedInvalidCharAttributes): %1$S is the cookie name.
+CookieRejectedInvalidCharAttributes=Cookie “%1$S” has been rejected for invalid characters in the attributes.
 # LOCALIZATION NOTE (CookieRejectedInvalidDomain): %1$S is the cookie name.
 CookieRejectedInvalidDomain=Cookie “%1$S” has been rejected for invalid domain.
 # LOCALIZATION NOTE (CookieRejectedInvalidPrefix): %1$S is the cookie name.

testing/web-platform/meta/cookies/name/name-ctl.html.ini

@@ -1,7 +1,4 @@
 [name-ctl.html]
-  [Cookie with %x0 in name is rejected (DOM).]
-    expected: FAIL
-
   [Cookie with %x9 in name is accepted (DOM).]
     expected: FAIL
 

testing/web-platform/meta/cookies/value/value-ctl.html.ini

@@ -1,7 +1,4 @@
 [value-ctl.html]
-  [Cookie with %x0 in value is rejected (DOM).]
-    expected: FAIL
-
   [Cookie with %xa in value is rejected (DOM).]
     expected: FAIL
 

testing/web-platform/meta/html/dom/documents/resource-metadata-management/document-cookie.html.ini

@@ -1,5 +0,0 @@
-[document-cookie.html]
-  expected:
-    if (os == "android") and fission: [OK, TIMEOUT]
-  [document.cookie 2]
-    expected: FAIL