CSP parser not correctly parsing single token hosts

RESOLVED FIXED in Firefox 17

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bc, Assigned: Marshall Moutenot)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla18
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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:

<quote>
The CSP is:

allow 'self'; options inline-script eval-script; frame-ancestors 'self';
img-src 'self' data:; script-src 'self' http://www.phpmyadmin.net

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

The site is not publicly available but I can provide instructions if you have mpt access.
Assignee: nobody → mmoutenot
Status: NEW → ASSIGNED
Depends on: 737064
Hardware: x86 → All
Summary: CSP blocking phpmyadmin with single token hosts → CSP parser not correctly parsing single token hosts
(Assignee)

Comment 1

5 years ago
Created attachment 653835 [details] [diff] [review]
First Fix

Moving patch from CSP Spec Compliance bug to here.
Attachment #653835 - Flags: feedback?(sstamm)
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?
(Assignee)

Comment 3

5 years ago
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.
Attachment #653835 - Attachment is obsolete: true
Attachment #653835 - Flags: feedback?(sstamm)
Attachment #654011 - Flags: feedback?(sstamm)
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 = Services.io.getProtocolHandler(sObj._scheme).defaultPort;
>        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).
Attachment #654011 - Flags: feedback?(sstamm) → feedback-
(Assignee)

Comment 5

5 years ago
Created attachment 654835 [details] [diff] [review]
Patch, correctly handles scheme conflicts
Attachment #654011 - Attachment is obsolete: true
Attachment #654835 - Flags: review?(sstamm)
Duplicate of this bug: 785292
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.
Attachment #654835 - Flags: review?(sstamm) → review+
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5184abd63490
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/5184abd63490
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 10

5 years ago
https://sourceforge.net/tracker/index.php?func=detail&aid=3560441&group_id=23067&atid=377408

Comment 11

5 years ago
This bug "Blocked by Content Security Policy".
is back in Aurora night release:

firefox-17.0a2.en-US.linux-i686.tar.bz2

phpmyadmin frame opens normally in present 16 beta:

firefox-16.0b3.tar.bz2
status-firefox17: --- → affected
Duplicate of this bug: 791444

Comment 13

5 years ago
Maybe this fix should be uplifted to 17/Aurora ?
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.
tracking-firefox17: --- → ?
(Reporter)

Comment 15

5 years ago
I didn't turn it on explicitly.
Sid/Marshall - can you nominate for uplift to Aurora 17?
tracking-firefox17: ? → +

Comment 17

5 years ago
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
Attachment #654835 - Flags: approval-mozilla-aurora?

Comment 18

5 years ago
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.
Duplicate of this bug: 791444
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.
Attachment #654835 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 21

5 years ago
Landed on Aurora, thanks ! 

https://hg.mozilla.org/releases/mozilla-aurora/rev/3227b2f0e9ef

Updated

5 years ago
status-firefox17: affected → fixed
You need to log in before you can comment on or make changes to this bug.