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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 + fixed

People

(Reporter: bc, Assigned: mmoutenot)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Attached patch First Fix (obsolete) (deleted) — Splinter Review
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?
Attached patch Fix without all CSP spec compliance changes (obsolete) (deleted) — Splinter Review
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-
Attachment #654011 - Attachment is obsolete: true
Attachment #654835 - Flags: review?(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.
Attachment #654835 - Flags: review?(sstamm) → review+
Flags: in-testsuite+
Target Milestone: --- → mozilla18
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
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.
I didn't turn it on explicitly.
Sid/Marshall - can you nominate for uplift to Aurora 17?
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?
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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: