680302 - Add a pref to control whether javascript: in urlbar inherits principal

Status: RESOLVED WONTFIX

(Firefox :: Address Bar, enhancement)

Description

[al_9x]

perhaps it could be a bitmask to differentiate typed and pasted URIs, and perhaps typed could be allowed by default, as in Chrome and IE

(In reply to Brendan Eich [:brendan] from comment #44)
> Jonas, comment 35 does not say why we didn't prevent pasting only.
> Intentionally typing javascript: is allowed by IE9 and Chrome. What was the
> exact rationale for going beyond restricting just paste, which was the
> exploit vector in the social engineering attack?
> 
> /be
https://bugzilla.mozilla.org/show_bug.cgi?id=527530#c44

Comment 1

[al_9x]

(In reply to al_9x from comment #0)
> perhaps it could be a bitmask

bitfield

Comment 2

[al_9x]

https://bugzilla.mozilla.org/show_bug.cgi?id=527530#c46

There's a third method, drag and drop.  So the pref would consist of three bits, typing, pasting, dropping, with typing allowed by default.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

maybe wontfix (we prefer not adding tons of options for any single thing) but for sure no reason to stay unconfirmed

Comment 4

[:Gavin Sharp [email: [e-mail]]]

We're not going to add a bitmask pref for this - that's overkill.

We could add a boolean that disables the prevention of javascript: URIs inheriting the context of the current page. I'm not sure that this pref would have a large enough target audience to be worth it.

Comment 5

[al_9x]

(In reply to Gavin Sharp from comment #4)
> We're not going to add a bitmask pref for this - that's overkill.
> 
> We could add a boolean that disables the prevention of javascript: URIs
> inheriting the context of the current page. I'm not sure that this pref
> would have a large enough target audience to be worth it.

The reason I proposed a bitmask is because of the following question by Brendan Eich:

https://bugzilla.mozilla.org/show_bug.cgi?id=527530#c44
> Jonas, comment 35 does not say why we didn't prevent pasting only.
> Intentionally typing javascript: is allowed by IE9 and Chrome. What was the
> exact rationale for going beyond restricting just paste, which was the
> exploit vector in the social engineering attack?

It still has not been answered, why?  It seems to make sense to treat typing differently from pasting and dropping.  The bitmask makes possible this (or any) default and also user overrides.

As to worth, keep in mind that you are taking away functionality that has been available and used since the inception (probably) of Fx.  That some can't handle it, does not justify taking it away from everyone.

Comment 6

[Marcio]

(In reply to Gavin Sharp from comment #4)
> We're not going to add a bitmask pref for this - that's overkill.
> 
> We could add a boolean that disables the prevention of javascript: URIs
> inheriting the context of the current page. I'm not sure that this pref
> would have a large enough target audience to be worth it.

Yes please, a boolean is just what we need.

Comment 7

[Andreas Gal :gal]

Attachment: patch, untested

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 558226 [details] [diff] [review]
patch

Let's not include "javascript" in the pref name - the patch affects data: the same way. browser.urlbar.allowInheritPrincipal seems like a reasonable pref name.

You can pass the value of _inheritPrincipal to loadCurrent directly rather than using the this/self (or just access it via named closure the same way url/flags are).

Comment 10

[Andreas Gal :gal]

Thanks for the fly-by. Sounds good. Will fix and hit you again in a few minutes.

Comment 11

[Andreas Gal :gal]

Attachment: patch

Comment 12

[Justin Dolske [:Dolske]]

How about we just roll this into being controlled by browser.urlbar.filter.javascript?

Thus if you enable that, everything works as before. It seems ultraspecialized for people to want to have one but not the other...

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 558234 [details] [diff] [review]
patch

>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js

Firefox-specific prefs belong in firefox.js.

Also, what's the answer to bug 527530 comment 44 that comment 0 refers to? Brendan doesn't really seem to ask for a hidden pref.

Comment 14

[Justin Dolske [:Dolske]]

I'm also curious what Jesse and Brandon think about this, given that they were for bug 656433 (well, at least Brandon was).

I'd also second comments 3 and 4 here; feels like this is veering into edgecase territory.

Comment 15

[Andreas Gal :gal]

People like using the urlbar to run javascript. Its off by default. You have to venture deep into about:config land to enable this. It will restore a feature we had for ages without hurting anyone. The patch is 6 lines now and doesn't add an extra pref. Happy to fly this past the sec team before landing.

Comment 16

[Andreas Gal :gal]

Attachment: patch

Comment 17

[al_9x]

Why should this and "filter.javascript" be conflated?  They control different aspects of urlbar behavior.  One is a filter for searches, the other a permission 
to run in the context of the current page. Wanting one does not necessarily imply wanting the other, they should be decoupled.

Comment 18

[al_9x]

(In reply to al_9x from comment #17)
> Wanting one does not necessarily
> imply wanting the other

At least for the case of allowing principle inheriting.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 558238 [details] [diff] [review]
patch

>-          function loadCurrent() {
>+          function loadCurrent(allowInheritPrincipal) {
>             let flags = Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>             // Pass LOAD_FLAGS_DISALLOW_INHERIT_OWNER to prevent any loads from
>             // inheriting the currently loaded document's principal, unless this
>             // URL is marked as safe to inherit (e.g. came from a bookmark
>             // keyword).
>-            if (!mayInheritPrincipal)
>+            if (!allowInheritPrincipal && !mayInheritPrincipal)
>               flags |= Ci.nsIWebNavigation.LOAD_FLAGS_DISALLOW_INHERIT_OWNER;
>             gBrowser.loadURIWithFlags(url, flags, null, null, postData);
>           }

You could just read the pref directly here instead of caching it...

I agree we shouldn't reuse browser.urlbar.filter.javascript. Apart from overloading being bad, this has the same problem that Gavin minused the first patch for.

Comment 20

[Jesse Ruderman]

> Also, what's the answer to bug 527530 comment 44 that comment 0 refers to?
> Brendan doesn't really seem to ask for a hidden pref.

My answer is bug 527530 comment 57.

> I'm also curious what Jesse and Brandon think about this, given that they were 
> for bug 656433 (well, at least Brandon was).

As far as "hidden prefs to undo security fixes" go, this one seems relatively innocuous.  But several things might not be obvious to power users:

* The pref applies to both javascript: and data: URLs.

* Paste exploits can pad their tail with enough spaces to *appear* to be http URLs.

* They must ensure {⌘L ⌘V Enter} does not enter muscle memory as a single action.

Comment 21

[Andreas Gal :gal]

> You could just read the pref directly here instead of caching it...

All other prefs are cached. At the very least for consistency I want to leave it like this. Also, slowing down page load of all things is not a good idea imo, even if its ever so slightly.

> 
> I agree we shouldn't reuse browser.urlbar.filter.javascript. Apart from
> overloading being bad, this has the same problem that Gavin minused the
> first patch for.

Done.

Comment 22

[Andreas Gal :gal]

Attachment: patch

Comment 23

[Brendan Eich [:brendan]]

(In reply to Jesse Ruderman from comment #20)
> > Also, what's the answer to bug 527530 comment 44 that comment 0 refers to?
> > Brendan doesn't really seem to ask for a hidden pref.
> 
> My answer is bug 527530 comment 57.

No evidence of typing javascript:... in any known attacks. That's what I thought.

The rest of the comment argues that other UX exists now and people should retrain. That's all nice but not relevant in our competitive circumstances for the reasons I give in bug 527530 comment 65.

> > I'm also curious what Jesse and Brandon think about this, given that they were 
> > for bug 656433 (well, at least Brandon was).
> 
> As far as "hidden prefs to undo security fixes" go, this one seems
> relatively innocuous.  But several things might not be obvious to power
> users:
> 
> * The pref applies to both javascript: and data: URLs.

Tomato, tomAHto. ;-)

> * Paste exploits can pad their tail with enough spaces to *appear* to be
> http URLs.

No one wants to support pasting, right?

> * They must ensure {⌘L ⌘V Enter} does not enter muscle memory as a single
> action.

I do that all the time with real URLs, but not with javascript: URLs -- again, the 15+ year old use-case (from yours truly) is a one-shot "try composing some fresh JS and see what it does" experiment. Not a "copy from a JS cheat sheet."

/be

Comment 24

[Andreas Gal :gal]

Brendan suggested to bounce this to the module owner since there seems to be substantial disagreement how this should be solved.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

I don't think there is substantial disagreement about what we should do. Jesse has minor concerns about "power users" not necessarily realizing what toggling this pref opens them up to (but overall thinks the pref is "relatively innocuous"), and dolske and I have concerns about doing work that adds complexity to this code and only benefits a very tiny minority of users (and increases the maintenance burden / testing matrix etc.). This latter concern stems primarily from frustration about being handicapped by a vocal minority (like Brendan! :P ) when it comes to making UI changes, but that's an argument for another day (prolonged debate only worsens the "time spent unwisely" aspect). Neither of those are particularly strong objections.

Typing vs. pasting differentiation is a more complicated fix, one that I think significantly alters the cost/benefit ratio; I don't think we want to go down that route.

Let's just take your patch, with a tweak to the relevant test (browser_loadDisallowInherit.js) to add test coverage for the pref-flipped case.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 558307 [details] [diff] [review]
patch

r=me with the test tweak from comment 25

Comment 27

[Andreas Gal :gal]

Looking for a volunteer to update the test for me. Due to other tasks I won't be able to get to this until after the allhands.

Comment 28

[Valadrem]

For those interested in executing javascript in urlbar, I have written an addon which completely disables flag LOAD_FLAGS_DISALLOW_INHERIT_OWNER so it may be considered a -temporary- workaround until this bug gets fixed.

You may take a look at it in
https://addons.mozilla.org/en-US/firefox/addon/inheritprincipal/

Comment 29

[Andreas van dem Helge]

Thanks for sneaking this "security" into Firefox. When analyzing websites we would often copy javscript: links from the website into the address bar to test various actions. The past several times I have personally tried this it didn't work "site issues" or something I figured. Are you serious? Why isn't there some sort of warning!? All users would benefit from it, the advanced users will know its just their paranoid browser blocking it and dumb users will maybe learn a lesson. "WARNING: You have attempted to execute potentially malicious code and it has been blocked" Throw some good PR if you wish "Firefox has protected you from yourself!"

Sure there's a console and some extension... but easier to just open Opera or Internet Explorer.

Comment 30

[Manish Goregaokar [:manishearth]]

What's the status of this bug?

Comment 31

[Peter Kasting]

I realize that since it's been years since I worked on Firefox' address bar my opinions don't count for much, but as the Chrome omnibox owner, I strongly agree with Brendan.  There shouldn't be a pref to configure this.  Firefox' address bar should simply behave like Chrome's and aim to prevent pasted XSS but not manually-typed JS URLs.  We've been shipping this in Chrome for some time now and it seems to be sufficiently effective.

Firefox' current behavior breaks something that has worked for a long time in most browsers, and still works everywhere else.  While I conceded that the developer console can be used instead, as a non-web developer I don't even know the keyboard shortcut for the console offhand, whereas I use javascript: URLs in the address bar frequently for very simple tests -- this week I was trying to do an alert("Foo") to see whether different browsers allowed selection within the text of alert boxes.  Because javascript: URLs in Firefox not only don't work, but fail completely silently, I retyped my input multiple times (suspecting typos) before realizing that it would never work.  In Chrome, by contrast, I get feedback instantly if I paste, before I've even hit enter, and can certainly figure out the issue after the fact.

There's no reason to add a pref here when other browsers demonstrate a solution that is sufficiently effective without breaking as many use cases.  Prefs also don't help users (like me) who don't know what obscure config options for Firefox exist, so they don't solve the problem sufficiently anyway.  Finally, the code to strip "javascript:" on paste is easy and results in a better UI, compared to the much harder idea of distinguishing pasted and typed input, which in the limit isn't feasible (paste + further typed editing).

Comment 32

[:Gavin Sharp [email: [e-mail]]]

We're not going to add this pref - addons are available to address this use case.

I'd be open to exploring the "strip javascript: on paste" behavior as a replacement for our current behavior, but I don't think it's a high priority.

Comment 33

[Peter Kasting]

Is there a bug on doing that exploration?

Comment 34

[Dão Gottwald [:dao]]

(In reply to Peter Kasting from comment #33)
> Is there a bug on doing that exploration?

filed bug 1018154