the HSTS header parser doesn't conform to spec (it is too permissive)

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

In particular, the following from http://tools.ietf.org/html/rfc6797#section-6.1.1 are violated:

   2.  All directives MUST appear only once in an STS header field.
       Directives are either optional or required, as stipulated in
       their definitions.

   4.  UAs MUST ignore any STS header field containing directives, or
       other header field value data, that does not conform to the
       syntax defined in this specification.

For instance, my understanding of these statements is that the following should be rejected:

"max-age=100, max-age=200; includeSubdomains" (the first directive doesn't follow the syntax and thus should be ignored; consequently, the required max-age directive is not present)
"max-age=100; max-age=200" (the max-age directive is present twice)
"includeSubdomains; max-age=200; includeSubdomains" (the includeSubdomains directive is present twice)
Actually, re-reading requirement 4 and considering requirement 5, the situation is a little different from how I thought it was.

   5.  If an STS header field contains directive(s) not recognized by
       the UA, the UA MUST ignore the unrecognized directives, and if
       the STS header field otherwise satisfies the above requirements
       (1 through 4), the UA MUST process the recognized directives.

4 says the UA must ignore the entire header if it encounters a directive that doesn't conform to the syntax. So my first example (in comment 0) would just be ignored outright. I guess 5 covers something like this: "max-age=100; ignorethis=4", which would be accepted as valid.
(In reply to David Keeler (:keeler) from comment #0)
> "max-age=100, max-age=200; includeSubdomains" (the first directive doesn't
> follow the syntax and thus should be ignored; consequently, the required
> max-age directive is not present)

In HTTP, a header fields with the same name can be folded together. In the case of the above, it is likely that the server actually sent:

Strict-Transport-Security: max-age=100
Strict-Transport-Security: max-age=200; includeSubdomains

I agree that this is an invalid policy. But, it is likely a very easy-toi-make configuration error on the server. Do you know if we warn about invalid HSTS policies in the web console?
Flags: needinfo?(dkeeler)
I just gave it a try in 20, and it doesn't look like we do warn about either invalid HSTS headers or even the "success with loss of data" case. We probably should. That said, if someone misconfigures their STS header as in the above, it should fail as early as possible (i.e. by failing to set any state in the browser), not when they were expecting max-age to be X and we parse it as Y.
Flags: needinfo?(dkeeler)
I agree. i filed bug 846918 about the lack of reporting.
Posted patch patch (obsolete) — Splinter Review
This probably seems like overkill, but I think having a formal, provably-correct (in the sense that it is possible to prove this correct, not that I claim it is right now) parser makes our implementation more robust.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #722895 - Flags: feedback?(bsmith)
Depends on: 851975
No longer depends on: 851975
Posted patch patch v2 (obsolete) — Splinter Review
This refactors the header parsing into a separate utility class that other parts of the code can use (for example, when we have OCSP-must-staple, which will want to parse very similar headers).
Attachment #722895 - Attachment is obsolete: true
Attachment #722895 - Flags: feedback?(bsmith)
Attachment #767498 - Flags: feedback?(bsmith)
Posted patch patch v3 (obsolete) — Splinter Review
This re-implements the parser class as a recursive descent parser like we talked about. Let me know if there's anything unclear in the implementation. Also, I initially figured the new parser should go in netwerk/base/src, but do you think security/manager/boot/src is a better place? Or is there another? Thanks.
Attachment #767498 - Attachment is obsolete: true
Attachment #767498 - Flags: feedback?(bsmith)
Attachment #768641 - Flags: review?(bsmith)
Comment on attachment 768641 [details] [diff] [review]
patch v3

Brian said to have Camilo and Garrett review this.
Attachment #768641 - Flags: review?(grobinson)
Attachment #768641 - Flags: review?(cviecco)
Attachment #768641 - Flags: review?(brian)
Comment on attachment 768641 [details] [diff] [review]
patch v3

Review of attachment 768641 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good.


if it was not for the missing the 127 char it would probably be an r+

::: netwerk/base/public/nsSecurityHeaderParser.h
@@ +65,5 @@
> +  void LWSCRLF();        // Handles the [CRLF] part of LWS
> +  void LWS();            // Handles the 1*( SP | HT ) part of LWS
> +
> +  LinkedList<nsSecurityHeaderDirective> mDirectives;
> +  const char *mPtr;

is mPtr the bset name?

::: netwerk/base/src/nsSecurityHeaderParser.cpp
@@ +22,5 @@
> +  for (size_t i = 0; i < separatorsLength; i++) {
> +    if (chr == separators[i]) {
> +      return false;
> +    }
> +  }

This function is incorrect,it should return false on 127. (nice how you used the signedness to not check for 128-255)(maybe add a comment for this?)
From the rfc:
 token          = 1*<any CHAR except CTLs or separators>

This function is also optimized for the bad case, which is strange, but is really readable. (and thou shall not optmimize prematurely)

@@ +33,5 @@
> +// 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-127 except <"> or "\"
> +bool

127 is a CTL, should not return true.

@@ +66,5 @@
> +nsresult
> +nsSecurityHeaderParser::Parse() {
> +  MOZ_ASSERT(mDirectives.isEmpty());
> +  fprintf(stderr, "trying to parse '%s'\n", mPtr);
> +  mError = false;

question: should the error be initialized in the constructor?

@@ +124,5 @@
> +  }
> +}
> +
> +void
> +nsSecurityHeaderParser::Header()

do you really need this function? it seems like it is called only once, why not fold it into parse?

@@ +145,5 @@
> +    DirectiveValue();
> +    LWSMultiple();
> +  }
> +  mDirectives.insertBack(mDirective);
> +  fprintf(stderr, "read directive name '%s', value '%s'\n", mDirective->mName.Data(), mDirective->mValue.Data());

convert fprintfs into prlogs.

@@ +152,5 @@
> +void
> +nsSecurityHeaderParser::DirectiveName()
> +{
> +  mOutput.Truncate(0);
> +  mOutput.SetCapacity(64);

m output is an AutoCstring, do you need to call setcapacity? (ditto for the other places where you call it)
Attachment #768641 - Flags: review?(cviecco) → review-
Posted patch patch v4 (obsolete) — Splinter Review
Thanks for the review - I've cleaned up the patch a bit and addressed your comments.
About having a separate function for Header - I think it's important to reflect that this is a recursive descent parser in the structure of the implementation, hence one function for each part of the grammar.
Attachment #768641 - Attachment is obsolete: true
Attachment #768641 - Flags: review?(grobinson)
Attachment #774147 - Flags: review?(cviecco)
Attachment #774147 - Flags: review?(cviecco) → review+
Comment on attachment 774147 [details] [diff] [review]
patch v4

Review of attachment 774147 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Hopefully we can reuse this code in the future for parsing other, similar, HTTP headers. r+ modulo changing the interpretation of includeSubdomains with a value, described below.

::: netwerk/test/TestSTSParser.cpp
@@ +150,5 @@
> +    rvs.AppendElement(TestSuccess("max-age=100; unrelated=\"quoted \\\"thingy\\\"\"", true, 100, false, stss, pm));
> +    // XXX it's not clear to me if the includeSubdomains directive having
> +    // a value is an error or not (the spec says it's valueless, but is mum
> +    // on what happens if it has a value...)
> +    rvs.AppendElement(TestSuccess("max-age = 100; includeSubdomains=unexpected", false, 100, true, stss, pm));

I think it a "valueless" directive has a value, that should be treated as a failure to conform to syntax and so should be ignored according to Requirement #4.
Attachment #774147 - Flags: review?(grobinson) → review+
Comment on attachment 774147 [details] [diff] [review]
patch v4

jduell - we spoke on IRC about reviewing this, but I forgot to mark r?.
This patch refactors parsing headers like Strict-Transport-Security into its own file/class. I put it in netwerk/ because it has to do with parsing http headers, and I basically just wanted a necko peer to sign off on that. Thanks!
Attachment #774147 - Flags: review?(jduell.mcbugs)

Comment 15

6 years ago
Comment on attachment 774147 [details] [diff] [review]
patch v4

Review of attachment 774147 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC, since reviewers for this are likely to be security folks, and rest of STS code is outside /netwerk, these new files shouldn't be in necko.
Attachment #774147 - Flags: review?(jduell.mcbugs)
Posted patch patch v4.1Splinter Review
Moved parsing files to security/manager/boot/src, carrying over r+.
Attachment #774147 - Attachment is obsolete: true
Attachment #781286 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2e6def7eb4db
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.