ReaderMode parent validates URL protocols only after XHR requests are made

RESOLVED FIXED in Firefox 59

Status

()

Toolkit
Reader Mode
P1
normal
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: cr, Assigned: Gijs)

Tracking

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
ReaderMode Parent exposes XHR request GET to arbitrary URLs through its "Reader:ArticleGet" message handler. URLs are checked only on the request result to determine whether document content is to be returned to the child or not:

http://searchfox.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#235

async downloadAndParseDocument(url) {
  let doc = await this._downloadDocument(url);
  if (!this._shouldCheckUri(doc.documentURIObject) || !this._shouldCheckUri(doc.baseURIObject, true)) {
    this.log("Reader mode disabled for URI");
    return null;
  }
  return this._readerParse(doc);
},

The security risk here is that a compromised child can trigger the parent to make arbitrary GET requests (despite not always getting the answer), potentially weakening security controls like CSP and sandboxing for data exfiltration.
The sandbox doesn't provide any restrictions on same-origin or networking - it can't. By definition, the child process is a container which contains all web pages that the user loads, or will load. There is no way for the parent to know what request or link from the child is as a result or normal page load or navigation.

The one thing that we can and should enforce in the parent is ensure that the child process can't read local files. But from looking at _downloadDocument it looks like this check is being done:

http://searchfox.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#280

try {
  ssm.checkLoadURIStrWithPrincipal(doc.nodePrincipal, newURL, flags);
} catch (ex) {
  let errorMsg = "Reader mode disallowed meta refresh (reason: " + ex + ").";

It does seem out of order that the check is after the load, but I think what this is, is actually a second check, AFTER the real security check which is on line 280. This prevents reader from loading weird protocols that would be legal, but unpredictable (e.g. ftp:// or data://)
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → INVALID
Group: toolkit-core-security, mozilla-employee-confidential
CC list accessible: false
Not accessible to reporter
(Assignee)

Comment 2

9 months ago
As far as I can tell, this code should still check the URL of the request prior to initiating the XHR. It checks any <meta> redirects explicitly before making them (that's line 280 - https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/components/reader/ReaderMode.jsm#280 - it's in the load handler of the original XHR and only runs when we find a <meta redirect>!), but not the original request, which could equally be problematic or be a file: request. There are other checks in place and I don't think it's possible to do anything nefarious today, but it would be better (defense-in-depth-wise) if the protocol check came before the XHR.

Additionally, I wonder if it's possible to check the source docshell of the message and verify that the requested URI matches (if this is the initial request rather than one from a meta redirect - once we get redirected the URI won't match. We could alter how redirects get handled by reader mode but that code is pretty error-prone so I dunno if that's easy to do or not off-hand...).

The other thing I don't really get is why this is even going through the parent process in the first place. But I haven't looked too hard - been traveling all day and I need food first. :-)

Re-marking sec-sensitive to be super cautious even if I don't think this can actually be abused today. Self-ni to remind me to come back to this, if nobody else picks it up "soon".
Group: firefox-core-security
Status: RESOLVED → REOPENED
status-firefox57: affected → wontfix
status-firefox58: --- → affected
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P2
Resolution: INVALID → ---
Summary: ReaderMode parent validates URLs only after XHR requests are made → ReaderMode parent validates URL protocols only after XHR requests are made
(Assignee)

Updated

8 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Priority: P2 → P1
(Assignee)

Updated

8 months ago
status-firefox59: --- → affected
(Assignee)

Comment 3

8 months ago
The simple fix here is just adding some more guards to _downloadDocument.

However, the root cause is kind of that we use child->parent->child messaging to do the actual fetching here, which is kind of weird.

It seems the reason this goes via messaging to the parent process is that in mobile, we can sometimes get the data from the reader mode cache rather than the network. On desktop though, this seems like a useless roundtrip that defeats sandboxing. I'll see if I can find a sane way to change how this works on desktop without breaking the cache stuff on mobile...
Yeh I had assumed other checks would stop the loading of file:// uris and thats the only URI level thing we enforce with the sandbox. But definitely agree that defense in depth would be better here, I hadn't thought about the meta redirect case.
(Assignee)

Updated

8 months ago
CC list accessible: true
Accessible to reporter
(Assignee)

Comment 5

8 months ago
Created attachment 8932958 [details] [diff] [review]
Patch

This includes both a guard and moves the fetching of the article to the content process in desktop. The moving doesn't really do much (given the guard, but also (per comment 1) given that once you're chrome privileged in the content process, you can already fetch whatever you like), but it avoids sending the entire article data via IPC which seems like something we should avoid anyhow. I don't know that mobile can do something better here, given the caching, so I left it alone.

Paul/Christiane, do you think we can unhide this, or would you prefer to treat this as sec-low/sec-moderate ? AFAICT exploiting this would still require a privesc in the content process, at a minimum to run script on the about:reader page (which content can't trivially do because it runs on a separate principal and because we sanitize the HTML we render in reader mode with nsIParserUtils).
Flags: needinfo?(ptheriault)
Flags: needinfo?(cr)
Attachment #8932958 - Flags: review?(jaws)
Attachment #8932958 - Flags: review?(cr)
Comment on attachment 8932958 [details] [diff] [review]
Patch

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

::: toolkit/components/reader/AboutReader.jsm
@@ +675,5 @@
>      this._showContent(article);
>    },
>  
>    _getArticle(url) {
> +    if (this.PLATFORM_HAS_CACHE) {

Indeed if we are worried about this on Desktop then we should be equally concerned about Android. Likewise in the reverse.

But I do see the upside for the perf optimization for Desktop that doesn't benefit from caching.
Attachment #8932958 - Flags: review?(jaws) → review+
(Assignee)

Comment 7

8 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Indeed if we are worried about this on Desktop then we should be equally
> concerned about Android. Likewise in the reverse.
> 
> But I do see the upside for the perf optimization for Desktop that doesn't
> benefit from caching.

Note that technically the check in ReaderMode.jsm is sufficient to avoid a sandbox-escape style thing where we pass non-http/https things to ReaderMode.jsm in the parent. This also applies on mobile.

Further note that mobile, AIUI, doesn't have an e10s-style content process separation and thus no sandbox at all, so this issue as a sandbox escape doesn't really exist there. Once Fennec gains a sandbox, we would want to do work to move the http request into the sandbox there, but right now I don't really see any reason to do more work there.
Yes this is fine to un-hide - I think this mostly a defense in depth change.
Group: firefox-core-security
Flags: needinfo?(ptheriault)
Flags: needinfo?(cr)
(Assignee)

Comment 9

8 months ago
Comment on attachment 8932958 [details] [diff] [review]
Patch

Per IRC, clearing this review request.
Attachment #8932958 - Flags: review?(cr)

Comment 10

8 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c2e009dffc
make reader mode fetch the article from within the content process on desktop, r=jaws

Comment 11

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6c2e009dffc
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago8 months ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
status-firefox58: affected → wontfix
You need to log in before you can comment on or make changes to this bug.