1150862 - Make about:reader un-linkable from content

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Margaret Leibovic]

+++ This bug was initially created as a clone of Bug #1147597 +++

Splitting this off from bug 1147597, since this will involve making changes to our about page code.

From bug 1147597 comment 26:

(In reply to :Margaret Leibovic from comment #17)
> What do I need to do to make this non-linkable? I think we should also to
> that to avoid the phishing problems.

Remove URI_SAFE_FOR_UNTRUSTED_CONTENT from https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/components/about/AboutRedirector.cpp#l121.

Except then you'll run into the issue described in https://hg.mozilla.org/mozilla-central/annotate/da2f28836843/browser/components/about/AboutRedirector.cpp#l27.

We should probably fix that, which expands the scope, and probably requires some changes to nsAboutProtocolHandler.cpp's newURI logic and adding another flag to nsIAboutModule.

Gijs, would you be interested in taking this?

Comment 1

[:Margaret Leibovic]

Gijs beat me to this! I should read bugmail before filing new bugs :)

Comment 2

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

Seems like we can track "add the flag" and "use it" separately.

Comment 3

[:Gijs (away/busy until Mar 10)]

Attachment: Patch

I'm fine with splitting, though I'll note that the patch here will bitrot if we decide on another name in bug 1150703.

Comment 4

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

Comment on attachment 8588036 [details] [diff] [review]
Patch

Name seems fine to me!

Comment 5

[:Margaret Leibovic]

We should also add this for mobile:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js#62

However, we have a separate JS AboutRedirector implementation, so I'm not sure how this patch should be ported over.

Comment 6

[:Gijs (away/busy until Mar 10)]

Attachment: Patch for mobile

This should work, I think? Untested because I don't have an android build setup at the minute.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8588146 [details] [diff] [review]
Patch for mobile

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

Tested locally, this works. Thanks!

::: mobile/android/components/AboutRedirector.js
@@ +115,2 @@
>  
>      return flags | Ci.nsIAboutModule.ALLOW_SCRIPT;

Huh? Does this work properly if flags is undefined?

Comment 8

[:Gijs (away/busy until Mar 10)]

(In reply to :Margaret Leibovic from comment #7)
> ::: mobile/android/components/AboutRedirector.js
> @@ +115,2 @@
> >  
> >      return flags | Ci.nsIAboutModule.ALLOW_SCRIPT;
> 
> Huh? Does this work properly if flags is undefined?

I wondered this straight after attaching, too. It does, per my tests in the JS console, but I might as well update the patch to set flags to 0 where it's defined so that people reading after us stop wondering. :-)

Comment 9

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > ::: mobile/android/components/AboutRedirector.js
> > @@ +115,2 @@
> > >  
> > >      return flags | Ci.nsIAboutModule.ALLOW_SCRIPT;
> > 
> > Huh? Does this work properly if flags is undefined?
> 
> I wondered this straight after attaching, too. It does, per my tests in the
> JS console, but I might as well update the patch to set flags to 0 where
> it's defined so that people reading after us stop wondering. :-)

r+ :)

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Does this need to be a hidden security bug? This reads like defense in depth feature work.

Comment 11

[:Gijs (away/busy until Mar 10)]

(In reply to Al Billings [:abillings] from comment #10)
> Does this need to be a hidden security bug? This reads like defense in depth
> feature work.

Technically there is bug 1147597 comment #3 which explains how the current state of things can be abused.

Comment 12

[:Gijs (away/busy until Mar 10)]

(ie there is a real, documented way to abuse the fact that this hasn't landed yet - I don't know how that'd sec-rate, and if it that in itself is sec-low we might still want to open this up)

Comment 13

[:Gijs (away/busy until Mar 10)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/49ef1a47e042
remote:   https://hg.mozilla.org/integration/fx-team/rev/37b9fc12d132

Comment 14

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/49ef1a47e042
https://hg.mozilla.org/mozilla-central/rev/37b9fc12d132

Comment 15

[:Gijs (away/busy until Mar 10)]

Comment on attachment 8588036 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: content pages can link to reader mode which is a Bad Idea (defense in depth)
[Describe test coverage new/current, TreeHerder]: nope :-(
[Risks and why]: pretty low
[String/UUID change made/needed]: no, but needs change from bug 1150703 .

Comment 16

[:Gijs (away/busy until Mar 10)]

Comment on attachment 8588146 [details] [diff] [review]
Patch for mobile

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: content pages can link to reader mode which is a Bad Idea (defense in depth)
[Describe test coverage new/current, TreeHerder]: nope :-(
[Risks and why]: pretty low
[String/UUID change made/needed]: no, but needs change from bug 1150703 .

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8588146 [details] [diff] [review]
Patch for mobile

Should be in 38 beta 5.

Comment 18

[:Gijs (away/busy until Mar 10)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/01b4c5a22dd1
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0539704521a1

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/a5203cabcc04
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/062e49bcb2da

Comment 19

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b5-build1 (20150416143048), Aurora 39.0a2 (2015-04-19) and Nightly 40.0a1 (2015-04-19), using Ubuntu 14.04 (x64).

Per my conversation with :gijs, I tried accessing about:reader URLs using a simple testcase consisting of an HTML <a> href attribute and a JavaScript location href property. The links either don't do anything or they throw the following error in the Browser Console, e.g.:
> Error: Access to 'about:reader?url=https%3a%2f%2fwww.google.ro%2f%3fgws_rd%3dcr%2cssl%26ei%3dLZw0VdWuM8ebsgH2pIH4BA%23q%3dfirefox' from script denied