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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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)

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I agree. i filed bug 846918 about the lack of reporting.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

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.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v2

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).

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v3

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.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 768641 [details] [diff] [review]
patch v3

Brian said to have Camilo and Garrett review this.

Comment 9

[Camilo Viecco (:cviecco)]

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)

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v4

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.

Comment 11

[Garrett Robinson [:grobinson]]

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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the reviews, all.
https://tbpl.mozilla.org/?tree=Try&rev=54717ee17c95

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

That didn't work, but it looks like this did:
https://tbpl.mozilla.org/?tree=Try&rev=bf50413f9acb

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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!

Comment 15

[Jason Duell]

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.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v4.1

Moved parsing files to security/manager/boot/src, carrying over r+.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6def7eb4db

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e6def7eb4db