javascript: links don't work in subframes of a page with a CSP policy, even one in report-only mode

RESOLVED FIXED in mozilla11

Status

()

Core
Security
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: renuxman, Assigned: bsterne)

Tracking

Trunk
mozilla11
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

anchor href="javascript:..." syntax does not work anymore for apps in Facebook Iframe with Firefox

TEST CASE:

1) Go to http://apps.facebook.com/nuxfootprints/ (no Facebook account is needed, you can be completely logged out of Facebook)

2) Click on the test link "anchor href=javascript test"


Actual results:

Javascript code does not run. Nothing is reported in firebug either. It simply does nothing.

This issue repros in Firefox 5.0 in Linux ,and Firefox 8.0 in Windows.

Note: before upgrading to Firefox 8.0 in Windows, I tried this in Firefox 3.something in Windows and it worked. It also works in IE and Chrome.


Expected results:

Javascript code should be executed.

This issue basically breaks my app in Facebook with Firexfox.
For what it's worth, it's working fine for me on current nightlies (FF11 nightlies)

Comment 2

6 years ago
Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0

WFM:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.24) Gecko/20111103 Firefox/3.6.24
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111114 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111114 Firefox/11.0a1
Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.835.202 Safari/535.1
Opera/9.80 (X11; Linux x86_64; U; en) Presto/2.9.168 Version/11.52
Thomas, thank you for testing that!  I was about to ask whether it worked on beta.  Since it does, it should be fixed once we ship Firefox 9, on Dec 20.

On nightlies, I see this start working in this range:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=630e28e90986&tochange=f69a10f23bf3

That's the same fix range as bug 699893, and similar symptoms.

Sicking, any idea what might have fixed this?
Blocks: 699893
No idea, no.
My bisect ended up on the versio bump.

And indeed, if I take Firefox 8 and load the testcase in it, I see the bug.  If I then change the UA string of Firefox 8 to this:

  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0

(no code changes; just change the UA string) then the bug disappears.  So there's some sort of broken UA sniffing going on on the Facebook end.
Assignee: nobody → english-other
Status: UNCONFIRMED → NEW
Component: General → English Other
Ever confirmed: true
Product: Firefox → Tech Evangelism
QA Contact: general → english-other
Version: 5 Branch → unspecified
And ccing the browser-bugs address, in hopes that the Facebook folks will actually notice this.
Duplicate of this bug: 699893
(Reporter)

Comment 8

6 years ago
Added comments to Facebook's bug reports about this issue and linked back to this bug report:
http://developers.facebook.com/bugs/197517616988383
http://developers.facebook.com/bugs/282419221777641

Comment 9

6 years ago
This is being caused by our X-Content-Security-Policy-Report-Only header, which we are sending to Firefox <= 8, but not Firefox 9 until we've tested Firefox 9. Doesn't make sense that the *-Report-Only header is blocking anything, though.

The content of the header is:

allow *;script-src https://*.facebook.com http://*.facebook.com https://*.fbcdn.net http://*.fbcdn.net *.facebook.net *.google-analytics.com *.virtualearth.net *.google.com 127.0.0.1:* *.spotilocal.com:*;options inline-script eval-script;report-uri http://www.facebook.com/csp.php
Marshall, thanks!  That's enough info for me to fix on our end.  Though you may still want to stop sending that header for down-rev Firefox....  :(

bsterne, nsJSChannel is not a writable property bag, and NS_NewChannel currently errors out if there is a non-null channelPolicy and the channel is not a writable property bag.  Is that desired/expected behavior (in that nsJSChannel needs to implement this interface), or just an accidental set of |rv| in NS_NewChannel?
Assignee: english-other → nobody
Component: English Other → Security
Product: Tech Evangelism → Core
QA Contact: english-other → toolkit
Version: unspecified → Trunk
(Assignee)

Comment 11

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #10)
> bsterne, nsJSChannel is not a writable property bag, and NS_NewChannel
> currently errors out if there is a non-null channelPolicy and the channel is
> not a writable property bag.  Is that desired/expected behavior (in that
> nsJSChannel needs to implement this interface), or just an accidental set of
> |rv| in NS_NewChannel?

The JSChannel is not a writable property bag, but the channel it creates is:
http://hg.mozilla.org/mozilla-central/annotate/30161b298513/dom/src/jsurl/nsJSProtocolHandler.cpp#l531

That's the thing CSP cares about.  CSP needs to add the channelPolicy object to the channel's property bag to handle redirects.  When a channel redirects, the channelPolicy object is forwarded from the old channel to the new channel.

So my assumption that every channel that matters within the context of CSP will implement nsIWritablePropertyBag, and create their channels using NS_NewChannel.  I believe the fix is simply to make nsJSChannel implement the writable property bag.  I'm not sure how we didn't catch this sooner :-(
Brandon, you taking this bug?

/be
(Assignee)

Comment 13

6 years ago
Yes, I'll take this.  Boris was exactly right when he asked:

(In reply to Boris Zbarsky (:bz) from comment #10)
> bsterne, ... Is that desired/expected behavior ... or just an accidental set of
> |rv| in NS_NewChannel?

This is in fact a misguided setting of rv from within that QI, since I don't check rv anyway and simply pass it through.  So we don't actually need to make nsJSChannel implement nsIWritablePropertyBag.

Before I write the patch to remove the rv setting, is it safe to assume that any channel that doesn't implement nsIWritablePropertyBag isn't capable of redirecting?  If this isn't true, than we have other potential hazards where a redirect can be used to bypass CSP.

One way to address the latter would be to add an assertion, or even return an error from our redirect listener if the channel doesn't implement writable property bag.  This would be a change to our current code which simply allows the redirect (from CSPs perspective) if the channel isn't a WPB:
http://hg.mozilla.org/mozilla-central/annotate/53f4c8abf558/content/base/src/nsCSPService.cpp#l220

That code essentially makes the assumption that I am questioning above.
Assignee: nobody → bsterne
> is it safe to assume that any channel that doesn't implement nsIWritablePropertyBag isn't
> capable of redirecting?

For extension-implemented channels, probably not.  If the writable property bit is only needed to handle redirects securely, then failing redirect of non-writable-property-bag channels when there is a CSP seems much better than failing their creation when there is a CSP to me.  And logging something to the error console in that situation would be _really_ nice.

Updated

6 years ago
No longer blocks: 699893
Summary: anchor href="javascript:..." syntax does not work anymore for apps in Facebook Iframe with Firefox → javascript: links don't work for apps in Facebook Iframes even though CSP policy allows inline-script

Comment 15

6 years ago
Daniel: isn't the problem that it's blocking while in Report-Only mode?
Summary: javascript: links don't work for apps in Facebook Iframes even though CSP policy allows inline-script → javascript: links don't work on any page with a CSP policy, even one in report-only mode
(Assignee)

Comment 16

6 years ago
(In reply to Marshall Roch from comment #15)
> Daniel: isn't the problem that it's blocking while in Report-Only mode?

Not really. The bug is that all javascript: links are broken when any CSP policy is sent: report-only or regular, as well as policies that allow inline-script.

I'll put up a patch shortly.
(Assignee)

Comment 17

6 years ago
Created attachment 575541 [details] [diff] [review]
patch v1

Here's the meat of the patch, which I built and verified fixes the bug (allows the javascript: link in the subframe to load).

Requesting bz's review on the substance of the patch and in the meantime I'll write a test and add a console warning when we kill the redirect to a non-writable property bag.
Attachment #575541 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

6 years ago
(In reply to Marshall Roch from comment #9)
> The content of the header is:
> 
> allow *;script-src https://*.facebook.com http://*.facebook.com
> https://*.fbcdn.net http://*.fbcdn.net *.facebook.net *.google-analytics.com
> *.virtualearth.net *.google.com 127.0.0.1:* *.spotilocal.com:*;options
> inline-script eval-script;report-uri http://www.facebook.com/csp.php

FYI, this policy will not generate any reports for you if the top-level page is served from https.  The report-uri must share the same scheme, port and public suffix +1 as the top-level document:
https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#report-uri

A relative URI is valid there, so you may want to just remove the scheme so the report-uri works for both http and https pages.
Comment on attachment 575541 [details] [diff] [review]
patch v1

r=me
Attachment #575541 - Flags: review?(bzbarsky) → review+
Given the lack of motion on the Facebook side on this, we probably need to backport this to aurora and maybe even beta if we can....
(Assignee)

Comment 21

6 years ago
Created attachment 576645 [details] [diff] [review]
patch with test

Same patch as before but adds a test, and a console warning when the redirect is canceled due to no writable property bag.

Boris, what's your preference for the content of the error message? Right now it just says "Warning: Unable to redirect to <URL>".
Attachment #575541 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
I am going to push this to Try while I wait for review, but I don't expect any surprises.
Summary: javascript: links don't work on any page with a CSP policy, even one in report-only mode → javascript: links don't work in subframes of a page with a CSP policy, even one in report-only mode
(Assignee)

Updated

6 years ago
Attachment #576645 - Flags: review?(bzbarsky)
Comment on attachment 576645 [details] [diff] [review]
patch with test

You may be able to use the logging facilities in nsContentUtils to simplify your logging code (and incidentally make it localizable!).  Also, mentioning the need to implement nsIWritablePropertyBag2 is a good idea.  Something like:

  Unable to redirect to %S because the channel doesn't implement
  nsIWritablePropertyBag2.

One other thing...  You should check the return value of SetPropertyAsInterface.  A JS-implemented channel might QI to nsIWritablePropertyBag2 but throw on attempts to invoke any method from the interface.

r=me with those nits.
Attachment #576645 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 24

6 years ago
Thanks, Boris.  Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c9926f0f45
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a9e843e573 - somebody's leaking in browser-chrome, and with all the burning from the missing test and burning from ftp.m.o being broken, it's hard to tell who.

I picked you to back out first because the dom.properties hunk of https://hg.mozilla.org/integration/mozilla-inbound/rev/79c9926f0f45 is pretty broken looking, missing a newline after your added string, but... I don't see that in either of the patches attached to this bug. Was it from some other bug, or not the patch you meant to push?
Off the hook for the leak, it persisted past your backout.
(Assignee)

Comment 27

6 years ago
Push to m-i, take 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0706bf7b4508
Created attachment 579334 [details] [diff] [review]
Remove unused variable

This patch introduced an unused variable...
Attachment #579334 - Flags: review?(bsterne)
https://hg.mozilla.org/mozilla-central/rev/0706bf7b4508

(Leaving open for comment 28's patch)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla11
(Assignee)

Comment 30

6 years ago
(In reply to Ms2ger from comment #28)
> Created attachment 579334 [details] [diff] [review]
> Remove unused variable
> 
> This patch introduced an unused variable...

Thanks, pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a61be93cb7
https://hg.mozilla.org/mozilla-central/rev/87a61be93cb7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #579334 - Flags: review?(bsterne) → review+
Comment on attachment 579334 [details] [diff] [review]
Remove unused variable

http://hg.mozilla.org/mozilla-central/rev/87a61be93cb7
Attachment #579334 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.