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?

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.

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

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.

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

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 on attachment 768641 [details] [diff] [review] patch v3 Brian said to have Camilo and Garrett review this.

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)

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

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

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

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

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