Closed Bug 656433 Opened 13 years ago Closed 13 years ago

Disallow javascript: and data: URLs entered into the location bar from inheriting the principal of the currently-loaded page

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 6
Tracking Status
firefox5 - wontfix
blocking2.0 --- -
status2.0 --- wontfix

People

(Reporter: bsterne, Assigned: Gavin)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-spoof, dev-doc-needed, sec-want, Whiteboard: Developers: Use the developer console (Ctrl+Shift+K) instead)

Attachments

(1 file, 3 obsolete files)

I'm splitting this off from bug 527530 as a short term solution to the bookmarklet-pasting attacks being seen in the wild on Facebook and similar.

The change this bug will make is to disallow loading of javascript: URLs from the URL bar *at all*.  This behavior varies slightly from IE 9 which strips the leading "javascript:" from a pasted URL, and Chrome which does something similar.

The approach in this bug still enables bookmarklet functionality through bookmarks as well as entering them on the Web Console or Error Console.
Attached patch simplest fix (obsolete) — Splinter Review
This is the fix I spoke with Gavin about earlier today.  It seems to be the smallest and safest patch to fix the bug.  I'm hoping to land it on Aurora for Fx5.
Attachment #531737 - Flags: review?(gavin.sharp)
(In reply to comment #1)
> This is the fix I spoke with Gavin about earlier today.  It seems to be the
> smallest and safest patch to fix the bug.  I'm hoping to land it on Aurora for
> Fx5.
Er, Aurora?
(In reply to comment #0)
> This behavior varies slightly from IE 9 which
> strips the leading "javascript:" from a pasted URL, and Chrome which does
> something similar.

If Chrome and IE are both doing something similar, why should we differ? I guess just stripping "javascript:" doesn't make much sense, since it leaves users with invalid junk in the URL bar. Why did they choose that approach, I wonder?

I liked the idea of a paste-specific approach, since that leaves only relatively impractical attack alternatives (convincing users to manually type part of it in, having them paste it in the error console instead, etc.).
(In reply to comment #2)
The motivation for wanting it on Aurora is that we're seeing a lot of attacks in the wild that involve convincing users to paste javascript: URIs, so we'd like to fix it ASAP.
(In reply to comment #3)
> I liked the idea of a paste-specific approach, since that leaves only
> relatively impractical attack alternatives (convincing users to manually type
> part of it in, having them paste it in the error console instead, etc.).
That's just a matter of opinion though, right?  I mean, I think it's relatively impractical for users to copy and paste something from a web page into their address bar, but here we are.
(In reply to comment #3)
> (In reply to comment #0)
> > This behavior varies slightly from IE 9 which
> > strips the leading "javascript:" from a pasted URL, and Chrome which does
> > something similar.
> 
> If Chrome and IE are both doing something similar, why should we differ? I
> guess just stripping "javascript:" doesn't make much sense, since it leaves
> users with invalid junk in the URL bar. Why did they choose that approach, I
> wonder?
> 
> I liked the idea of a paste-specific approach, since that leaves only
> relatively impractical attack alternatives (convincing users to manually
> type part of it in, having them paste it in the error console instead, etc.).

The very fact that pages are able to convince a giant heap of gobbledygook and paste it into the urlbar says a lot about what users are willing to do. Adding an extra step of asking the user to add "javascript: in the beginning doesn't seem like a far stretch to me.

Also, as you point out, users are left with something unusable in the url-bar. I don't really see who would want that behavior, beginners or power users alike.
We need to fix this for data: URLs too. I'd prefer loading data: URLs from the address bar will a null principal, rather than disabling them.

> if (/^(\s)*javascript:/.test(url.toLowerCase()))

Can we use the standard URL parser, instead of adding yet another (blacklist!) regexp that may or may not match its behavior?
(In reply to comment #7)
> We need to fix this for data: URLs too. I'd prefer loading data: URLs from
> the address bar will a null principal, rather than disabling them.
> 
> > if (/^(\s)*javascript:/.test(url.toLowerCase()))
> 
> Can we use the standard URL parser, instead of adding yet another
> (blacklist!) regexp that may or may not match its behavior?

It looks like there is code later which does URI/scheme verification (makeURI / schemeIs). Perhaps the patch can copy/paste that?

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#454
(In reply to comment #8)
> It looks like there is code later which does URI/scheme verification (makeURI /
> schemeIs). Perhaps the patch can copy/paste that?
Yes, makeURI gives an nsIURI object which will use our standard parser.  Also, this really ought to have a test.
Attached patch Simple fix v2 (obsolete) — Splinter Review
(In reply to comment #7)
> We need to fix this for data: URLs too. I'd prefer loading data: URLs from
> the address bar will a null principal, rather than disabling them.
> 
> Can we use the standard URL parser, instead of adding yet another
> (blacklist!) regexp that may or may not match its behavior?

patch v2 addresses both of these.
Attachment #531737 - Attachment is obsolete: true
Attachment #531737 - Flags: review?(gavin.sharp)
Attachment #531796 - Flags: review?(gavin.sharp)
Comment on attachment 531796 [details] [diff] [review]
Simple fix v2

re: the more general question of what to do:

Disabling only pastes *will* reduce the success rate of these attacks (impossible to say by how much). Disabling entering javascript: URIs in the location bar entirely will reduce the success rate further (impossible to say how much further), by eliminating the "type this prefix yourself and then paste this" variants, at the cost of breaking functionality that some users (a minority) have come to depend on (e.g. loading simple data: URIs to test something quickly). Evaluating that tradeoff isn't trivial, and given the "impossible to says", it's very subjective.

A cleaner solution to this problem might be to force a blank context for the load, so that the security context of the currently loaded page isn't inherited. We could do this hackily by forcing the load into a new tab, but there might be a cleaner solution involving a new load flag to be passed to LoadURI.

re: this patch specifically:

We should use URIChainHasFlags(URI_INHERITS_SECURITY_CONTEXT) rather than a regex check. http://hg.mozilla.org/mozilla-central/annotate/1706e681390c/browser/components/nsBrowserContentHandler.js#l514 is an example. A test that covers clicks on the Go button and Enter in the location bar would be good, too.
Attachment #531796 - Flags: review?(gavin.sharp) → review-
(In reply to comment #12)
> rather than a regex check.

s/regex/schemeIs/, but the point stands :)
Attached patch more complicated fix (obsolete) — Splinter Review
This implements my idea of blocking these loads from inheriting the currently loaded page's principal, rather than blocking them entirely. This seems to have the effect of blocking loads of javascript: entirely (not sure why exactly - I assume inheriting the current principal is required for them to have some place to execute), but leaving data: URIs usable (in a way that isn't dangerous - data:text/html,<script>alert(document.cookie);</script> will not show the current document's cookies).

This is a quick hack I threw together - the nsDocShell.cpp code here is fairly complicated, and I couldn't follow all of the logic related to inheritOwner (or match it up with the comments).
Attachment #531992 - Flags: feedback?
Please, please, please make this a (hidden, I guess) pref.  This is an invaluable tool for debugging web pages; I wouldn't be able to get my day-to-day work on Gecko done without it...
Attachment #531992 - Flags: feedback? → feedback?(bzbarsky)
(In reply to comment #15)
> Please, please, please make this a (hidden, I guess) pref.  This is an
> invaluable tool for debugging web pages; I wouldn't be able to get my
> day-to-day work on Gecko done without it...

Does running the JS snippets from the Web Console not meet your requirements?
Not quite, for two reasons (there are other less important ones, but these are the key ones):

1)  This bug is blocking data: URIs too; I use those all the time.
2)  The web console execution environment is not the same as the page execution
    environment in various ways that cause results to differ in some cases.

Just try this:

1)  data:text/html,<div id="x"></div>
2)  Open web console
3)  Type 'x' in the evaluation field.  Hit enter.
4)  Type 'javascript:alert(x)' in the url bar, hit enter.
5)  Compare the results.

(There are variations; e.g. this.x behaves the same as the bareword 'x' except it returns undefined in the web console instead of throwing, etc.)

In particular, you can't use script extracted from the page in the web console and assume it will work the same way as in the page itself without very very careful analysis.
After further reading, my issue #1 is not a problem.  I can probably try to work with the web console's limitations for issue #2...
Comment on attachment 531992 [details] [diff] [review]
more complicated fix

The docshell parts of this look fine (you can consider this an r= on those parts), if you fix the IDL comment to say something useful.

The reason javascript: stops running completely with this change is that you tell it to not inherit the page principal, but don't give it another principal to run as, and we don't run javascript: URIs if we don't have a principal to do it with.
Attachment #531992 - Flags: review+
Attachment #531992 - Flags: feedback?(bzbarsky)
Attachment #531992 - Flags: feedback+
Gavin's going to take this from here.  Thanks for stepping in, and thanks bz for the review.
Assignee: bsterne → gavin.sharp
I didn't see anyone answer Gavin's question from comment 3, first paragraph, and I agree with Gavin's comment 12 on the subjectivity of the trade-off. For data:, bz agreed too.

I'd rather we do what IE9 and Chrome do, than something different and breakier for my subjective trade-off result, which favors typing javascript: into the URL bar often. I hardly ever paste. Really, if I type it, I want to see it load.

bz: any definite idea on why the patch stops all javascript: URLs cold?

/be
(In reply to comment #21)
> I didn't see anyone answer Gavin's question from comment 3, first paragraph

I think the general consensus about those approaches are that they resulted in pretty bad user experience (you just end up loading an invalid URL?), and yet they were still susceptible to additional-hoop-jumping attacks (first type this in, then paste this, etc. - I think this is a weak argument).

> bz: any definite idea on why the patch stops all javascript: URLs cold?

See last paragraph of comment 19. Perhaps I will investigate finding a way to have the JS evaluate against a safe/new principal. Seems doable in theory!
Doing that is just a matter of setting owner to a new nullprincipal in docshell in the cases when this patch would prevent inheriting.
Attached patch complete patchSplinter Review
I'll investigate doing that in a (potentially quick) followup.
Attachment #531796 - Attachment is obsolete: true
Attachment #531992 - Attachment is obsolete: true
Attachment #532083 - Flags: review?(dao)
Blocks: 656823
(In reply to comment #22)
> (In reply to comment #21)
> > I didn't see anyone answer Gavin's question from comment 3, first paragraph
> 
> I think the general consensus about those approaches are that they resulted
> in pretty bad user experience (you just end up loading an invalid URL?),

We could completely refuse to take the pasted value...
Attachment #532083 - Flags: review?(dao) → review+
(In reply to comment #26)
> We could completely refuse to take the pasted value...

Only if it starts with "javascript:" right? If so, the site could just make the user copy a string that doesn't start with that string and then ask the user to prepend it after pasting.
(In reply to comment #27)
> (In reply to comment #26)
> > We could completely refuse to take the pasted value...
> 
> Only if it starts with "javascript:" right?

Yes.

> If so, the site could just make
> the user copy a string that doesn't start with that string and then ask the
> user to prepend it after pasting.

That's a different concern from the one I was responding to.
http://hg.mozilla.org/mozilla-central/rev/5f42fe17284f
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Summary: Disallow javascript: URLs to be run from the URL bar → Disallow javascript: and data: URLs entered into the location bar from inheriting the principal of the currently-loaded page
(In reply to comment #4)
> The motivation for wanting it on Aurora is that we're seeing a lot of
> attacks in the wild that involve convincing users to paste javascript: URIs,
> so we'd like to fix it ASAP.

Awesome work, Gavin. Thanks.  I realize the merge from Aurora to Beta for Firefox 5 was happening this morning, but is there any way we can land this there?
The right thing to do now is to request beta approval if we want to get this in FF5.
Attachment #532083 - Flags: approval-mozilla-beta?
Whiteboard: [sg:want]
blocking2.0: --- → -
status2.0: --- → wontfix
additionally, because this has only been on m-c for a couple of days, a description of the risk of the patch and what kind of testing's been done to ameliorate any risk would be helpful.
Risks include:

* Making web developers sad, because they don't know about the alternatives (such as Web Console in Fx5+ and Scratchpad in Fx6+) or find the alternatives unwieldy.

* Breaking instructions on web sites.  For example, the widespread cheat to "unlock all levels in Angry Birds" involves pasting a javascript: URL.

We'll learn the extent of these issues through widespread testing.
This breaks keyword invoked parameterized javascript: bookmarklets.  The javascript: url is not typed or pasted to the urlbar, it's stored as a bookmark and invoked on the urlbar with a keyword and an argument.

Example:

javascript:open('http://www.google.com/search?q=%s','_self');open('http://www.bing.com/search?q=%s');

Considering that regular (mouse invoked) javascript: bookmarklets still work,  there is no justification for breaking the keyword invoked ones, hopefully this was not intentional as they are very useful and should still work even if typed/pasted javascript: urls are disallowed.

__________________________________________________________

> We'll learn the extent of these issues through widespread testing.

If you make this optional, there will be no issues.  Many people are able and willing to take responsibility for typing and running javascript: urls, they won't mind changing a pref.  The issues will stem from forcing this straitjacket on everyone.
The issue you mentioned is due to the _canonizeURL method which calls getShortcutOrURI. This could be moved within handleCommand so as to allow keyword bookmarklets (which as far as bookmarklets go, I believe, aren't that rare).

For now, you can get the old behaviour by clicking the Go button instead of hitting Enter. I'm not sure if that was intentional or an oversight. You can also get non-document type stuff (alert(2+3) etc.) by hitting Alt+Enter.
(In reply to comment #35)
> For now, you can get the old behaviour by clicking the Go button instead of
> hitting Enter. I'm not sure if that was intentional or an oversight.

Bug 658220 - keyword bookmarklets don't run on enter
Depends on: 658220
I don't think we should take this on beta. We need to shake things out more before shipping it.
Given the regression I reluctantly agree
Comment on attachment 532083 [details] [diff] [review]
complete patch

This needs a full cycle to shake it all out. Not going to take this for 5.
Attachment #532083 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
After reading through this and experimenting, it looks as if data: and javascript: URLs are essentially just ignored if typed into the URL bar. Is that a correct assessment?
javascript: is more or less ignored when typed in.

data: is loaded fine, but just gets a new security context, not the same one as the page that was loaded before.
javascript: is not actually ignored - they're run, but in an "empty" context that doesn't have any of the usual DOM methods you would expect, so most common uses (e.g. javascript:alert(1)) just throw (and thus are effectively ignored). javascript:1+1 works fine, though.
Updated:

https://developer.mozilla.org/en/data_URIs

And mentioned on Firefox 6 for developers.
Whiteboard: [sg:want] → [sg:want] [tracking+ because it's new in 6]
is there a way to activate this behavior (javascript in url bar) again?
I find it very annoying to create a new bookmark to use javascript: URIs and Scratchpad doesn't work the way I want. It only shows me errors, etc.

Please revert this or make setting for it. I don't want to be patronized, I am an experienced computer user/developer/engineer I know what I do with my Browser and/or on web sites I visit and I want fully control about my system.
Does the tracking change mean this is being dropped from Firefox 6?
(In reply to Eric Shepherd [:sheppy] from comment #46)
> Does the tracking change mean this is being dropped from Firefox 6?

No. It means that the release team is no longer keeping an eye on this new feature.
Blocks: 680302
Why should developers like me, who really need the javascript: functionality, be private to use this feature while we are coding and testing.

Most of web page today are created with a whole javascript background and I think we should be able to test and debug or code in some way.

Please back this feature up or put an option for enable this in debug mode.
Is there a reason you can't simply use the developer console? I.e. press Ctrl-K and you'll get to a place which is much nicer for evaluating JS-expressions.
This changes sucks.  I wish Firefox team would stop making changes that remove features I use everyday without a way of re-enabling them.  https://support.mozilla.com/en-US/questions/885558#answer-259571
I'm really worried Mozilla is making changes in six months to please a PRIVATE CORPORATION when dozens of serious years old bugs are being ignored or postponed.
The change here wasn't to help facebook, but rather people that use facebook. It wasn't facebook that was being hacked, it was users of facebook who got their data stolen.

As it turns out, there are a lot of people that use facebook, hence we wanted to help them.

Additionally, the problem here was in absolutely no way facebook specific. So likely it was happening in plenty of other places too. It just so happened that facebook was able to accumulate data showing it was happening there, but I see no reason to believe it was happening only there.
(In reply to Jonas Sicking (:sicking) from comment #52)
> The change here wasn't to help facebook, but rather people that use
> facebook. It wasn't facebook that was being hacked, it was users of facebook
> who got their data stolen.

Facebook has a notorious habit of hardening access to features not enabled in their interface. One example is asking a question to all your friends, which is horribly hard to perform only using clicks when the average person has about 100 contacts or so. As a consequence other people dig into the javascript page source and taught other people how to instantly select all people. This obviously annoyed fb so they went screaming "omg our users emails are getting stolen!!!1111oneone"
I'm just that happened too, but there were also actual user data getting stolen too.
I'm not complaining about disabling the feature by default but not allowing a setting to turn it back on for those of us who know better was wrong.  This is dumbing down the app to the least common denominator.
There is a side effect of this fix that has not been discussed on this thread so far.

This side effect is that the href="javascript:..." syntax does not work anymore for apps in Facebook Iframe with FireFox.

Some developers have been lucky enough to apply a hack to make their apps work again in Facebook Iframe with FireFox:
http://stackoverflow.com/questions/7918874/href-javascript-syntax-suddenly-does-not-work-in-fb-iframe-in-firefox

The hack uses jquery, and dynamically searches for all anchors with href="javascript:" attributes, gets the js code from the href attribute, and applies the js code to the onclick event of the anchor element.

Unfortunately, I have hundreds of these elements throughout my app, and worst of all, most of them are generated dynamically, so I can not just apply the hack once right after my app has been loaded.

So I would either have to apply the hack every time after I generate dynamically a new anchor element, or I would have to replace all my anchor href="javascript:..." syntax, with anchor onclick="..." syntax.

Needless to say, both of these are major changes that could potentially introduce many bugs in my app, and since the number of users that I have in Facebook with Firefox is not very big, my "solution" for now is going to be, not to load my app in Facebook with Firefox, but instead show a message to my users saying that because of a recent change in Firefox, my app does not work in Facebook with Firefox anymore, and suggest them to use Chrome instead.

I would really appreciate it if you could reconsider doing what Chrome and IE are doing, instead of differing with your own solution and breaking apps in Facebook Iframe with FireFox.

Firefox is a great browser. I would really not like at all, to have to drop support for my app in Facebook with Firefox for good.
> This side effect is that the href="javascript:..." syntax does not work anymore for apps
> in Facebook Iframe with FireFox.

This bug should have had absolutely no effect on that; the code changed in this bug does not run for <a href="javascript:....">.  So whatever problem you're seeing has nothing to do with this bug.

If you have a testcase that shows such links not working, please file a bug on that testcase and add me to the cc list.  A testcase that does not involve a Facebook account would be _very_ much appreciated.
Hi Boris, thanks for your prompt reply, please find new bug and test case at:
https://bugzilla.mozilla.org/show_bug.cgi?id=702439

No Facebook account required :-)
I second #45 - there are plenty of people that know what they're doing. It makes my work much harder now. Running JavaScript commands at the address bar is invaluable as it lets me test behavior and other aspects that otherwise require special circumstances and tons of workarounds.

Those that want to hack will continue doing it - they'll find a way.
You're not stopping them, you're stopping regular developers.

Please add an option (I don't mind being hidden) so that we can revert this if necessary.
Whiteboard: [sg:want] [tracking+ because it's new in 6] → Developers: Use the developer console (Ctrl+K) instead
(In reply to vs3 from comment #59)
> I second #45 - there are plenty of people that know what they're doing. It
> makes my work much harder now. Running JavaScript commands at the address
> bar is invaluable as it lets me test behavior and other aspects that
> otherwise require special circumstances and tons of workarounds.
> 
> Those that want to hack will continue doing it - they'll find a way.
> You're not stopping them, you're stopping regular developers.
> 
> Please add an option (I don't mind being hidden) so that we can revert this
> if necessary.

Hi, I have written an addon which reverts this and allows you to execute javascript uris in urlbar. You may take a look at it in
https://addons.mozilla.org/en-US/firefox/addon/inheritprincipal/
Thanks,
I installed that on FF 8.0 and I still can't get this command to run on the URL bar
javascript:self.resizeTo(1024,768)
That has nothing to do with this bug.  javascript: urls can only do what web pages can do, and web pages can no longer resize windows they didn't open.
Unlike closed-source browsers that impose or remove features that people want/don't want, I thought the point of open-source browsers is to prevent that from happening.

A simple javascript:alert("Time to switch browsers") returns NOTHING. Justifying this by saying that a bunch of 13 year old facebook kids get tricked into pasting a javascript link is barely adequate.

Let's remember what happens when you sacrifice too much liberty for "security". This applies to everything. I am very close to stopping using this once great browser.
(In reply to nick9v from comment #63)
> Unlike closed-source browsers that impose or remove features that people
> want/don't want, I thought the point of open-source browsers is to prevent
> that from happening.

You can get the Firefox source code here. https://developer.mozilla.org/en/Download_Mozilla_Source_Code Because we use an open source software license, you are free to take the code, modify it, and distribute it yourself. That is the point of open source software: to give you the right to fork if you are unhappy with how it is being developed.
Adding an about:config pref to re-enable this feature is currently disabled in bug 680302.
Err... I meant discussed.
dev-doc-needed: Changes to nsIWebNavigation.idl
(In reply to Johan from comment #65)
> Adding an about:config pref to re-enable this feature is currently disabled
> in bug 680302.

May I ask what is the setting name? (or what to look for under about:config ?)
That bug isn't fixed, so there isn't a pref at the moment.
Keywords: csec-spoof
Keywords: sec-want
Whiteboard: Developers: Use the developer console (Ctrl+K) instead → Developers: Use the developer console (Ctrl+Shift+K) instead
Depends on: 885733
Sorry for the dumb question but now that the status shows:

Status: 	RESOLVED FIXED 

what is the solution?
(previously it was mentioned that it would be a setting - if so what is its name?)
Bug 680302 (NOT this bug) is still open.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #42)
> javascript: is not actually ignored - they're run, but in an "empty" context
> that doesn't have any of the usual DOM methods you would expect, so most
> common uses (e.g. javascript:alert(1)) just throw (and thus are effectively
> ignored). javascript:1+1 works fine, though.

This executes native JavaScript code:
 javascript:this.window='<script>alert(1)</script>'

and the context is the same used for data scheme.
It's just the same as |javascript:'<script>alert(1)</script>'|.
Depends on: 1018154
Blocks: 1203282
See Also: → 1725626
You need to log in before you can comment on or make changes to this bug.