1043110 - an appropriate way to prevent forbidden pages to be embedded in a widget iframe

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Junior [inactive]]

If a widget iframe navigates to pages which are not listed in widgetPages, we should prevent forbidden pages to be embedded in a widget. 

We want an appropriate way to react the forbidden navitgation.

Comment 1

[Junior [inactive]]

Attachment: WIP: test

WIP: test case
Simply set source to a invalid page

Besides, I do some refactor

Comment 2

[Junior [inactive]]

Attachment: navigation

WIP

this patch doesn't match the test WIP patch and need some polish
And causes false positive when the src is not in |widgetPages|

Need to investigate.

Comment 3

[Junior [inactive]]

Attachment: Part1: display_error_if_navigate_to_invalid_src - v1

For mozWidget, display a load error if we navigate to a page which is not
claimed in |widgetPages|.

Hello bz,
May I have your review?
Thanks.

Comment 4

[Junior [inactive]]

This test doesn't work since displayLoadError doesn't dispatch a |mozbrowsererror|. Will implement in another patch.

(In reply to Junior [:junior] from comment #1)
> Created attachment 8589513 [details] [diff] [review]
> WIP: test
> 
> WIP: test case
> Simply set source to a invalid page
> 
> Besides, I do some refactor

Comment 5

[Junior [inactive]]

Attachment: Part2: dispatch_mozbrowsererror-v1

Dispatch a mozbrowsererror event when one navigates to an invalid src

Comment 6

[Junior [inactive]]

Attachment: Part3: test-v1

Test for navigation issue on mozWidget
Wait for the review for previous parts

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8633364 [details] [diff] [review]
Part1: display_error_if_navigate_to_invalid_src - v1

Sorry for the lag here....

>+    rv = aURI->SchemeIs("about", &isAbout);
>+    if (!isAbout) {

You need to check NS_SUCCEEDED/NS_FAILED(rv) before touching isAbout.  I don't have a strong opinion as to whether failure should be treated as "about" or "not about".

>+      nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(frameElement);

This is just a complicated way of checking for an HTML <iframe>, right?  frameElement->IsHTMLElement(nsGkAtoms::iframe") is clearer as a test.  But...

We don't really want to run this code in random iframes on the web that have a "mozwidget" attribute.  I guess it would end up kinda working out because GetAppByLocalId will return null for the "no app id" case, but it's a little annoying to depend on that.  Furthermore, don't we have to pay attention to the "dom.enable_widgets" preference somewhere here?

Is there a reason that GetReallyIsWidget() does the wrong thing here?  Or can we use it?  I have to admit to not being very up on what the whole widget/browserframe setup is...

>+        aURI->GetSpec(spec);

Having to serialize the URI like that is a bit annoying, but ok.

That said, won't this likely prevent data: URI loads?  Do we want to do that, or should those succeed like about: loads succeed?

I'd like to get the actual behavior we're after nailed down here, but in general this looks pretty good.

Comment 8

[Junior [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8633364 [details] [diff] [review]
> Part1: display_error_if_navigate_to_invalid_src - v1
> 
> Sorry for the lag here....
> 
> >+    rv = aURI->SchemeIs("about", &isAbout);
> >+    if (!isAbout) {
> 
> You need to check NS_SUCCEEDED/NS_FAILED(rv) before touching isAbout.  I
> don't have a strong opinion as to whether failure should be treated as
> "about" or "not about".
> 
The reason I didn't check if |SchemeIs| success or not is we've checked in
http://lxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10531

I bailed out here since non-mozWidget case will trigger
|DisplayLoadError| -> |InternalLoad| -> |DoURILoad(about:neterror...|,
 and |about:neterror| is always not a valid src.
I need a condition to avoid recursive condition to infinitely call |DoURILoad|

Or you still want to have a check here?


> >+      nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(frameElement);
> 
> This is just a complicated way of checking for an HTML <iframe>, right? 
> frameElement->IsHTMLElement(nsGkAtoms::iframe") is clearer as a test.  But...
> 
My idea here is to identify if it's a mozBrowser iframe (i.e. <iframe mozbrowser>)

> We don't really want to run this code in random iframes on the web that have
> a "mozwidget" attribute.  I guess it would end up kinda working out because
> GetAppByLocalId will return null for the "no app id" case, but it's a little
> annoying to depend on that.  Furthermore, don't we have to pay attention to
> the "dom.enable_widgets" preference somewhere here?

My bad, I need to check permission and preference again. That would also check
if the embedder is an app or not I guess. Will check in next version.
> 
> Is there a reason that GetReallyIsWidget() does the wrong thing here?  Or
> can we use it?  I have to admit to not being very up on what the whole
> widget/browserframe setup is...

|GetReallyIsWidget| doesn't work because |DisplayLoadError| will not change src in 
|HTMLGeneralFrameElement|. Maybe I should cook out a refactor or some helper to
make things easy to use and more intuitive. I'll file a follow-up bug.

> 
> >+        aURI->GetSpec(spec);
> 
> Having to serialize the URI like that is a bit annoying, but ok.
> 
> That said, won't this likely prevent data: URI loads?  Do we want to do
> that, or should those succeed like about: loads succeed?
We like to prevent navigating to src not in |widgetPages| claimed in manifest to
avoid information leak. Briefly, |mozWidget| is proposed to be embedded in privileged app,
thus causing us to have some restriction, such as limited navigation, limited browser API.

SchemeIs('about') is only a sugar to prevent recursion. 

> 
> I'd like to get the actual behavior we're after nailed down here, but in
> general this looks pretty good.

Thanks for your review!

Comment 9

[Boris Zbarsky [:bzbarsky]]

> is we've checked in
> http://lxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10531

Nothing requires a URI implementation to consistently throw or not throw from SchemeIs.  And if it throws you're reading a random boolean.  So please do check.

> My idea here is to identify if it's a mozBrowser iframe (i.e. <iframe mozbrowser>)

Then the QI is not the right way to do it.  All <iframe> elements QI to nsIMozBrowserFrame.  You presumably need to check GetReallyIsBrowserOrApp() too, right?

> Maybe I should cook out a refactor or some helper to
> make things easy to use and more intuitive.

That would be lovely.

> We like to prevent navigating to src not in |widgetPages| claimed in manifest 

OK.  I'm fine with them; just figured I should point out that it prevents things some people might expect to work  ;)

Comment 10

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=562f26bcdc59

Comment 11

[Junior [inactive]]

Attachment: Part1: display_error_if_navigate_to_invalid_src - v2

Here I use |nsIDocShell::GetIsApp()| to check if this docshell is an app or not, and find a way to use |GetReallyIsApp| instead of copying a dup logic.

I know |GetReallyIsApp| is a little ambiguous. Maybe we should rename to |GetReallyIsAppOrWidget|.

Comment 12

[Junior [inactive]]

Attachment: Part2: dispatch_mozbrowsererror-v2

Rebase and carry r+

Comment 13

[Junior [inactive]]

Attachment: Part3: test-v2

Rebase. Wait for previous patches r+'ed

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8637113 [details] [diff] [review]
Part1: display_error_if_navigate_to_invalid_src - v2

>+ // Once navigate to an invalid mozWidget page, trigger |displayLoadError| and
>+ // load an about:neterror, which is also an invalid mozWidget page. To avoid
>+ // recursion, we skip to check if aURI is of |about:| scheme.

How about:

  When we go to display a load error for an invalid mozWidget page, we will try
  to load an about:neterror page, which is also an invalid mozWidget page. To
  avoid recursion, we skip this check if aURI's scheme is "about".

That said, I assume we also want this for about:blank, right, not just about:neterror...  and do we really want to allow other about: pages here?  Maybe it would be best to just explicitly check for about:neterror and about:blank?

>+        // widget. We want to check if the widget comes to an invalid page. (This

"We want to check whether the widget is trying to navigate to an invalid page."?

>+        if (browserFrame && !browserFrame->GetReallyIsApp()) {

So the point here is that GetReallyIsApp will return false for widgets but true for apps while nsIDocShell::GetIsApp will return true for both?  Or is the logic something else?  The comments aren't actually making it clear why this check is the right one.

r=me with those bits addressed

Comment 15

[Junior [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Comment on attachment 8637113 [details] [diff] [review]
> Part1: display_error_if_navigate_to_invalid_src - v2
> 
> >+ // Once navigate to an invalid mozWidget page, trigger |displayLoadError| and
> >+ // load an about:neterror, which is also an invalid mozWidget page. To avoid
> >+ // recursion, we skip to check if aURI is of |about:| scheme.
> 
> How about:
> 
>   When we go to display a load error for an invalid mozWidget page, we will
> try
>   to load an about:neterror page, which is also an invalid mozWidget page. To
>   avoid recursion, we skip this check if aURI's scheme is "about".
> 
> That said, I assume we also want this for about:blank, right, not just
> about:neterror...  and do we really want to allow other about: pages here? 
> Maybe it would be best to just explicitly check for about:neterror and
> about:blank?
> 

What we are afraid of is leaking information in the "mozWidget". Therefore, it
doesn't matter if we allow about:blank or not. But is there any elegant way to
check aURI is about:neterror? If we serialize the |aURI| to check if
|aURI->spec| has prefix "neterror", it's a little annoying and highly depends to
the implementation of |displayLoadError|.


> >+        // widget. We want to check if the widget comes to an invalid page. (This
> 
> "We want to check whether the widget is trying to navigate to an invalid
> page."?
> 
> >+        if (browserFrame && !browserFrame->GetReallyIsApp()) {
> 
> So the point here is that GetReallyIsApp will return false for widgets but
> true for apps while nsIDocShell::GetIsApp will return true for both?  Or is
> the logic something else?  The comments aren't actually making it clear why
> this check is the right one.
It's a bit ambiguous as I mentioned :(

|nsIDocShell::GetIsApp| is decided as the frameLoader start to load this docshell,
returning true for both mozApp and mozWidget. And this value doesn't change during
the lifecycle of the frame.

|GetReallyIsApp| delegates the check to |nsGenericHTMLFrameElement|, which will 
dynamically check the manifest/permission/preference/validity of src, returning true
iff a frame is a mozApp or mozWidget at that time.
But generally manifest/permission/preference do not change during the life time of
the frame, hence an app frame is always valid; an widget frame becomes invalid when
navigate to invalid page.

Moreover, privileged app is able to embed widget. What we really concern is leaking
the information of an "widget app" to a third-party app.

To conclude: |nsIDocShell:GetIsApp| returning true means the docshell acts as an 
app/widget. We should care about the information leakage. Therefore, we check 
|GetReallyIsApp| to see if the app is still valid, and the only case |GetReallyIsApp|
returning false is that it's a widget app and navigates to a invalid page.

Maybe I should put the bug number to the code comment. Will update a WIP for reference.

> 
> r=me with those bits addressed

Comment 16

[Junior [inactive]]

Attachment: Part1: display_error_if_navigate_to_invalid_src - v3

Update per review command, carry r+

Comment 17

[Junior [inactive]]

Attachment: dispatch_mozbrowsererror

Rebase, carry r+

Comment 18

[Junior [inactive]]

Comment on attachment 8637116 [details] [diff] [review]
Part3: test-v2

Previous patches are r+'ed, I guess it's time for a review.

Fabrice, could I have your review? Thanks.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Therefore, it doesn't matter if we allow about:blank or not.

I was thinking in terms of it possibly breaking widgets if loading about:blank is not allowed.

But if the goal is to prevent information leakage, then that should be clearly documented in this code.  In that situation, loading about:anything is probably fine.  I wish we consistently used the URI_IS_LOCAL_RESOURCE protocol flag, since that would have the right meaning here, but about: URIs don't have it set.  :(  Maybe file a followup bug about that?

In any case, with the clear documentation about what the goal is, just checking for about: is OK.  But I still think allowing loading about:blank should be part of that goal and explicitly mentioned in the comment.

> Maybe I should put the bug number to the code comment

The bug number can be found via the blame.  I would much rather the code comment summarized the intent of the code.

So is the goal to prevent both widgets _and_ apps from navigating to URIs that are not valid, not just to prevent widgets from navigating?  If so, then the code in the patch makes a lot more sense.  If the goal is to only affect widgets but not apps, then I'm not clear on what's going on here.

Comment 20

[Junior [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> > Therefore, it doesn't matter if we allow about:blank or not.
> 
> I was thinking in terms of it possibly breaking widgets if loading
> about:blank is not allowed.
> 
> But if the goal is to prevent information leakage, then that should be
> clearly documented in this code.  In that situation, loading about:anything
> is probably fine.  I wish we consistently used the URI_IS_LOCAL_RESOURCE
> protocol flag, since that would have the right meaning here, but about: URIs
> don't have it set.  :(  Maybe file a followup bug about that?

I'm not familiar with with protocol handler, bug I guess URI_DANGEROUS_TO_LOAD 
is an appropriate flag. So the question is: should "about:" protocol never hits
the network to let it be with URI_IS_LOCAL_RESOURCE? 

But on the other hands, it's hard to have a flag to check if the page is valid
for a mozWidget since the forbidden list depends on manifest and and the manifestURL, 
which is beyond the scope of "protocol handler" I guess. 
 
> 
> In any case, with the clear documentation about what the goal is, just
> checking for about: is OK.  But I still think allowing loading about:blank
> should be part of that goal and explicitly mentioned in the comment.

Sure, will try to have a clear comment for what we did now and what the intention was.
> 
> > Maybe I should put the bug number to the code comment
> 
> The bug number can be found via the blame.  I would much rather the code
> comment summarized the intent of the code.
> 
> So is the goal to prevent both widgets _and_ apps from navigating to URIs
> that are not valid, not just to prevent widgets from navigating?  If so,
> then the code in the patch makes a lot more sense.  If the goal is to only
> affect widgets but not apps, then I'm not clear on what's going on here.

Let me rephrase. The goal of this bug is to affect widgets only. However, I think
it works to generalize to apps and widgets, although this patch does not have
impact on apps _now_. If an app possibly loses its validity of app in the future,
 this patch is able to prevent the navigation.

Comment 21

[Junior [inactive]]

Attachment: Part1: display_error_if_navigate_to_invalid_src - v4

Modify comments, carry r+

Comment 22

[Boris Zbarsky [:bzbarsky]]

> bug I guess URI_DANGEROUS_TO_LOAD is an appropriate flag

URI_DANGEROUS_TO_LOAD is a security flag.  It can be set whether LOCAL_RESOURCE is set or not.

> it's hard to have a flag to check if the page is valid for a mozWidget 

We shouldn't be trying to do that.  For the actual checking for URIs we want to check for, the approach in the patch is fine; I'm just talking about the "about:" whitelisting in the patch.

> although this patch does not have
> impact on apps _now_. If an app possibly loses its validity of app in the future,
> this patch is able to prevent the navigation.

OK.  Please just make sure this is clearly documented.

Comment 23

[[:fabrice] Fabrice Desré]

Comment on attachment 8637116 [details] [diff] [review]
Part3: test-v2

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

r=me with nit addressed

::: dom/apps/tests/file_test_widget.js
@@ +42,5 @@
>    domParent.appendChild(ifr);
>  
> +  ifr.addEventListener("mozbrowserloadend", function _onloadend() {
> +    ok(true, "receive mozbrowserloadend");
> +    // ifr.removeEventListener("mozbrowserloadend", _onloadend);

remove commented out code.

Comment 24

[Junior [inactive]]

Attachment: Part1: display_error_if_navigate_to_invalid_src - v5

Rebase, carry r+

Comment 25

[Junior [inactive]]

Attachment: Part2: dispatch_mozbrowsererror-v4

Rebase, carry r+

Comment 26

[Junior [inactive]]

Attachment: Part3: test-v3

Rebase and modify by review comment, carry r+

Comment 27

[Carsten Book [:Tomcat]]

can we get a try run here ? thanks!

Comment 28

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e924f96385

Comment 29

[Junior [inactive]]

Try run looks good

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/031cc887765e
https://hg.mozilla.org/integration/mozilla-inbound/rev/623942e8b052
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2a33f119cd

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/031cc887765e
https://hg.mozilla.org/mozilla-central/rev/623942e8b052
https://hg.mozilla.org/mozilla-central/rev/fb2a33f119cd