bug 846825 - refactor, make HSTS header parser more spec-conformant r=cviecco r=grobinson
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 25 Jul 2013 16:13:50 -0700
changeset 140171 2e6def7eb4db466cd4dcb42fbbc590fa171d90a7
parent 140170 8b32dad46ea1c5434719f3c31e7cdd115002aebf
child 140172 b51cb3254d1883bdf5797e76794dbdc063cf0cff
push id25016
push userryanvm@gmail.com
push dateSat, 27 Jul 2013 02:25:56 +0000
treeherdermozilla-central@fb48c7d58b8b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscviecco, grobinson
bugs846825
milestone25.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 846825 - refactor, make HSTS header parser more spec-conformant r=cviecco r=grobinson
netwerk/test/TestSTSParser.cpp
security/manager/boot/src/moz.build
security/manager/boot/src/nsSecurityHeaderParser.cpp
security/manager/boot/src/nsSecurityHeaderParser.h
security/manager/boot/src/nsStrictTransportSecurityService.cpp
security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js
--- a/netwerk/test/TestSTSParser.cpp
+++ b/netwerk/test/TestSTSParser.cpp
@@ -111,65 +111,88 @@ main(int32_t argc, char *argv[])
     // *** parsing tests
     printf("*** Attempting to parse valid STS headers ...\n");
 
     // SHOULD SUCCEED:
     rvs.AppendElement(TestSuccess("max-age=100", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("max-age  =100", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess(" max-age=100", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("max-age = 100 ", false, 100, false, stss, pm));
+    rvs.AppendElement(TestSuccess("max-age = \"100\" ", false, 100, false, stss, pm));
+    rvs.AppendElement(TestSuccess("max-age=\"100\"", false, 100, false, stss, pm));
+    rvs.AppendElement(TestSuccess(" max-age =\"100\" ", false, 100, false, stss, pm));
+    rvs.AppendElement(TestSuccess("\tmax-age\t=\t\"100\"\t", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("max-age  =       100             ", false, 100, false, stss, pm));
 
     rvs.AppendElement(TestSuccess("maX-aGe=100", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("MAX-age  =100", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("max-AGE=100", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("Max-Age = 100 ", false, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("MAX-AGE = 100 ", false, 100, false, stss, pm));
 
     rvs.AppendElement(TestSuccess("max-age=100;includeSubdomains", false, 100, true, stss, pm));
-    rvs.AppendElement(TestSuccess("max-age=100; includeSubdomains", false, 100, true, stss, pm));
+    rvs.AppendElement(TestSuccess("max-age=100\t; includeSubdomains", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess(" max-age=100; includeSubdomains", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess("max-age = 100 ; includeSubdomains", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess("max-age  =       100             ; includeSubdomains", false, 100, true, stss, pm));
 
     rvs.AppendElement(TestSuccess("maX-aGe=100; includeSUBDOMAINS", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess("MAX-age  =100; includeSubDomains", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess("max-AGE=100; iNcLuDeSuBdoMaInS", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess("Max-Age = 100; includesubdomains ", false, 100, true, stss, pm));
     rvs.AppendElement(TestSuccess("INCLUDESUBDOMAINS;MaX-AgE = 100 ", false, 100, true, stss, pm));
+    // Turns out, the actual directive is entirely optional (hence the
+    // trailing semicolon)
+    rvs.AppendElement(TestSuccess("max-age=100;includeSubdomains;", true, 100, true, stss, pm));
 
     // these are weird tests, but are testing that some extended syntax is
     // still allowed (but it is ignored)
-    rvs.AppendElement(TestSuccess("max-age=100randomstuffhere", true, 100, false, stss, pm));
-    rvs.AppendElement(TestSuccess("max-age=100 includesubdomains", true, 100, false, stss, pm));
-    rvs.AppendElement(TestSuccess("max-age=100 bar foo", true, 100, false, stss, pm));
     rvs.AppendElement(TestSuccess("max-age=100 ; includesubdomainsSomeStuff", true, 100, false, stss, pm));
+    rvs.AppendElement(TestSuccess("\r\n\t\t    \tcompletelyUnrelated = foobar; max-age= 34520103    \t \t; alsoUnrelated;asIsThis;\tincludeSubdomains\t\t \t", true, 34520103, true, stss, pm));
+    rvs.AppendElement(TestSuccess("max-age=100; unrelated=\"quoted \\\"thingy\\\"\"", true, 100, false, stss, pm));
 
     rv0 = rvs.Contains(false) ? 1 : 0;
     if (rv0 == 0)
       passed("Successfully Parsed STS headers with mixed case and LWS");
 
     rvs.Clear();
 
     // SHOULD FAIL:
     printf("*** Attempting to parse invalid STS headers (should not parse)...\n");
     // invalid max-ages
+    rvs.AppendElement(TestFailure("max-age", stss, pm));
     rvs.AppendElement(TestFailure("max-age ", stss, pm));
     rvs.AppendElement(TestFailure("max-age=p", stss, pm));
     rvs.AppendElement(TestFailure("max-age=*1p2", stss, pm));
     rvs.AppendElement(TestFailure("max-age=.20032", stss, pm));
     rvs.AppendElement(TestFailure("max-age=!20032", stss, pm));
     rvs.AppendElement(TestFailure("max-age==20032", stss, pm));
 
     // invalid headers
     rvs.AppendElement(TestFailure("foobar", stss, pm));
     rvs.AppendElement(TestFailure("maxage=100", stss, pm));
     rvs.AppendElement(TestFailure("maxa-ge=100", stss, pm));
     rvs.AppendElement(TestFailure("max-ag=100", stss, pm));
     rvs.AppendElement(TestFailure("includesubdomains", stss, pm));
     rvs.AppendElement(TestFailure(";", stss, pm));
+    rvs.AppendElement(TestFailure("max-age=\"100", stss, pm));
+    // The max-age directive here doesn't conform to the spec, so it MUST
+    // be ignored. Consequently, the REQUIRED max-age directive is not
+    // present in this header, and so it is invalid.
+    rvs.AppendElement(TestFailure("max-age=100, max-age=200; includeSubdomains", stss, pm));
+    rvs.AppendElement(TestFailure("max-age=100 includesubdomains", stss, pm));
+    rvs.AppendElement(TestFailure("max-age=100 bar foo", stss, pm));
+    rvs.AppendElement(TestFailure("max-age=100randomstuffhere", stss, pm));
+    // All directives MUST appear only once in an STS header field.
+    rvs.AppendElement(TestFailure("max-age=100; max-age=200", stss, pm));
+    rvs.AppendElement(TestFailure("includeSubdomains; max-age=200; includeSubdomains", stss, pm));
+    rvs.AppendElement(TestFailure("max-age=200; includeSubdomains; includeSubdomains", stss, pm));
+    // The includeSubdomains directive is valueless.
+    rvs.AppendElement(TestFailure("max-age=100; includeSubdomains=unexpected", stss, pm));
+    // LWS must have at least one space or horizontal tab
+    rvs.AppendElement(TestFailure("\r\nmax-age=200", stss, pm));
 
     rv1 = rvs.Contains(false) ? 1 : 0;
     if (rv1 == 0)
       passed("Avoided parsing invalid STS headers");
 
     return (rv0 + rv1);
 }
--- a/security/manager/boot/src/moz.build
+++ b/security/manager/boot/src/moz.build
@@ -5,14 +5,15 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 MODULE = 'pipboot'
 
 CPP_SOURCES += [
     'nsBOOTModule.cpp',
     'nsEntropyCollector.cpp',
     'nsSecureBrowserUIImpl.cpp',
+    'nsSecurityHeaderParser.cpp',
     'nsSecurityWarningDialogs.cpp',
     'nsStrictTransportSecurityService.cpp',
 ]
 
 LIBRARY_NAME = 'pipboot'
 
new file mode 100644
--- /dev/null
+++ b/security/manager/boot/src/nsSecurityHeaderParser.cpp
@@ -0,0 +1,254 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "nsSecurityHeaderParser.h"
+#include "prlog.h"
+
+// The character classes in this file are informed by [RFC2616], Section 2.2.
+// signed char is a signed data type one byte (8 bits) wide, so its value can
+// never be greater than 127. The following implicitly makes use of this.
+
+// A token is one or more CHAR except CTLs or separators.
+// A CHAR is any US-ASCII character (octets 0 - 127).
+// A CTL is any US-ASCII control character (octets 0 - 31) and DEL (127).
+// A separator is one of ()<>@,;:\"/[]?={} as well as space and
+// horizontal-tab (32 and 9, respectively).
+// So, this returns true if chr is any octet 33-126 except ()<>@,;:\"/[]?={}
+bool
+IsTokenSymbol(signed char chr) {
+  if (chr < 33 || chr == 127 ||
+      chr == '(' || chr == ')' || chr == '<' || chr == '>' ||
+      chr == '@' || chr == ',' || chr == ';' || chr == ':' ||
+      chr == '"' || chr == '/' || chr == '[' || chr == ']' ||
+      chr == '?' || chr == '=' || chr == '{' || chr == '}' || chr == '\\') {
+    return false;
+  }
+  return true;
+}
+
+// A quoted-string consists of a quote (") followed by any amount of
+// qdtext or quoted-pair, followed by a quote.
+// qdtext is any TEXT except a quote.
+// TEXT is any 8-bit octet except CTLs, but including LWS.
+// quoted-pair is a backslash (\) followed by a CHAR.
+// So, it turns out, \ can't really be a qdtext symbol for our purposes.
+// This returns true if chr is any octet 9,10,13,32-126 except <"> or "\"
+bool
+IsQuotedTextSymbol(signed char chr) {
+  return ((chr >= 32 && chr != '"' && chr != '\\' && chr != 127) ||
+          chr == 0x9 || chr == 0xa || chr == 0xd);
+}
+
+// The octet following the "\" in a quoted pair can be anything 0-127.
+bool
+IsQuotedPairSymbol(signed char chr) {
+  return (chr >= 0);
+}
+
+#if defined(PR_LOGGING)
+static PRLogModuleInfo *
+GetSHParserLog()
+{
+  static PRLogModuleInfo *sSHParserLog;
+  if (!sSHParserLog) {
+    sSHParserLog = PR_NewLogModule("nsSecurityHeaderParser");
+  }
+  return sSHParserLog;
+}
+#endif
+
+#define SHPARSERLOG(args) PR_LOG(GetSHParserLog(), PR_LOG_DEBUG, args)
+
+nsSecurityHeaderParser::nsSecurityHeaderParser(const char *aHeader)
+  : mCursor(aHeader)
+  , mError(false)
+{
+}
+
+nsSecurityHeaderParser::~nsSecurityHeaderParser() {
+  nsSecurityHeaderDirective *directive;
+  while ((directive = mDirectives.popFirst())) {
+    delete directive;
+  }
+}
+
+mozilla::LinkedList<nsSecurityHeaderDirective> *
+nsSecurityHeaderParser::GetDirectives() {
+  return &mDirectives;
+}
+
+nsresult
+nsSecurityHeaderParser::Parse() {
+  MOZ_ASSERT(mDirectives.isEmpty());
+  SHPARSERLOG(("trying to parse '%s'", mCursor));
+
+  Header();
+
+  // if we didn't consume the entire input, we were unable to parse it => error
+  if (mError || *mCursor) {
+    return NS_ERROR_FAILURE;
+  } else {
+    return NS_OK;
+  }
+}
+
+bool
+nsSecurityHeaderParser::Accept(char aChr)
+{
+  if (*mCursor == aChr) {
+    Advance();
+    return true;
+  }
+
+  return false;
+}
+
+bool
+nsSecurityHeaderParser::Accept(bool (*aClassifier) (signed char))
+{
+  if (aClassifier(*mCursor)) {
+    Advance();
+    return true;
+  }
+
+  return false;
+}
+
+void
+nsSecurityHeaderParser::Expect(char aChr)
+{
+  if (*mCursor != aChr) {
+    mError = true;
+  } else {
+    Advance();
+  }
+}
+
+void
+nsSecurityHeaderParser::Advance()
+{
+  // Technically, 0 is valid in quoted-pair, but we were handed a
+  // null-terminated const char *, so this doesn't handle that.
+  if (*mCursor) {
+    mOutput.Append(*mCursor);
+    mCursor++;
+  } else {
+    mError = true;
+  }
+}
+
+void
+nsSecurityHeaderParser::Header()
+{
+  Directive();
+  while (Accept(';')) {
+    Directive();
+  }
+}
+
+void
+nsSecurityHeaderParser::Directive()
+{
+  mDirective = new nsSecurityHeaderDirective();
+  LWSMultiple();
+  DirectiveName();
+  LWSMultiple();
+  if (Accept('=')) {
+    LWSMultiple();
+    DirectiveValue();
+    LWSMultiple();
+  }
+  mDirectives.insertBack(mDirective);
+  SHPARSERLOG(("read directive name '%s', value '%s'",
+               mDirective->mName.Data(), mDirective->mValue.Data()));
+}
+
+void
+nsSecurityHeaderParser::DirectiveName()
+{
+  mOutput.Truncate(0);
+  Token();
+  mDirective->mName.Assign(mOutput);
+}
+
+void
+nsSecurityHeaderParser::DirectiveValue()
+{
+  mOutput.Truncate(0);
+  if (Accept(IsTokenSymbol)) {
+    Token();
+    mDirective->mValue.Assign(mOutput);
+  } else if (Accept('"')) {
+    // Accept advances the cursor if successful, which appends a character to
+    // mOutput. The " is not part of what we want to capture, so truncate
+    // mOutput again.
+    mOutput.Truncate(0);
+    QuotedString();
+    mDirective->mValue.Assign(mOutput);
+    Expect('"');
+  }
+}
+
+void
+nsSecurityHeaderParser::Token()
+{
+  while (Accept(IsTokenSymbol));
+}
+
+void
+nsSecurityHeaderParser::QuotedString()
+{
+  while (true) {
+    if (Accept(IsQuotedTextSymbol)) {
+      QuotedText();
+    } else if (Accept('\\')) {
+      QuotedPair();
+    } else {
+      break;
+    }
+  }
+}
+
+void
+nsSecurityHeaderParser::QuotedText()
+{
+  while (Accept(IsQuotedTextSymbol));
+}
+
+void
+nsSecurityHeaderParser::QuotedPair()
+{
+  Accept(IsQuotedPairSymbol);
+}
+
+void
+nsSecurityHeaderParser::LWSMultiple()
+{
+  while (true) {
+    if (Accept('\r')) {
+      LWSCRLF();
+    } else if (Accept(' ') || Accept('\t')) {
+      LWS();
+    } else {
+      break;
+    }
+  }
+}
+
+void
+nsSecurityHeaderParser::LWSCRLF() {
+  Expect('\n');
+  if (!(Accept(' ') || Accept('\t'))) {
+    mError = true;
+  }
+  LWS();
+}
+
+void
+nsSecurityHeaderParser::LWS()
+{
+  // Note that becaue of how we're called, we don't have to check for
+  // the mandatory presense of at least one of SP or HT.
+  while (Accept(' ') || Accept('\t'));
+}
new file mode 100644
--- /dev/null
+++ b/security/manager/boot/src/nsSecurityHeaderParser.h
@@ -0,0 +1,74 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef nsSecurityHeaderParser_h__
+#define nsSecurityHeaderParser_h__
+
+#include "nsString.h"
+#include "mozilla/LinkedList.h"
+#include "nsCOMPtr.h"
+
+// Utility class for handing back parsed directives and (optional) values
+class nsSecurityHeaderDirective : public mozilla::LinkedListElement<nsSecurityHeaderDirective> {
+public:
+  nsAutoCString mName;
+  nsAutoCString mValue;
+};
+
+// This class parses security-related HTTP headers like
+// Strict-Transport-Security. The Augmented Backus-Naur Form syntax for this
+// header is reproduced below, for reference:
+//
+//   Strict-Transport-Security = "Strict-Transport-Security" ":"
+//                               [ directive ]  *( ";" [ directive ] )
+//
+//   directive                 = directive-name [ "=" directive-value ]
+//   directive-name            = token
+//   directive-value           = token | quoted-string
+//
+//   where:
+//
+//   token          = <token, defined in [RFC2616], Section 2.2>
+//   quoted-string  = <quoted-string, defined in [RFC2616], Section 2.2>/
+//
+// For further reference, see [RFC6797], Section 6.1
+
+class nsSecurityHeaderParser {
+public:
+  nsSecurityHeaderParser(const char *aHeader);
+  ~nsSecurityHeaderParser();
+
+  // Only call Parse once.
+  nsresult Parse();
+  // The caller does not take ownership of the memory returned here.
+  mozilla::LinkedList<nsSecurityHeaderDirective> *GetDirectives();
+
+private:
+  bool Accept(char aChr);
+  bool Accept(bool (*aClassifier) (signed char));
+  void Expect(char aChr);
+  void Advance();
+  void Header();         // header = [ directive ] *( ";" [ directive ] )
+  void Directive();      // directive = directive-name [ "=" directive-value ]
+  void DirectiveName();  // directive-name = token
+  void DirectiveValue(); // directive-value = token | quoted-string
+  void Token();          // token = 1*<any CHAR except CTLs or separators>
+  void QuotedString();   // quoted-string = (<"> *( qdtext | quoted-pair ) <">)
+  void QuotedText();     // qdtext = <any TEXT except <"> and "\">
+  void QuotedPair();     // quoted-pair = "\" CHAR
+
+                         // LWS = [CRLF] 1*( SP | HT )
+  void LWSMultiple();    // Handles *( LWS )
+  void LWSCRLF();        // Handles the [CRLF] part of LWS
+  void LWS();            // Handles the 1*( SP | HT ) part of LWS
+
+  mozilla::LinkedList<nsSecurityHeaderDirective> mDirectives;
+  const char *mCursor;
+  nsSecurityHeaderDirective *mDirective;
+
+  nsAutoCString mOutput;
+  bool mError;
+};
+
+#endif /* nsSecurityHeaderParser_h__ */
--- a/security/manager/boot/src/nsStrictTransportSecurityService.cpp
+++ b/security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ -12,16 +12,18 @@
 #include "nsStrictTransportSecurityService.h"
 #include "nsIURI.h"
 #include "nsNetUtil.h"
 #include "nsThreadUtils.h"
 #include "nsStringGlue.h"
 #include "nsIScriptSecurityManager.h"
 #include "nsISocketProvider.h"
 #include "mozilla/Preferences.h"
+#include "mozilla/LinkedList.h"
+#include "nsSecurityHeaderParser.h"
 
 // A note about the preload list:
 // When a site specifically disables sts by sending a header with
 // 'max-age: 0', we keep a "knockout" value that means "we have no information
 // regarding the sts state of this host" (any ancestor of "this host" can still
 // influence its sts status via include subdomains, however).
 // This prevents the preload list from overriding the site's current
 // desired sts status. Knockout values are indicated by permission values of
@@ -40,22 +42,16 @@ GetSTSLog()
   if (!gSTSLog)
     gSTSLog = PR_NewLogModule("nsSTSService");
   return gSTSLog;
 }
 #endif
 
 #define STSLOG(args) PR_LOG(GetSTSLog(), 4, args)
 
-#define STS_PARSER_FAIL_IF(test,args) \
-  if (test) { \
-    STSLOG(args); \
-    return NS_ERROR_FAILURE; \
-  }
-
 ////////////////////////////////////////////////////////////////////////////////
 
 nsSTSHostEntry::nsSTSHostEntry(const char* aHost)
   : mHost(aHost)
   , mExpireTime(0)
   , mStsPermission(STS_UNSET)
   , mExpired(false)
   , mIncludeSubdomains(false)
@@ -247,116 +243,121 @@ nsStrictTransportSecurityService::Proces
 
 nsresult
 nsStrictTransportSecurityService::ProcessStsHeaderMutating(nsIURI* aSourceURI,
                                                            char* aHeader,
                                                            uint32_t aFlags,
                                                            uint64_t *aMaxAge,
                                                            bool *aIncludeSubdomains)
 {
-  STSLOG(("STS: ProcessStrictTransportHeader(%s)\n", aHeader));
+  STSLOG(("STS: processing header '%s'", aHeader));
 
   // "Strict-Transport-Security" ":" OWS
   //      STS-d  *( OWS ";" OWS STS-d  OWS)
   //
   //  ; STS directive
   //  STS-d      = maxAge / includeSubDomains
   //
   //  maxAge     = "max-age" "=" delta-seconds v-ext
   //
   //  includeSubDomains = [ "includeSubDomains" ]
-
-  const char* directive;
+  //
+  //  The order of the directives is not significant.
+  //  All directives must appear only once.
+  //  Directive names are case-insensitive.
+  //  The entire header is invalid if a directive not conforming to the
+  //  syntax is encountered.
+  //  Unrecognized directives (that are otherwise syntactically valid) are
+  //  ignored, and the rest of the header is parsed as normal.
 
   bool foundMaxAge = false;
-  bool foundUnrecognizedTokens = false;
-  bool includeSubdomains = false;
+  bool foundIncludeSubdomains = false;
+  bool foundUnrecognizedDirective = false;
   int64_t maxAge = 0;
 
   NS_NAMED_LITERAL_CSTRING(max_age_var, "max-age");
   NS_NAMED_LITERAL_CSTRING(include_subd_var, "includesubdomains");
 
-  while ((directive = NS_strtok(";", &aHeader))) {
-    //skip leading whitespace
-    directive = NS_strspnp(" \t", directive);
-    STS_PARSER_FAIL_IF(!(*directive), ("error removing initial whitespace\n."));
+
+  nsSecurityHeaderParser parser(aHeader);
+  nsresult rv = parser.Parse();
+  if (NS_FAILED(rv)) {
+    STSLOG(("STS: could not parse header"));
+    return rv;
+  }
+  mozilla::LinkedList<nsSecurityHeaderDirective> *directives = parser.GetDirectives();
 
-    if (!PL_strncasecmp(directive, max_age_var.get(), max_age_var.Length())) {
-      // skip directive name
-      directive += max_age_var.Length();
-      // skip leading whitespace
-      directive = NS_strspnp(" \t", directive);
-      STS_PARSER_FAIL_IF(*directive != '=',
-                  ("No equal sign found in max-age directive\n"));
+  for (nsSecurityHeaderDirective *directive = directives->getFirst();
+       directive != nullptr; directive = directive->getNext()) {
+    if (directive->mName.Length() == max_age_var.Length() &&
+        directive->mName.EqualsIgnoreCase(max_age_var.get(),
+                                          max_age_var.Length())) {
+      if (foundMaxAge) {
+        STSLOG(("STS: found two max-age directives"));
+        return NS_ERROR_FAILURE;
+      }
 
-      // skip over the equal sign
-      STS_PARSER_FAIL_IF(*(++directive) == '\0',
-                  ("No delta-seconds present\n"));
-
-      // obtain the delta-seconds value
-      STS_PARSER_FAIL_IF(PR_sscanf(directive, "%lld", &maxAge) != 1,
-                  ("Could not convert delta-seconds\n"));
-      STSLOG(("STS: ProcessStrictTransportHeader() STS found maxage %lld\n", maxAge));
+      STSLOG(("STS: found max-age directive"));
       foundMaxAge = true;
 
-      // skip max-age value and trailing whitespace
-      directive = NS_strspnp("0123456789 \t", directive);
+      size_t len = directive->mValue.Length();
+      for (size_t i = 0; i < len; i++) {
+        char chr = directive->mValue.CharAt(i);
+        if (chr < '0' || chr > '9') {
+          STSLOG(("STS: invalid value for max-age directive"));
+          return NS_ERROR_FAILURE;
+        }
+      }
 
-      // log unknown tokens, but don't fail (for forwards compatibility)
-      if (*directive != '\0') {
-        foundUnrecognizedTokens = true;
-        STSLOG(("Extra stuff in max-age after delta-seconds: %s \n", directive));
+      if (PR_sscanf(directive->mValue.get(), "%lld", &maxAge) != 1) {
+        STSLOG(("STS: could not parse delta-seconds"));
+        return NS_ERROR_FAILURE;
       }
-    }
-    else if (!PL_strncasecmp(directive, include_subd_var.get(), include_subd_var.Length())) {
-      directive += include_subd_var.Length();
 
-      // only record "includesubdomains" if it is a token by itself... for
-      // example, don't set includeSubdomains = true if the directive is
-      // "includesubdomainsFooBar".
-      if (*directive == '\0' || *directive =='\t' || *directive == ' ') {
-        includeSubdomains = true;
-        STSLOG(("STS: ProcessStrictTransportHeader: obtained subdomains status\n"));
-
-        // skip trailing whitespace
-        directive = NS_strspnp(" \t", directive);
+      STSLOG(("STS: parsed delta-seconds: %lld", maxAge));
+    } else if (directive->mName.Length() == include_subd_var.Length() &&
+               directive->mName.EqualsIgnoreCase(include_subd_var.get(),
+                                                 include_subd_var.Length())) {
+      if (foundIncludeSubdomains) {
+        STSLOG(("STS: found two includeSubdomains directives"));
+        return NS_ERROR_FAILURE;
+      }
 
-        if (*directive != '\0') {
-          foundUnrecognizedTokens = true;
-          STSLOG(("Extra stuff after includesubdomains: %s\n", directive));
-        }
-      } else {
-        foundUnrecognizedTokens = true;
-        STSLOG(("Unrecognized directive in header: %s\n", directive));
+      STSLOG(("STS: found includeSubdomains directive"));
+      foundIncludeSubdomains = true;
+
+      if (directive->mValue.Length() != 0) {
+        STSLOG(("STS: includeSubdomains directive unexpectedly had value '%s'", directive->mValue.get()));
+        return NS_ERROR_FAILURE;
       }
-    }
-    else {
-      // log unknown directives, but don't fail (for backwards compatibility)
-      foundUnrecognizedTokens = true;
-      STSLOG(("Unrecognized directive in header: %s\n", directive));
+    } else {
+      STSLOG(("STS: ignoring unrecognized directive '%s'", directive->mName.get()));
+      foundUnrecognizedDirective = true;
     }
   }
 
   // after processing all the directives, make sure we came across max-age
   // somewhere.
-  STS_PARSER_FAIL_IF(!foundMaxAge,
-              ("Parse ERROR: couldn't locate max-age token\n"));
+  if (!foundMaxAge) {
+    STSLOG(("STS: did not encounter required max-age directive"));
+    return NS_ERROR_FAILURE;
+  }
 
   // record the successfully parsed header data.
-  SetStsState(aSourceURI, maxAge, includeSubdomains, aFlags);
+  SetStsState(aSourceURI, maxAge, foundIncludeSubdomains, aFlags);
 
   if (aMaxAge != nullptr) {
     *aMaxAge = (uint64_t)maxAge;
   }
 
   if (aIncludeSubdomains != nullptr) {
-    *aIncludeSubdomains = includeSubdomains;
+    *aIncludeSubdomains = foundIncludeSubdomains;
   }
 
-  return foundUnrecognizedTokens ?
+  return foundUnrecognizedDirective ?
          NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA :
          NS_OK;
 }
 
 NS_IMETHODIMP
 nsStrictTransportSecurityService::IsStsHost(const char* aHost, uint32_t aFlags, bool* aResult)
 {
   // Should be called on the main thread (or via proxy) since the permission
--- a/security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js
+++ b/security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js
@@ -158,23 +158,22 @@ function test_private_browsing1() {
   // Test that an expired private browsing entry results in correctly
   // identifying a host that is on the preload list as no longer sts.
   // (This happens when we're in private browsing mode, we get a header from
   // a site on the preload list, and that header later expires. We need to
   // then treat that host as no longer an sts host.)
   // (sanity check first - this should be in the preload list)
   do_check_true(gSTSService.isStsHost("login.persona.org", IS_PRIVATE));
   var uri = Services.io.newURI("http://login.persona.org", null, null);
-  // according to the rfc, max-age can't be negative, but this is a great
-  // way to test an expired entry
-  gSTSService.processStsHeader(uri, "max-age=-1000", IS_PRIVATE);
-  do_check_false(gSTSService.isStsHost("login.persona.org", IS_PRIVATE));
-
-  // Simulate leaving private browsing mode
-  Services.obs.notifyObservers(null, "last-pb-context-exited", null);
+  gSTSService.processStsHeader(uri, "max-age=1", IS_PRIVATE);
+  do_timeout(1250, function() {
+    do_check_false(gSTSService.isStsHost("login.persona.org", IS_PRIVATE));
+    // Simulate leaving private browsing mode
+    Services.obs.notifyObservers(null, "last-pb-context-exited", null);
+  });
 }
 
 function test_private_browsing2() {
   // if this test gets this far, it means there's a private browsing service
   do_check_true(gSTSService.isStsHost("bugzilla.mozilla.org", 0));
   // the bugzilla.mozilla.org entry has includeSubdomains set
   do_check_true(gSTSService.isStsHost("subdomain.bugzilla.mozilla.org", 0));