Last Comment Bug 702439 - javascript: links don't work in subframes of a 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 o...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla11
Assigned To: Brandon Sterne (:bsterne)
:
Mentors:
: 699893 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-14 14:34 PST by renuxman
Modified: 2012-02-01 14:00 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.58 KB, patch)
2011-11-18 13:31 PST, Brandon Sterne (:bsterne)
bzbarsky: review+
Details | Diff | Splinter Review
patch with test (4.50 KB, patch)
2011-11-23 16:13 PST, Brandon Sterne (:bsterne)
bzbarsky: review+
Details | Diff | Splinter Review
Remove unused variable (1017 bytes, patch)
2011-12-06 09:21 PST, :Ms2ger (⌚ UTC+1/+2)
brandon: review+
Ms2ger: checkin+
Details | Diff | Splinter Review

Description renuxman 2011-11-14 14:34:32 PST
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.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-14 14:44:15 PST
For what it's worth, it's working fine for me on current nightlies (FF11 nightlies)
Comment 2 Thomas Ahlblom 2011-11-14 15:04:57 PST
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
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-11-14 15:11:29 PST
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?
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-14 15:31:46 PST
No idea, no.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-11-14 17:32:40 PST
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.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-11-14 17:34:09 PST
And ccing the browser-bugs address, in hopes that the Facebook folks will actually notice this.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-11-14 17:36:26 PST
*** Bug 699893 has been marked as a duplicate of this bug. ***
Comment 8 renuxman 2011-11-15 09:28:56 PST
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 Marshall Roch 2011-11-15 19:56:35 PST
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
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-11-15 20:22:09 PST
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?
Comment 11 Brandon Sterne (:bsterne) 2011-11-16 08:39:14 PST
(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 :-(
Comment 12 Brendan Eich [:brendan] 2011-11-16 11:09:17 PST
Brandon, you taking this bug?

/be
Comment 13 Brandon Sterne (:bsterne) 2011-11-16 13:05:13 PST
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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-11-16 13:14:54 PST
> 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.
Comment 15 Marshall Roch 2011-11-17 21:45:27 PST
Daniel: isn't the problem that it's blocking while in Report-Only mode?
Comment 16 Brandon Sterne (:bsterne) 2011-11-18 09:05:22 PST
(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.
Comment 17 Brandon Sterne (:bsterne) 2011-11-18 13:31:00 PST
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.
Comment 18 Brandon Sterne (:bsterne) 2011-11-18 13:39:10 PST
(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 19 Boris Zbarsky [:bz] (still a bit busy) 2011-11-18 14:35:45 PST
Comment on attachment 575541 [details] [diff] [review]
patch v1

r=me
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2011-11-18 14:36:15 PST
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....
Comment 21 Brandon Sterne (:bsterne) 2011-11-23 16:13:07 PST
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>".
Comment 22 Brandon Sterne (:bsterne) 2011-11-23 16:14:15 PST
I am going to push this to Try while I wait for review, but I don't expect any surprises.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2011-11-24 11:32:17 PST
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.
Comment 24 Brandon Sterne (:bsterne) 2011-12-02 16:14:27 PST
Thanks, Boris.  Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c9926f0f45
Comment 25 Phil Ringnalda (:philor) 2011-12-02 19:53:02 PST
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?
Comment 26 Phil Ringnalda (:philor) 2011-12-02 22:29:24 PST
Off the hook for the leak, it persisted past your backout.
Comment 27 Brandon Sterne (:bsterne) 2011-12-05 09:43:46 PST
Push to m-i, take 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0706bf7b4508
Comment 28 :Ms2ger (⌚ UTC+1/+2) 2011-12-06 09:21:17 PST
Created attachment 579334 [details] [diff] [review]
Remove unused variable

This patch introduced an unused variable...
Comment 29 Ed Morley [:emorley] 2011-12-06 10:06:55 PST
https://hg.mozilla.org/mozilla-central/rev/0706bf7b4508

(Leaving open for comment 28's patch)
Comment 30 Brandon Sterne (:bsterne) 2011-12-08 15:00:10 PST
(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
Comment 31 Ed Morley [:emorley] 2011-12-09 06:53:55 PST
https://hg.mozilla.org/mozilla-central/rev/87a61be93cb7
Comment 32 :Ms2ger (⌚ UTC+1/+2) 2011-12-14 10:25:57 PST
Comment on attachment 579334 [details] [diff] [review]
Remove unused variable

http://hg.mozilla.org/mozilla-central/rev/87a61be93cb7

Note You need to log in before you can comment on or make changes to this bug.