1396565 - ReaderMode parent validates URL protocols only after XHR requests are made

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P1)

Description

[Christiane Ruetten [INACTIVE]]

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.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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://)

Comment 2

[:Gijs (he/him)]

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".

Comment 3

[:Gijs (he/him)]

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...

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 5

[:Gijs (he/him)]

Attachment: 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).

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[:Gijs (he/him)]

(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.

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Yes this is fine to un-hide - I think this mostly a defense in depth change.

Comment 9

[:Gijs (he/him)]

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

Per IRC, clearing this review request.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b6c2e009dffc