1031372 - CSP parser should accept 127.0.0.1:*

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Related to bug Bug 1031259, the parser rejects parsing 127.0.0.1:* - which ias a valid host, according to the spec: http://www.w3.org/TR/CSP11/#parsing

Comment 1

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Attachment: bug_1031372.patch

The ABNF for hosts is defined as followed:
> host              = "*" / [ "*." ] 1*host-char *( "." 1*host-char )
> host-char         = ALPHA / DIGIT / "-"

Currently, the parser rejected hosts that didn't start with an actual character (a-z), but in fact, the parser should accept all host-chars (ALPHA/DIGIT/"-").

The attached patch fixes the problem, which caused two testcases from the compiled code tests to be moved from bad policies, to good policies. Further, I added two testcases that verify that the parser accepts 127.0.0.1.

Also, in nsCSPParser::sourceList I added resetCurValue(), which does not affect the actual outcome of parsing, but it makes PR logging more readable, because the logging was still holding the previously parsed value, which was confusing.

Please note, that this patch does *not* fix 1031259,it's only one of the sub-problems for bug 1031259.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8447272 [details] [diff] [review]
bug_1031372.patch

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

Looks right. Did you do a pass to verify the rest of the ABNF against the implementation? r=me.

::: content/base/test/TestCSPParser.cpp
@@ +401,5 @@
> +      "default-src http://127.0.0.1:*" },
> +    { "default-src -; ",
> +      "default-src http://-" },
> +    { "script-src 1",
> +      "script-src http://1" }

I don't like http://1, but the spec does appear to allow it as valid syntax so we should probably accept it as a valid source.

Comment 3

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> Comment on attachment 8447272 [details] [diff] [review]
> bug_1031372.patch
> 
> Review of attachment 8447272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks right. Did you do a pass to verify the rest of the ABNF against the
> implementation? r=me.
> 
> ::: content/base/test/TestCSPParser.cpp
> @@ +401,5 @@
> > +      "default-src http://127.0.0.1:*" },
> > +    { "default-src -; ",
> > +      "default-src http://-" },
> > +    { "script-src 1",
> > +      "script-src http://1" }
> 
> I don't like http://1, but the spec does appear to allow it as valid syntax
> so we should probably accept it as a valid source.

I don't like that either, but that's what the spec defines :-(

Comment 4

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

This patch can land together with bug 1031259.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a57cfa94499e

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba869669bb9

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ba869669bb9