If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

CSP parser should accept 127.0.0.1:*

RESOLVED FIXED in mozilla33

Status

()

Core
DOM: Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1031259
(Assignee)

Comment 1

3 years ago
Created attachment 8447272 [details] [diff] [review]
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.
Attachment #8447272 - Flags: review?(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.
Attachment #8447272 - Flags: review?(sstamm) → review+
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
(Assignee)

Comment 3

3 years ago
(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 :-(
(Assignee)

Comment 4

3 years ago
This patch can land together with bug 1031259.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a57cfa94499e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba869669bb9
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/9ba869669bb9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.