Closed
Bug 784315
Opened 12 years ago
Closed 12 years ago
CSP parser not correctly parsing single token hosts
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: bc, Assigned: mmoutenot)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
24.65 KB,
patch
|
geekboy
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
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•12 years ago
|
||
Moving patch from CSP Spec Compliance bug to here.
Attachment #653835 -
Flags: feedback?(sstamm)
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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 4•12 years ago
|
||
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•12 years ago
|
||
Attachment #654011 -
Attachment is obsolete: true
Attachment #654835 -
Flags: review?(sstamm)
Comment 7•12 years ago
|
||
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+
Comment 8•12 years ago
|
||
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5184abd63490
Flags: in-testsuite+
Target Milestone: --- → mozilla18
Comment 9•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 10•12 years ago
|
||
Comment 11•12 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
Updated•12 years ago
|
status-firefox17:
--- → affected
Comment 13•12 years ago
|
||
Maybe this fix should be uplifted to 17/Aurora ?
Comment 14•12 years ago
|
||
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•12 years ago
|
||
I didn't turn it on explicitly.
Comment 17•12 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•12 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.
Comment 20•12 years ago
|
||
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•12 years ago
|
||
Landed on Aurora, thanks !
https://hg.mozilla.org/releases/mozilla-aurora/rev/3227b2f0e9ef
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•