784315 - CSP parser not correctly parsing single token hosts

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bob Clary [:bc] (inactive)]

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.

Comment 1

[Marshall Moutenot]

Attachment: First Fix

Moving patch from CSP Spec Compliance bug to here.

Comment 2

[Sid Stamm [:geekboy or :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?

Comment 3

[Marshall Moutenot]

Attachment: 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]]

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

Comment 5

[Marshall Moutenot]

Attachment: Patch, correctly handles scheme conflicts

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

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

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5184abd63490

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5184abd63490

Comment 10

[J.M.]

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

Comment 11

[Serge Montagnac]

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

Comment 13

[Ian Melven :imelven]

Maybe this fix should be uplifted to 17/Aurora ?

Comment 14

[Daniel Veditz [:dveditz]]

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] (inactive)]

I didn't turn it on explicitly.

Comment 16

[Alex Keybl [:akeybl]]

Sid/Marshall - can you nominate for uplift to Aurora 17?

Comment 17

[Ian Melven :imelven]

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]

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 20

[Alex Keybl [:akeybl]]

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]

Landed on Aurora, thanks ! 

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