Closed Bug 1358248 (CVE-2017-7762) Opened 3 years ago Closed 3 years ago

Address bar spoof in reader mode

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: s.h.h.n.j.k, Assigned: Gijs)

References

Details

(Keywords: csectype-spoof, regression, sec-moderate, Whiteboard: [adv-main54+])

Attachments

(3 files)

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
Sorry, 
This is incomplete fix of https://bugzilla.mozilla.org/show_bug.cgi?id=1338867
Flags: needinfo?(gijskruitbosch+bugs)
Was this report intended to be about Firefox for Desktop or for Android?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(s.h.h.n.j.k)
Desktop.
Flags: needinfo?(s.h.h.n.j.k)
Status: UNCONFIRMED → NEW
Component: Untriaged → Reader Mode
Ever confirmed: true
Product: Firefox → Toolkit
Version: 1.0 Branch → Trunk
We should have put the exposableURI transform into reader mode and fixed desktop as well in bug 1338867.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8860639 - Flags: review?(max)
Attachment #8860639 - Flags: review?(evan)
This is a regression from bug 1173823. We shouldn't prominently display a baseURI instead of the documentURI. This patch fixes that.
Attachment #8860640 - Flags: review?(evan)
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.
Flags: sec-bounty?
Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

redirect review to Nevin, current mobile ReaderMode owner.
Attachment #8860639 - Flags: review?(max) → review?(cnevinchen)
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(cnevinchen)
Sorry I just need to wait for another bug to land to see how should we solve this bug.
Flags: needinfo?(cnevinchen)
The behavior between release and nightly is different. 53 is spoofy, mwobensmith reports nightly is not. What else changed in here?
Blocks: 1173823
Flags: needinfo?(gijskruitbosch+bugs)
(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,com@test.shhnjk.com/another.html, in-document domain header shows "google.com".
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mwobensmith)
Comment on attachment 8860640 [details] [diff] [review]
Patch to fix baseURI display

Looks good to me.

Sorry for the late review.
Attachment #8860640 - Flags: review?(evan) → review+
Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

Looks good to me. :)
Attachment #8860639 - Flags: review?(evan) → review+
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.
Flags: needinfo?(mwobensmith)
(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.
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?
Flags: needinfo?(cnevinchen)
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.
Attachment #8860639 - Flags: review?(cnevinchen) → review-
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.
Attachment #8860639 - Flags: review- → review?(cnevinchen)
Flags: sec-bounty?
Comment on attachment 8860639 [details] [diff] [review]
Patch to fix url bar display on desktop

># HG changeset patch
># User Gijs Kruitbosch <gijskruitbosch@gmail.com>
># 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:
Flags: needinfo?(cnevinchen)
Attachment #8860639 - Flags: review?(cnevinchen) → review+
Group: firefox-core-security → core-security-release
Can this ride the 55 train or should we consider it for backport?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
I'm assuming comment 28 was an implicit wontfix for esr52.
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.
(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.
Attached patch Patches for betaSplinter Review
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8868506 - Flags: review+
Attachment #8868506 - Flags: approval-mozilla-beta?
(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.
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!
Flags: qe-verify+
Flags: needinfo?(mihai.ninu)
Flags: needinfo?(brindusa.tot)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Flags: sec-bounty? → sec-bounty+
Keywords: csectype-spoof
Thanks!!!
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.
Attachment #8868506 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
Flags: needinfo?(mihai.ninu)
Whiteboard: [adv-main54+]
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.
Flags: qe-verify+
Alias: CVE-2017-7762
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.