Disable the javascript: protocol when it is entered in the location bar of browser in Firefox OS

VERIFIED FIXED

Status

defect
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: gkw, Assigned: baku)

Tracking

({b2g-testdriver, sec-high, unagi})

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox18+ fixed, firefox19+ fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

7 years ago
I enter:

javascript:alert("hi");

in the Firefox browser in Firefox OS and the alert shows. It does not show on desktop because bug 656433 disabled it.

We should disable it for the browser in Firefox OS too.
Reporter

Updated

7 years ago
blocking-basecamp: --- → ?
Eep.
blocking-basecamp: ? → +
Reporter

Comment 2

7 years ago
My Git commit info currently shows:

2012-10-17 12:16:24
e3efbd0411218762cf9a62278bf58ee513ff331f

Also, seeing my comments in https://b2gtestdrivers.allizom.org/comments_table - my build ID currently is:

20121018092134
Assignee

Comment 3

7 years ago
Posted patch patch 1 (obsolete) — Splinter Review
Is this good enough? A simple black list of protocols.
Attachment #674136 - Flags: review?(jones.chris.g)
Assignee

Comment 4

7 years ago
Posted patch patch 1b to Gaia (obsolete) — Splinter Review
A check was missing.
Attachment #674136 - Attachment is obsolete: true
Attachment #674136 - Flags: review?(jones.chris.g)
Attachment #674137 - Flags: review?(jones.chris.g)
Reporter

Comment 5

7 years ago
Can a test also be added for this?
Reporter

Updated

7 years ago
Flags: in-testsuite?
Reporter

Comment 6

7 years ago
I met w/ Tony f2f and he mention we should probably also block the protocols, as per mobile Fennec (I'm not sure if I spelt them correctly): file, wysiwyg, chrome and resource
Comment on attachment 674137 [details] [diff] [review]
patch 1b to Gaia

We should do this in gecko, ideally using the same code that other products do.  I don't know how to do that though.
Attachment #674137 - Flags: review?(jones.chris.g)
Attachment #674137 - Flags: feedback?(justin.lebar+bug)
Attachment #674137 - Flags: feedback?(bzbarsky)
Reporter

Comment 8

7 years ago
CC'ing Gavin - he originally wrote the patch in bug 656433.
Assignee

Comment 9

7 years ago
> We should do this in gecko, ideally using the same code that other products
> do.  I don't know how to do that though.

Can we block these protocols for any <iframe mozbrowser> ?
If not, how to detect which iframe should not allow these protocols and which should?
Comment on attachment 674137 [details] [diff] [review]
patch 1b to Gaia

The only way to do it in Gecko is to communicate to Gecko that you're doing a "url bar" load....
Attachment #674137 - Flags: feedback?(bzbarsky)
> Can we block these protocols for any <iframe mozbrowser> ?

We should be able to, yes.

Boris, can we just force LOAD_FLAGS_DISALLOW_INHERIT_OWNER for all loads inside mozbrowsers?  Or will that break something?

> Can a test also be added for this?

If we fix it in Gecko, we can add an automated test, yes.
Attachment #674137 - Attachment description: patch 1b → patch 1b to Gaia
Comment on attachment 674137 [details] [diff] [review]
patch 1b to Gaia

I agree we should fix this in Gecko if we can.
Attachment #674137 - Flags: feedback?(justin.lebar+bug) → feedback-
Assignee

Comment 13

7 years ago
Posted patch patch 2 (obsolete) — Splinter Review
Maybe this patch is "extreme". It enabled these 2 flags for any TabChild Browser:

nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
and
nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER

An other approach can be to change LoadURL() so that we share the flags that we want to use. I haven't written this code because right now LoadURL seems to be used just for browsers. I'm not confident with this code so I don't know if it's breaking something but I'm using b2g desktop and it seems working.
Attachment #674137 - Attachment is obsolete: true
Attachment #674613 - Flags: review?(justin.lebar+bug)
> Or will that break something?

That really depends on the use cases, no?  Doing that for Firefox tabs, for example, would break bookmarklets.  Do you plan to have people writing browsers that offer bookmarklets using mozbrowser?
> Doing that for Firefox tabs, for example, would break bookmarklets.  Do you plan to have people 
> writing browsers that offer bookmarklets using mozbrowser?

I don't think so, but Ben would know for sure.
(In reply to Boris Zbarsky (:bz) from comment #14)
> That really depends on the use cases, no?  Doing that for Firefox tabs, for
> example, would break bookmarklets.  Do you plan to have people writing
> browsers that offer bookmarklets using mozbrowser?

We have no plans for bookmarklets in v1 of the Gaia browser and any security issues the javascript: protocol creates would be higher priority to fix than allowing third party browsers to support them.

Please feel free to go ahead and take the nuclear option.
Comment on attachment 674613 [details] [diff] [review]
patch 2

This works, but only for out-of-process <iframe mozbrowser>'s.  I haven't kept up with the status of everything.me, but IIRC we were at some point using an in-process mozbrowser there.  So I think we should fix this for in-process mozbrowser's as well.

The fix would involve setting load flags somewhere within nsDocShell, if IsBrowserElement().  Probably in nsDocShell::InternalLoad would be sufficient.
Attachment #674613 - Flags: review?(justin.lebar+bug)
Reporter

Updated

7 years ago
OS: Mac OS X → Gonk (Firefox OS)
Assignee

Comment 18

7 years ago
Posted patch patch 3 (obsolete) — Splinter Review
Can you give me a feedback about this patch? I sent it to try. Thanks.
Attachment #674613 - Attachment is obsolete: true
Attachment #674790 - Flags: review?(justin.lebar+bug)
Comment on attachment 674790 [details] [diff] [review]
patch 3

I'd prefer to do this in just one place, rather than duplicating the code.  Is
there some reason you can't put this in nsDocShell::InternalLoad?

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -476,21 +476,34 @@ nsFrameLoader::ReallyStartLoadingInterna
>   loadInfo->SetOwner(mOwnerContent->NodePrincipal());
> 
>   nsCOMPtr<nsIURI> referrer;
>   rv = mOwnerContent->NodePrincipal()->GetURI(getter_AddRefs(referrer));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   loadInfo->SetReferrer(referrer);
> 
>+  // Default flags:
>+  int32_t flags = nsIWebNavigation::LOAD_FLAGS_NONE;
>+
>+  // Flags for mozbrowser:
>+  nsCOMPtr<nsIDOMMozBrowserFrame> mozbrowser = do_QueryInterface(mOwnerContent);
>+  if (mozbrowser) {
>+    bool isMozbrowser = false;
>+    mozbrowser->GetMozbrowser(&isMozbrowser);

You need to use nsIMozBrowserFrame::ReallyIsBrowser; this would apply the flags to any iframe which has the mozbrowser attribute, but we want to check permissions etc.

>+    if (isMozbrowser) {
>+      flags = nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP 
>+              nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER;
>+    }
>+  }
>+
>   // Kick off the load...
>   bool tmpState = mNeedsAsyncDestroy;
>   mNeedsAsyncDestroy = true;
>-  rv = mDocShell->LoadURI(mURIToLoad, loadInfo,
>-                          nsIWebNavigation::LOAD_FLAGS_NONE, false);
>+  rv = mDocShell->LoadURI(mURIToLoad, loadInfo, flags, false);
>   mNeedsAsyncDestroy = tmpState;
>   mURIToLoad = nullptr;
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return NS_OK;
> }

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
> bool
> TabChild::RecvLoadURL(const nsCString& uri)
> {
>     printf("loading %s, %d\n", uri.get(), NS_IsMainThread());
>     SetProcessNameToAppName();
> 
>     nsresult rv = mWebNav->LoadURI(NS_ConvertUTF8toUTF16(uri).get(),
>-                                   nsIWebNavigation::LOAD_FLAGS_NONE,
>+                                   nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP |
>+                                   nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER,
>                                    NULL, NULL, NULL);

TabChild knows whether it's a browser element, so please check that.
Attachment #674790 - Flags: review?(justin.lebar+bug)
Comment on attachment 674790 [details] [diff] [review]
patch 3

Review of attachment 674790 [details] [diff] [review]:
-----------------------------------------------------------------

bz should review this.

It's important that we don't disable webpages initiating navigation to javascript URLs since that would likely break the web.
Attachment #674790 - Flags: review?(bzbarsky)
Comment on attachment 674790 [details] [diff] [review]
patch 3

This looks fine assuming that all loads coming in over IPC there are directly from the UI and not via loads in named windows or whatnot.
Attachment #674790 - Flags: review?(bzbarsky) → review+
> It's important that we don't disable webpages initiating navigation to javascript URLs since that 
> would likely break the web.

Pages navigate themselves to javascript: URLs and expect that to work, really?
Comment on attachment 674790 [details] [diff] [review]
patch 3

At the very least, I would like to see a new patch with comment 19 addressed.

But I'd still like to put this somewhere closer to docshell if we can, so that we don't have to introduce this same code at every place which calls LoadURI.
Attachment #674790 - Flags: review-
> Pages navigate themselves to javascript: URLs and expect that to work, really?

All the time, yes.  They use it to run some script.  Typically like this:

  <a href="javascript:foo()">Click me</a>
I don't see how you can put it "closer to docshell" without breaking the use case from comment 24.  We really need to do this somewhere where we know the exact source of the URI we're loading.
(In reply to Boris Zbarsky (:bz) from comment #25)
> I don't see how you can put it "closer to docshell" without breaking the use
> case from comment 24.  We really need to do this somewhere where we know the
> exact source of the URI we're loading.

Sounds good, thanks.
Will this work as expected for something like:

window.open("javascript:...", "blank_");

I believe such a call should evaluate the javascript URL in the context of the opening page and then open a new window which displays the content of whatever the javascript code returned.
Assignee

Comment 28

7 years ago
I'm a bit lost... is this patch good enough or should I do something else?
> such a call should evaluate the javascript URL in the context of the opening page

It should evaluate it with the principal of the opening page, in the context of the new about:blank window.
> Will this work as expected for something like:
> 
> window.open("javascript:...", "blank_");

This /may/ work properly.  nsWindowWatcher::ProvideWindowInternal has its own call to LoadURI.

Andrea, can you write a test for this patch (and ideally, for the window.open case as well)?  See the tests in dom/browser-element/mochitest.  There's a python script in there which may alleviate some of the pain of creating these tests.
Assignee

Updated

7 years ago
Assignee: nobody → amarchesini
Assignee

Comment 32

7 years ago
Posted patch patch + test (obsolete) — Splinter Review
This new patch contains a mochitest.
Attachment #674790 - Attachment is obsolete: true
Attachment #675333 - Flags: review?(bzbarsky)
Comment on attachment 675333 [details] [diff] [review]
patch + test

This is incorrect per comment 19.  You can't use nsIDOMMozBrowserFrame for this.

In nsFrameLoader, you can call OwnerIsBrowserFrame(), which should be what you want.
Attachment #675333 - Flags: review-
Assignee

Comment 34

7 years ago
Posted patch patch + test 2 (obsolete) — Splinter Review
Right, OwnerIsBrowserFrame() does exactly what I need.
Attachment #675333 - Attachment is obsolete: true
Attachment #675333 - Flags: review?(bzbarsky)
Attachment #675360 - Flags: review?(justin.lebar+bug)
Comment on attachment 675360 [details] [diff] [review]
patch + test 2

This looks good to me.  The only place I see us calling PTab::LoadURL is from nsFrameLoader::ReallyStartLoadingInternal(), so this is effectively putting the flags in the same place for in-process and OOP, which is good.

I wish there was some way for us to test that iframeJS isn't doing anything, but I don't see how.  I guess we can rely on the fact that the test on the other iframe should take long enough to shake out any failures on iframeJS.
Attachment #675360 - Flags: review?(justin.lebar+bug)
Attachment #675360 - Flags: review?(bzbarsky)
Attachment #675360 - Flags: review+
Comment on attachment 675360 [details] [diff] [review]
patch + test 2

r=me
Attachment #675360 - Flags: review?(bzbarsky) → review+
Assignee

Comment 37

7 years ago
Thanks guys!
Attachment #675360 - Attachment is obsolete: true
Attachment #675477 - Flags: review+
Assignee

Updated

7 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b558dfe27a3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Reporter

Comment 40

7 years ago
This will likely need backporting to mozilla-aurora too.
Reporter

Comment 41

7 years ago
Setting sec-high instead of sec-want because this requires no more than normal phone-using actions and is not a new feature for security. Feel free to adjust as necessary.
Keywords: sec-wantsec-high
Reporter

Updated

7 years ago
Hardware: x86 → ARM
Reporter

Updated

7 years ago
Flags: in-testsuite? → in-testsuite+
Reporter

Comment 42

7 years ago
Setting checkin-needed for landing on aurora, since (I think) that's what B2G is building off now. Feel free to change this if this is incorrect.
Keywords: checkin-needed
Well, it would have landed on aurora if filed in the correct component...
Component: Gaia → General
Keywords: checkin-needed
Reporter

Comment 45

7 years ago
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.