1358248 - (CVE-2017-7762) Address bar spoof in reader mode

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

1. Go to about:reader?url=https%3A%2F%2Ftest.shhnjk.com%2Fcsp_read.php


Actual results:

address before @ is shown in address bar which could be used for phishing.


Expected results:

user information (left side of @) in url should not be shown in address bar. This is incomplete fix of https://www.mozilla.org/en-US/security/advisories/mfsa2017-10/#﷒0﷓

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

Sorry, 
This is incomplete fix of https://bugzilla.mozilla.org/show_bug.cgi?id=1338867

Comment 2

[:Gijs (he/him)]

Was this report intended to be about Firefox for Desktop or for Android?

Comment 3

[Jun (Request needinfo for comment, I ignore other updates)]

Desktop.

Comment 4

[:Gijs (he/him)]

Attachment: Patch to fix url bar display on desktop

We should have put the exposableURI transform into reader mode and fixed desktop as well in bug 1338867.

Comment 5

[:Gijs (he/him)]

Attachment: Patch to fix baseURI display

This is a regression from bug 1173823. We shouldn't prominently display a baseURI instead of the documentURI. This patch fixes that.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

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

::: mobile/android/chrome/content/browser.js
@@ +4519,5 @@
>      }
>    },
>  
>    _stripAboutReaderURL: function (originalURI) {
> +    return ReaderMode.getOriginalUrlObjectForDisplay(originalURI.spec) || originalURI;

Note that I haven't tested the android change here. It would be helpful if Max or someone else with the right Android knowledge / build setup could do this.

Comment 7

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

Reporter, do not set the sec-bounty flag yourself. If you wish to nominate bugs for bounty, please follow the bounty program instructions at https://www.mozilla.org/en-US/security/client-bug-bounty/ and email [e-mail] with your request.

Comment 8

[Max Liu [:maliu]]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

redirect review to Nevin, current mobile ReaderMode owner.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

mobile has some issue when cached reader page since we use startwith("about:reader") everywhere. Please let me check it and test for some time.

Comment 10

[:Gijs (he/him)]

(In reply to Nevin Chen [:nechen] from comment #9)
> Comment on attachment 8860639 [details] [diff] [review]
> Patch to fix url bar display on desktop
> 
> mobile has some issue when cached reader page since we use
> startwith("about:reader") everywhere. Please let me check it and test for
> some time.

Was there a question you wanted me to answer that prompted you to set needinfo? I'm not sure what you need from me right now.

Comment 11

[Jun (Request needinfo for comment, I ignore other updates)]

(In reply to Al Billings [:abillings] from comment #7)
> Reporter, do not set the sec-bounty flag yourself. If you wish to nominate
> bugs for bounty, please follow the bounty program instructions at
> https://www.mozilla.org/en-US/security/client-bug-bounty/ and email
> [e-mail] with your request.

Do I need to report browser bugs to [e-mail] if I need bounty? I didn't know that. I will send report to [e-mail] next time instead of Bugzilla.

Comment 12

[Marcia Knous [:marcia]]

Tracking 54/55 and esr52+.

Comment 13

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

(In reply to s.h.h.n.j.k from comment #11)

> Do I need to report browser bugs to [e-mail] if I need bounty? I
> didn't know that. I will send report to [e-mail] next time
> instead of Bugzilla.

Please don't email us bug reports. Log the bug here. If you email us the bug report instead of opening the bug, one of us has to open the bug for you and then you're not on the bug anywhere and cannot see updates to it.

If you want a bug considered for a bounty, email [e-mail] and tell us that you want the bug considered for a bounty. Otherwise, just because you reported a security bug doesn't mean it will be considered for the bounty program. We get a lot of reports.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Sorry I just need to wait for another bug to land to see how should we solve this bug.

Comment 15

[Daniel Veditz [:dveditz]]

The behavior between release and nightly is different. 53 is spoofy, mwobensmith reports nightly is not. What else changed in here?

Comment 16

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> The behavior between release and nightly is different. 53 is spoofy,
> mwobensmith reports nightly is not. What else changed in here?

I see the same results in today's nightly as in 53. Address bar shows https://google,[e-mail]/another.html, in-document domain header shows "google.com".

Comment 17

[Evan Tseng [:evanxd]]

Comment on attachment 8860640 [details] [diff] [review]
Patch to fix baseURI display

Looks good to me.

Sorry for the late review.

Comment 18

[Evan Tseng [:evanxd]]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

Looks good to me. :)

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

I see the opposite of comment 15. For me, only Nightly 55 is spoofy. 

The behavior I see there is what Gijs sees in comment 16.

Comment 20

[:Gijs (he/him)]

(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #19)
> I see the opposite of comment 15. For me, only Nightly 55 is spoofy. 
> 
> The behavior I see there is what Gijs sees in comment 16.

OK, I still see that behaviour also in 53... in any case, this isn't severe enough to cause a dot-release for 53. We should just get it landed and fixed in 54, and potentially the related ESR.

Comment 21

[:Gijs (he/him)]

Nevin, will you be able to review this change in the near future, or should I find another android peer to have a look at it?

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

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

I suggest to not to update the Android part. Since bug 1358946 will be landed soon.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

(In reply to Nevin Chen [:nechen] from comment #22)
> Comment on attachment 8860639 [details] [diff] [review]
> Patch to fix url bar display on desktop
> 
> Review of attachment 8860639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I suggest to not to update the Android part. Since bug 1358946 will be
> landed soon.

I don't think bug 1358946 should land in its current form, please see bug 1358946 comment 16. Please reconsider the r-. You can update android to do something separate from what is in ReaderMode.jsm if/when that is necessary, after this lands, but for the moment I don't see a convincing reason to stop the code sharing that is already there, especially given the regressions caused by the proposed patch in bug 1358946.

Comment 24

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

># HG changeset patch
># User Gijs Kruitbosch <[e-mail]>
># Date 1492869622 -3600
>#      Sat Apr 22 15:00:22 2017 +0100
># Node ID 50cff9056c43624c2638e6f4ad7d3a80508053a3
># Parent  7d85b081bfabe91dbf18a421f1eee2a32f335823
>Bug 1358248, r?maliu,evanxd
>
>MozReview-Commit-ID: 1EBZFcyvmY1
>
>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -184,19 +184,19 @@ file, You can obtain one at http://mozil
>                 break;
>               }
>               case "extension": {
>                 returnValue = action.params.content;
>                 break;
>               }
>             }
>           } else {
>-            let originalUrl = ReaderMode.getOriginalUrl(aValue);
>+            let originalUrl = ReaderMode.getOriginalUrlObjectForDisplay(aValue);
>             if (originalUrl) {
>-              returnValue = originalUrl;
>+              returnValue = originalUrl.spec;
>             }
>           }
> 
>           // Set the actiontype only if the user is not overriding actions.
>           if (action && this._pressedNoActionKeys.size == 0) {
>             this.setAttribute("actiontype", action.type);
>           } else {
>             this.removeAttribute("actiontype");
>@@ -820,26 +820,27 @@ file, You can obtain one at http://mozil
>             try {
>               uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
>             } catch (e) {}
>             if (!uri)
>               return selectedVal;
>           }
> 
>           // Avoid copying 'about:reader?url=', and always provide the original URI:
>-          let readerOriginalURL = ReaderMode.getOriginalUrl(uri.spec);
>-          if (readerOriginalURL) {
>-            uri = uriFixup.createFixupURI(readerOriginalURL, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
>+          // Reader mode ensures we call createExposableURI itself.
>+          let readerStrippedURI = ReaderMode.getOriginalUrlObjectForDisplay(uri.spec);
>+          if (readerStrippedURI) {
>+            uri = readerStrippedURI;
>+          } else {
>+            // Only copy exposable URIs
>+            try {
>+              uri = uriFixup.createExposableURI(uri);
>+            } catch (ex) {}
>           }
> 
>-          // Only copy exposable URIs
>-          try {
>-            uri = uriFixup.createExposableURI(uri);
>-          } catch (ex) {}
>-
>           // If the entire URL is selected, just use the actual loaded URI,
>           // unless we want a decoded URI, or it's a data: or javascript: URI,
>           // since those are hard to read when encoded.
>           if (inputVal == selectedVal &&
>               !uri.schemeIs("javascript") && !uri.schemeIs("data") &&
>               !Services.prefs.getBoolPref("browser.urlbar.decodeURLsOnCopy")) {
>             return uri.spec;
>           }
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js
>+++ b/mobile/android/chrome/content/browser.js
>@@ -4515,24 +4515,17 @@ Tab.prototype = {
>       // browser.contentDocument is changed to the new document we're loading
>       this.contentDocumentIsDisplayed = false;
>       this.hasTouchListener = false;
>       Services.obs.notifyObservers(this.browser, "Session:NotifyLocationChange");
>     }
>   },
> 
>   _stripAboutReaderURL: function (originalURI) {
>-    try {
>-      let strippedURL = ReaderMode.getOriginalUrl(originalURI.spec);
>-      if(strippedURL){
>-        // Continue to create exposable uri if original uri is stripped.
>-        originalURI = URIFixup.createExposableURI(Services.io.newURI(strippedURL));
>-      }
>-    } catch (ex) { }
>-    return originalURI;
>+    return ReaderMode.getOriginalUrlObjectForDisplay(originalURI.spec) || originalURI;
>   },
> 
>   // Properties used to cache security state used to update the UI
>   _state: null,
>   _hostChanged: false, // onLocationChange will flip this bit
> 
>   onSecurityChange: function(aWebProgress, aRequest, aState) {
>     // Don't need to do anything if the data we use to update the UI hasn't changed
>diff --git a/toolkit/components/reader/ReaderMode.jsm b/toolkit/components/reader/ReaderMode.jsm
>--- a/toolkit/components/reader/ReaderMode.jsm
>+++ b/toolkit/components/reader/ReaderMode.jsm
>@@ -161,16 +161,34 @@ this.ReaderMode = {
>         let uriObj = Services.io.newURI(originalUrl);
>         uriObj = Services.io.newURI("#" + outerHash, null, uriObj);
>         originalUrl = uriObj.spec;
>       } catch (ex) {}
>     }
>     return originalUrl;
>   },
> 
>+  getOriginalUrlObjectForDisplay(url) {
>+    let originalUrl = this.getOriginalUrl(url);
>+    if (originalUrl) {
>+      let uriObj;
>+      try {
>+        uriObj = Services.uriFixup.createFixupURI(originalUrl, Services.uriFixup.FIXUP_FLAG_NONE);
>+      } catch (ex) {
>+        return null;
>+      }
>+      try {
>+        return Services.uriFixup.createExposableURI(uriObj);
>+      } catch (ex) {
>+        return null;
>+      }
>+    }
>+    return null;
>+  },
>+
>   /**
>    * Decides whether or not a document is reader-able without parsing the whole thing.
>    *
>    * @param doc A document to parse.
>    * @return boolean Whether or not we should show the reader mode button.
>    */
>   isProbablyReaderable(doc) {
>     // Only care about 'real' HTML documents:

Comment 25

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e426eaa0546c421e106c47446ae4cc72c7bb110
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3de9541d95106d727a09379e7c19d53774dd398b

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/5e426eaa0546
https://hg.mozilla.org/mozilla-central/rev/3de9541d9510
https://hg.mozilla.org/mozilla-central/rev/9a806ddbcfc6

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Can this ride the 55 train or should we consider it for backport?

Comment 28

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Can this ride the 55 train or should we consider it for backport?

We should probably backport to 54. I need to collapse in the eslint fix Wes kindly added for me and fill out uplift requests. Hopefully tomorrow / Saturday, otherwise Monday. Leaving ni.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I'm assuming comment 28 was an implicit wontfix for esr52.

Comment 30

[Marcia Knous [:marcia]]

Gijs: Bug 1364056 indicates Reader view is not working properly both on Desktop and Android - can you take a look? Pascal's hint in Comment 7 led me back to this bug.

Comment 31

[:Gijs (he/him)]

(In reply to Marcia Knous [:marcia - use ni] from comment #30)
> Gijs: Bug 1364056 indicates Reader view is not working properly both on
> Desktop and Android - can you take a look? Pascal's hint in Comment 7 led me
> back to this bug.

I've commented on that bug. I'm jetlagged and have a pile of work on my plate right now. If nobody beats me to it I'll address this on Monday. Leaving ni and not requesting uplift until then.

Comment 32

[:Gijs (he/him)]

Attachment: Patches for beta

Approval Request Comment
[Feature/Bug causing the regression]: reader mode URL display in desktop location bar / bug 1173823 for the in-content URL shown
[User impact if declined]: users might be misled about what page they're looking at
[Is this code covered by automated tests?]: in a general "does it work" sense: yes. In the "are these security bugs fixed" sense: no.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Wouldn't hurt. Steps based on comment 0:

1. paste about:reader?url=https%3A%2F%2Ftest.shhnjk.com%2Fcsp_read.php into the URL bar and hit enter.

Expected: location bar shows test.shhnjk.com but does NOT show 'google,com@'
Pre-patch: location bar also shows 'google,com@'

Second expectation: at the top of the page (ie in-content), the link should NOT say "google.com"
Pre-patch: that link says "google.com".

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: It's a relatively small js-only patch. We're reorganizing some code so more of it is shared between desktop and android, to avoid hitting this problem in the future (as this bug was initially fixed only for android). This is relatively straightforward and the automated test coverage means we're relatively sure it doesn't majorly affect anything. I fixed one regression in bug 1364056, which is included in this set of patches.
[String changes made/needed]: nope

Comment 33

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)
> I'm assuming comment 28 was an implicit wontfix for esr52.

FWIW, I am on the fence about esr52. This isn't sec-high/sec-crit, but I think the in-content domain we show is the typical kind of thing that, in principle, shouldn't matter to users and isn't even a security bug (after all, untrusted page content can say whatever it likes), but is helpful to scammers in practice.

On the flip side, reader mode is pretty careful about HTML sanitization and so it would be very hard to take advantage of this inside the page, as we strip forms, scripts, frames, and a bunch of other things. Plus you have to convince someone to manually navigate to the about:reader URL.

I could be convinced either way but I'm not requesting uplift for 52 right now.

Comment 34

[Gerry Chang [:gchang]]

Hi Mihai & Brindusa,
could you help find someone to verify if this issue was fixed both in desktop & mobile on a latest Nightly build? Thanks!

Comment 35

[Brindusa Tot, Desktop QA]

I could reproduce this issue with a Nightly build from 04/26/2017, Version 55.0a1, Build ID 20170426030329 under Windows 10.

Verified as fixed with latest Nightly build 55.0a1, Build ID 20170517030204 on Windows 10 x64, Mac 10.12 and Ubuntu 16.04.

Comment 36

[Jun (Request needinfo for comment, I ignore other updates)]

Thanks!!!

Comment 37

[Gerry Chang [:gchang]]

Comment on attachment 8868506 [details] [diff] [review]
Patches for beta

Fix a security issue related to reader mode URL and was verified. Beta54+. Should be in 54 beta 10.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ce1b967c11f7

Comment 39

[u549602]

No issues present on the latest Nightly build, 55.0a1 on mobile.
This issue was tested on a Samsung Galaxy s6 Edge(Android 6.0) and on a Xiaomi mi i4 (Android 5.0.2)

Comment 40

[Ciprian Georgiu, Desktop QA]

I've reproduce this issue with an old Nightly build, following the STR from comment 32.

This bug is not reproducible anymore on 54.0 (20170605134926) under Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.