Last Comment Bug 784315 - CSP parser not correctly parsing single token hosts
: CSP parser not correctly parsing single token hosts
: regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Marshall Moutenot
: Andrew Overholt [:overholt]
: 785292 791444 (view as bug list)
Depends on: 737064
Blocks: CSP
  Show dependency treegraph
Reported: 2012-08-21 06:03 PDT by Bob Clary [:bc:]
Modified: 2013-04-03 00:32 PDT (History)
10 users (show)
mozbugs: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

First Fix (20.39 KB, patch)
2012-08-21 10:34 PDT, Marshall Moutenot
no flags Details | Diff | Splinter Review
Fix without all CSP spec compliance changes (4.83 KB, patch)
2012-08-21 16:45 PDT, Marshall Moutenot
mozbugs: feedback-
Details | Diff | Splinter Review
Patch, correctly handles scheme conflicts (24.65 KB, patch)
2012-08-23 16:25 PDT, Marshall Moutenot
mozbugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2012-08-21 06:03:39 PDT
I am running an instance of phpmyadmin where it is set up as a virtual server with a simple host name. phpmyadmin uses frames to compose its pages. Beginning with the 2012-08-19-03-06-01 Nightly nightly build, each frame would load with "Blocked by Content Security Policy".

Sid checked it out and said:

The CSP is:

allow 'self'; options inline-script eval-script; frame-ancestors 'self';
img-src 'self' data:; script-src 'self'

The error message says it's blocking a URL that specifies port 80, and
although that's supposed to be legit (http default port is 80), our
parser is probably to blame.

And to be absolutely clear, this only occurs for single-token hosts
names (no dots).  I'm not sure CSP is intended to support TLD like this,
but it probably wouldn't hurt.

The site is not publicly available but I can provide instructions if you have mpt access.
Comment 1 Marshall Moutenot 2012-08-21 10:34:16 PDT
Created attachment 653835 [details] [diff] [review]
First Fix

Moving patch from CSP Spec Compliance bug to here.
Comment 2 Sid Stamm [:geekboy or :sstamm] 2012-08-21 13:52:03 PDT
Marshall: attachment 653835 [details] [diff] [review] is not based on top of mozilla-central (it looks like it depends on rolling back some change sets).  Can you pull mozilla-central, rebase the patch, and upload it again?
Comment 3 Marshall Moutenot 2012-08-21 16:45:05 PDT
Created attachment 654011 [details] [diff] [review]
Fix without all CSP spec compliance changes

Had hg queue problems. Rebased and only specific to this bug now.

Sorry 'bout that sid.
Comment 4 Sid Stamm [:geekboy or :sstamm] 2012-08-23 14:01:42 PDT
Comment on attachment 654011 [details] [diff] [review]
Fix without all CSP spec compliance changes

Review of attachment 654011 [details] [diff] [review]:

::: content/base/src/CSPUtils.jsm
@@ +1015,5 @@
> +      hostMatch = R_HOST.exec(aStr.substring(schemeMatch[0].length + 3));
> +      sObj._host = CSPHost.fromString(hostMatch[0]);
> +    } else {
> +      sObj._host = CSPHost.fromString(hostMatch[0]);
> +    }

This is probably gonna break host/port expressions (e.g., "foo:80").  The best approach is probably to check for scheme+host (you may have to make a new regex here), and extract host from it if they are both there, or it might be possible to use R_HOSTSRC and extract the host source from the result of exec()ing that.

@@ +1021,5 @@
>      if (!portMatch) {
>        // gets the default port for the given scheme
>        defPort =;
>        if (!defPort) {
> +        CSPError(CSPLocalizer.getFormatStr("couldntParseInvalidSource",[aStr]));

Please undo this whitespace change.

@@ +1036,5 @@
>    // check for 'self' (case insensitive)
>    if (aStr.toUpperCase() === "'SELF'"){
>      if (!self){
> +  CSPError(CSPLocalizer.getStr("selfKeywordNoSelfData"));

Same here, please undo this whitespace change.  (While you're at it, put a space between ) and { on the previous line.)

::: content/base/test/unit/test_csputils.js
@@ +522,5 @@
> +      do_check_true(cspr.permits("http://foo:80", SD.FRAME_ANCESTORS));
> +
> +      do_check_false(cspr.permits("https://foo:400", SD.FRAME_ANCESTORS));
> +      do_check_false(cspr.permits("https://self:34", SD.FRAME_ANCESTORS));
> +     });

Add a couple more checks here.  Update the frame-ancestors list to include a few more sources ("bar:80", "http://three") and make sure those are valid for the default HTTP port 80 and invalid for other ports by checking for permission for "http://bar" and "http://three:80", and making sure "https://bar" and "http://three:81" are blocked.

You probably want to add a few more negative tests here too (more https or weird ports, etc).
Comment 5 Marshall Moutenot 2012-08-23 16:25:28 PDT
Created attachment 654835 [details] [diff] [review]
Patch, correctly handles scheme conflicts
Comment 6 :Ehsan Akhgari 2012-08-24 10:04:49 PDT
*** Bug 785292 has been marked as a duplicate of this bug. ***
Comment 7 Sid Stamm [:geekboy or :sstamm] 2012-08-24 15:12:20 PDT
Comment on attachment 654835 [details] [diff] [review]
Patch, correctly handles scheme conflicts

Review of attachment 654835 [details] [diff] [review]:

Looks good.  At my request, mmoutenot did a bunch of trailing-whitespace removals, which will tag along as part of this patch.
Comment 8 Sid Stamm [:geekboy or :sstamm] 2012-08-27 08:51:16 PDT
Pushed to inbound:
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-27 19:20:08 PDT
Comment 11 Serge Montagnac 2012-09-15 02:22:02 PDT
This bug "Blocked by Content Security Policy".
is back in Aurora night release:


phpmyadmin frame opens normally in present 16 beta:

Comment 12 Matthias Versen [:Matti] 2012-09-21 12:25:20 PDT
*** Bug 791444 has been marked as a duplicate of this bug. ***
Comment 13 Ian Melven :imelven 2012-09-21 12:30:00 PDT
Maybe this fix should be uplifted to 17/Aurora ?
Comment 14 Daniel Veditz [:dveditz] 2012-09-21 12:46:45 PDT
This is a regression in Firefox 17 from bug 737064, we should uplift to 17 (especially since that's going to be an ESR). In particular this bug breaks CSP use on phpmyadmin sites (see comment 0) although I'm not sure if phpmyadmin uses CSP by default or if that's something bc turned on for his site.
Comment 15 Bob Clary [:bc:] 2012-09-21 13:15:06 PDT
I didn't turn it on explicitly.
Comment 16 Alex Keybl [:akeybl] 2012-09-21 16:56:30 PDT
Sid/Marshall - can you nominate for uplift to Aurora 17?
Comment 17 Ian Melven :imelven 2012-09-21 17:14:54 PDT
Comment on attachment 654835 [details] [diff] [review]
Patch, correctly handles scheme conflicts

(In reply to Alex Keybl [:akeybl] from comment #16)
> Sid/Marshall - can you nominate for uplift to Aurora 17?

i'll do it, i'll land it too if it's approved.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 737064
User impact if declined: phpmyadmin is broken by default, some CSP's won't work correctly and things may be incorrectly allowed/blocked
Testing completed (on m-c, etc.): has been on m-c since 08-27, mmoutenot added more tests to make sure this doesn't regress again and to further verify CSP source parsing
Risk to taking this patch (and alternatives if risky): seems low due to m-c bake time and adding tests for this case plus the existing CSP test suite
String or UUID changes made by this patch: nope
Comment 18 Toralf Förster 2012-09-22 02:58:29 PDT
I'm suffering from the same problem. What I did not udnerstand - it happens only, when I point my Firefox to a phpmyadmin instance running within a user mode linux instance at the same system.

It happens however now when I point Firefox to a phpmyadmin instance running at the host system itself.

For the UML instance the workaround is to point to the ip address rather than to the hostname.
Comment 19 Matthias Versen [:Matti] 2012-09-22 04:44:52 PDT
*** Bug 791444 has been marked as a duplicate of this bug. ***
Comment 20 Alex Keybl [:akeybl] 2012-09-24 10:06:45 PDT
Comment on attachment 654835 [details] [diff] [review]
Patch, correctly handles scheme conflicts

[Triage Comment]
Approving for Aurora uplift given the risk evaluation, where we are in the cycle, and the fact that this is basically a web compatibility regression fix.
Comment 21 Ian Melven :imelven 2012-09-24 14:30:20 PDT
Landed on Aurora, thanks !

Note You need to log in before you can comment on or make changes to this bug.