1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Olivier Yiptong [:oyiptong]]

This tracks adding a build flag that limits the implementation of the Remote NewTab page using RemotePageListener to Nightly and Aurora due to a privilege escalation concern with RemotePageListener.

We will implement a set of API's instead of sending messages. We'll go beyond Aurora after removing our dependency on messages.

Comment 1

[Bobby Holley (:bholley)]

To be clear, message passing itself is not inherently a problem. The specific problems are:

(1) Running unprivileged content in an <iframe> of a privileged document.
(2) That RemotePageManager has not been audited or hardened to interact with untrusted code.

We could certainly fix (2), but my impression was that we weren't doing that because it wasn't what we were planning to ship anyway. My concern was that we were pressing ahead with shipping the interim solution, which may not have had proper security auditing.

Comment 2

[Olivier Yiptong [:oyiptong]]

Ok. Sounds good. We are fixing both, in bug 1218996 and bug 1218998.

Comment 3

[Olivier Yiptong [:oyiptong]]

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

Comment 4

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

Comment 5

[Olivier Yiptong [:oyiptong]]

A try-server build with the RELEASE_BUILD flag set can be found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ade074496609

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

https://reviewboard.mozilla.org/r/25401/#review22947

I built a release build locally, and it seemed to behave properly, and I could not access about:remote-newtab despite setting the pref, so that bit works.

I found a few places where you can use AppConstants. Otherwise, this looks great. Thanks for doing this! :D

::: browser/app/profile/firefox.js:1350
(Diff revision 1)
> -// activates the remote-hosted newtab page
> +// if true, it activates the remote-hosted newtab page
> +#ifndef RELEASE_BUILD
>  pref("browser.newtabpage.remote", false);
> +#endif

Best to move the comment within the ifdef

::: browser/components/nsBrowserGlue.js:29
(Diff revision 1)
> +#ifndef RELEASE_BUILD

```
if (!AppConstants.RELEASE_BUILD) {
...
}
```

::: browser/components/nsBrowserGlue.js:854
(Diff revision 1)
> +#ifndef RELEASE_BUILD

```
if (!AppConstants.RELEASE_BUILD) {
...
}
```

::: browser/components/nsBrowserGlue.js:1180
(Diff revision 1)
> +#ifndef RELEASE_BUILD

```
if (!AppConstants.RELEASE_BUILD) {
...
}
```

::: browser/components/nsBrowserGlue.js:2573
(Diff revision 1)
> +#ifndef RELEASE_BUILD

```
if (!AppConstants.RELEASE_BUILD) {
...
}
```

Comment 7

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/25401/#review22947

> Best to move the comment within the ifdef

Was just following what other ifdefs were doing. I don't have any strong feelings either way.

> ```
> if (!AppConstants.RELEASE_BUILD) {
> ...
> }
> ```

There is extensive use of ifdef in nsBrowserGlue.js and it does get in the list of pre-processed JS files. I'm following what other pieces of code is doing.
AppConstants is actually only used once:

https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/bin/browser/components/nsBrowserGlue.js#893

I would err on the side of using ifndef. Doubly so since this is a file which runs on startup.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Olivier Yiptong [:oyiptong] from comment #7)
> https://reviewboard.mozilla.org/r/25401/#review22947
> 
> > Best to move the comment within the ifdef
> 
> Was just following what other ifdefs were doing. I don't have any strong
> feelings either way.

Without doing that, the comment in the final output after pre-processing just kinda floats around not being applied to anything, which is kinda weird. Folks don't normally look at processed files, but they've started showing up in DXR, which is why I mention it.

> 
> > ```
> > if (!AppConstants.RELEASE_BUILD) {
> > ...
> > }
> > ```
> 
> There is extensive use of ifdef in nsBrowserGlue.js and it does get in the
> list of pre-processed JS files. I'm following what other pieces of code is
> doing.
> AppConstants is actually only used once:
> 
> https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/
> dist/bin/browser/components/nsBrowserGlue.js#893
> 
> I would err on the side of using ifndef. Doubly so since this is a file
> which runs on startup.

We've only recently (last couple of months?) introduced AppConstants, as a way of (slowly) moving our JS over to something that can be linted properly[1]. The reason you're probably seeing only one usage here is because we just haven't gotten around to introducing AppConstants in that file.

But we should avoid, if we can, introducing more pre-processor directives.

If this truly results in some kind of startup regression, I'm willing to relent.

[1]: https://mail.mozilla.org/pipermail/firefox-dev/2015-February/002689.html

Comment 9

[Olivier Yiptong [:oyiptong]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> We've only recently (last couple of months?) introduced AppConstants, as a
> way of (slowly) moving our JS over to something that can be linted
> properly[1]. The reason you're probably seeing only one usage here is
> because we just haven't gotten around to introducing AppConstants in that
> file.
> 
> But we should avoid, if we can, introducing more pre-processor directives.
> 
> If this truly results in some kind of startup regression, I'm willing to
> relent.
> 
> [1]: https://mail.mozilla.org/pipermail/firefox-dev/2015-February/002689.html

Got it. Makes sense. I'll add the AppConstants checks.

Comment 10

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99c72a85615d

Comment 11

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e7f37c9219

Comment 12

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25401/diff/1-2/

Comment 13

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

https://reviewboard.mozilla.org/r/25573/#review23131

Thanks!

Comment 15

[Olivier Yiptong [:oyiptong]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87a6c6accf1161b5086d648944ec77e873a0494
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/f69fd164a031deb79cfe3cc6fb914047364ab5ec
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r=mconley

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d87a6c6accf1
https://hg.mozilla.org/mozilla-central/rev/f69fd164a031

Comment 17

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for nightly: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f69fd164a031&selectedJob=17567943. testing triggering a release build and the tests/code didn't execute. tried a try-server release build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e01057d707c
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none

Comment 18

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for nightly: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f69fd164a031&selectedJob=17567943. testing triggering a release build and the tests/code didn't execute. tried a try-server release build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e01057d707c
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none

Comment 19

[Olivier Yiptong [:oyiptong]]

Note: Both of these patches need to be applied for a build to be successful.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

This has been on Nightly for a few days so seems safe. The patch is also removing code until the right mitigation is put into place. Let's uplift to Aurora44.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

Aurora44+

Comment 22

[Carsten Book [:Tomcat]]

this has problems during uplift to aurora:

warning: conflicts while merging browser/app/profile/firefox.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/about/AboutRedirector.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/newtab/moz.build! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/nsBrowserGlue.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue


could you take a look? Thanks

Comment 23

[Olivier Yiptong [:oyiptong]]

Ok I'll take a look

Comment 24

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b1994914f9

Comment 25

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25401/diff/2-3/

Comment 26

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25573/diff/1-2/

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Hey oyiptong - I don't think you need r+ for a straight rebase, which this looks like it was (the interdiff shows no actual changes to your patch). Can you confirm?

Comment 28

[Olivier Yiptong [:oyiptong]]

Ah, there were some (minor) changes. Perhaps reviewboard isn't showing them?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

I'm fine reviewing a Splinter patch, in that case.

Comment 30

[Olivier Yiptong [:oyiptong]]

Attachment: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora

Comment 31

[Olivier Yiptong [:oyiptong]]

Attachment: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8694233 [details] [diff] [review]
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora

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

So I manually compared this patch against the one posted in MozReview, and I think they're equivalent, which is probably why MozReview showed an empty interdiff. Or am I somehow missing some fiddling you had to do during the rebase?

::: browser/components/nsBrowserGlue.js
@@ +846,5 @@
>      NewTabUtils.init();
>      NewTabUtils.links.addProvider(DirectoryLinksProvider);
>      AboutNewTab.init();
>  
> +    if(!AppConstants.RELEASE_BUILD) {

Space after if. I guess I missed this when I reviewed the Nightly patch. :/

@@ +1171,5 @@
>  
>      CustomizationTabPreloader.uninit();
>      WebappManager.uninit();
>  
> +    if(!AppConstants.RELEASE_BUILD) {

Space after if

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8694234 [details] [diff] [review]
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js

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

Same as above.

Comment 34

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8688754 [details]
MozReview Request: Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora r?mconley

Removing uplift request, as the patch was made for m-c and the repos have diverged. Flagging aurora-specific bug instead

Comment 35

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8689360 [details]
MozReview Request: Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js r?mconley

Removing uplift request, as the patch was made for m-c and the repos have diverged. Flagging aurora-specific bug instead

Comment 36

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8694233 [details] [diff] [review]
Bug 1224950 - Limit Message-Using Remote New Tab to Nightly and Aurora

Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b1994914f9
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none

Comment 37

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8694234 [details] [diff] [review]
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js

Approval Request Comment
[Feature/regressing bug #]: Prevent message-using remote newtab to Nightly and Aurora
[User impact if declined]: Potential privilege escalation issue with the usage of RemotePageManager in about:remote-newtab
[Describe test coverage new/current, TreeHerder]: build looks fine for aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b1994914f9
[Risks and why]: low risk. this is about removing code from releases. worst case scenario is that the build is broken
[String/UUID change made/needed]:none

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Hey oyiptong - pretty sure you got approvals on the equivalent patches. You just rebased them. I don't think you need further approval. Pretty sure you're good to go here.

Comment 39

[Olivier Yiptong [:oyiptong]]

awesome!

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8694234 [details] [diff] [review]
Bug 1224950 - Remove redundant RemoteNewTabUtils use in content/browser.js

rebased. aurora44+

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3228f68ffc38
https://hg.mozilla.org/releases/mozilla-aurora/rev/54819a857281

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3228f68ffc38
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/54819a857281

Comment 43

[[e-mail]]

Not only was the NewTabUtils deprecation notice from bug 1204983 not mentioned on the Firefox 44 for developers page but there's no mention of this removal there either...

Comment 44

[Olivier Yiptong [:oyiptong]]

It was an oversight on my part. Please check bug 1240559, comment 10 for a solution for 44 and 45.
NewTabURL.jsm is back in 46 without a deprecation notice.

Comment 45

[[e-mail]]

Thanks for the update.