Page MenuHomePhabricator

Limit what URLs Proton can access
Closed, ResolvedPublic

Description

As discussed in T177765#4867361, Proton should not have access to the internal Wikimedia network (*.wmnet, IP addresses), and should probably only have to those external pages which are expected to be used for rendering the page (in a first approximation, Wikimedia domains only). So a web proxy or CSP injection or some other mechanism for ensuring that is needed.

Details

Related Gerrit Patches:
mediawiki/services/chromium-render/deploy : masterAdd gistcdn.githack.com to host blacklist
mediawiki/services/chromium-render/deploy : masterSet blacklist regex
mediawiki/vagrant : masterAdd domain blacklist to proton
mediawiki/services/chromium-render : masterAdd Puppeteer domain blacklist regexp

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 10 2019, 1:15 AM

Instead of creating a web proxy we can make use of Page Request Interception. This method should be much easier to maintain than hosting a proxy service.

Tgr renamed this task from Put Proton behind a web proxy to Limit what URLs Proton can access.Jan 15 2019, 8:34 AM
Tgr updated the task description. (Show Details)

Yeah, the task should have been phrased in terms of a goal, not an implementation. Fixed.

Jhernandez triaged this task as High priority.Jan 15 2019, 11:24 AM
Jhernandez added a subscriber: Jhernandez.

Should this be part of the firewall setup on the servers, rather than something on the service itself?

Is this a real problem?

Please Services advise if this is an actual problem with the deployment and how you think we should fix this.

As discussed in T177765#4867361, Proton should not have access to the internal Wikimedia network

This is actually not a problem. All the queries that the service receives contains the fixed domain and title to request. Moreover, external clients can only access it via RB, so they cannot control the domain the service receives as a param. Finally, Proton checks with RB that the title exists before requesting its contents.

@mobrovac I think what @Tgr was getting at is not about the title/url requested but if the page content that chromium will load has references to internal network resources, since chromium will load that content and the browser will try to fetch them (things like image srcs for example).

I'm not sure how exploitable that kind of vector really is, but it is something we should really double-check.

Tgr added a comment.Jan 15 2019, 8:30 PM

Yeah, this is about an attacker triggering requests from Proton by putting references to external resources in the article content (CSS, images, prefetch etc). Probably not really exploitable given that article HTML is restricted and Proton does not execute Javascript, and other methods are probably limited to GET and very restricted in what information they can return; but even so, giving attackers the ability to make requests from within the DMZ is just not something you want to do, no matter how directly exploitable it is.

Currently, the CSP for the HTML Proton will receive would be something like

content-security-policy: default-src 'none'; media-src *; img-src *; style-src http://*.wikipedia.org https://*.wikipedia.org 'unsafe-inline'; frame-ancestors 'self'

I guess we could further restrict the CSP for proton and rely on chromium for securing where it can go.

Tgr claimed this task.Jan 16 2019, 4:46 PM
Tgr added a comment.Jan 17 2019, 1:40 AM

IMO a stronger upstream CSP is nice to have but relying on just that is fragile; it is too easy to change it at the source without understanding what effect that will have on Proton. Whatever filtering is used should ideally be more self-contained than that.

Other options that were raised:

  • Set up some lightweight standalone proxy, force all requests through it (use the --proxy-server Chromium option in Puppeteer), and do the filtering there. Easy but one more moving part.
  • Use Puppeteer's request interception mode to abort non-whitelisted requests, as Piotr said above. Very easy and means URL filtering code is in the same place as the rest of Proton. The question is, how reliable is the Proton code? The have tests for request interception, so it's probably OK to rely on it not breaking without anyone noticing.
  • Inject CSP within Puppeteer? I seem to recall this being mentioned in some discussion, but at a glance it does not seem possible.

If we can do a combination of a stronger CSP (limit images/media to the wiki domain + uploads) and request interception, I think that would be pretty robust and still easy to do.

Regarding stronger CSP, we might need a bit of an investigation. We definitely do not want the CSP to vary by user agent, so we need to look how narrow we can make the CSP without breaking anything. I think wikidomain + *.wikimedia.org + wikidata (just in case) should work well. I'll look into it.

We definitely do not want the CSP to vary by user agent, so we need to look how narrow we can make the CSP without breaking anything. I think wikidomain + *.wikimedia.org + wikidata (just in case) should work well. I'll look into it.

I agree that varying CSP by UA is non-ideal – it's fragile and yet another moving part (it'd have to be implemented in Varnish, right?).

We could actually inject the CSP using the very same request/response interception mechanism in Proton: intercept all requests and test them against a whitelist, and intercept all responses and override the CSP header. This has the advantage that all related code is in one place, as @Tgr states above.

Tgr added a comment.Jan 17 2019, 5:31 PM

@phuedx as far as I could see Puppeteer does not provide a method for response interception. Some people intercept the request, then make a new request directly from the intercept handler callback and return that, but that seems a bit fragile.

Hrrm… I thought we might be able to listen to the response event. I'd assumed that the headers property was mutable but you might be right.

As an interim solution - which most probably will provide enough of security - let's provide:

  • a config variable, where we can store a regex to whitelist URL [or possible an array of regexes]
  • if the config option is set, use the Page Request Interception, if it doesn't match the regex abort the request?

Does it solve your concerns @Tgr?

Tgr added a comment.Jan 17 2019, 7:58 PM

@pmiazga yeah that sounds like the most practical approach.

LGoto added a subscriber: LGoto.

@Tgr Please move to the kanban board when you are working on this. Thanks!

Change 489101 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/vagrant@master] Add request whitelist to proton

https://gerrit.wikimedia.org/r/489101

Change 489102 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/services/chromium-render@master] Add Puppeteer URL whitelist regexp

https://gerrit.wikimedia.org/r/489102

LGoto added a comment.Feb 21 2019, 5:32 PM

Hi @Tgr this came up in the Audiences Platform Sync as a blocker to T210651 - can you give an update on where this is? Thanks!

ovasileva moved this task from Triage to Backlog on the Proton board.Feb 22 2019, 3:04 PM
Tgr added a comment.Feb 26 2019, 4:29 AM

This was blocked on me updating the patch to use a blacklist instead of a whitelist. Done now.

Change 489102 merged by Mobrovac:
[mediawiki/services/chromium-render@master] Add Puppeteer domain blacklist regexp

https://gerrit.wikimedia.org/r/489102

Proton patch is merged, need to add the blacklist to the production config + test it.

Change 489101 abandoned by Gergő Tisza:
Add domain blacklist to proton

Reason:
The corresponding proton patch was merged so I'll just squash this.

https://gerrit.wikimedia.org/r/489101

Change 494867 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/services/chromium-render/deploy@master] Set blacklist regex

https://gerrit.wikimedia.org/r/494867

Change 494867 merged by Mholloway:
[mediawiki/services/chromium-render/deploy@master] Set blacklist regex

https://gerrit.wikimedia.org/r/494867

phuedx removed a subscriber: phuedx.Mar 19 2019, 9:31 AM

@Tgr Can this be moved into To Deploy?

phuedx added a comment.Apr 5 2019, 2:57 PM

@Tgr: We'd talked about the deployment of this code having stalled out. Does this need a security review or at least an OK from Security prior to being deployed?

Tgr added a comment.Apr 7 2019, 2:54 AM

No, it just needs to be done. They can review at any time if they have the capacity, but Proton is already in production and even if the patch doesn't work it cannot make it *less* secure.

This did not go well; adding an external CSS import (tested via (1) and (2)) breaks the service: it just hangs and times out after 60 sec. (No log entries on the Proton channel; RESTBase logs the timeout, but that's not too informative).

As far as I can remember this is identical to how I tested it on Vagrant, so not quite sure how to debug it...

Change 510562 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/services/chromium-render/deploy@master] Add gistcdn.githack.com to host blacklist

https://gerrit.wikimedia.org/r/510562

Change 510562 merged by Mholloway:
[mediawiki/services/chromium-render/deploy@master] Add gistcdn.githack.com to host blacklist

https://gerrit.wikimedia.org/r/510562

Tgr added a comment.May 15 2019, 8:17 PM

This did not go well; adding an external CSS import (tested via (1) and (2)) breaks the service: it just hangs and times out after 60 sec. (No log entries on the Proton channel; RESTBase logs the timeout, but that's not too informative).

I forgot that we ended up using a whitelist instead of a blacklist (specifically .*:.*|[\d.]+|.*\.wmnet) so this is probably an unrelated bug. Also I can't reproduce it now: https://test.wikipedia.org/api/rest_v1/page/pdf/Test-T213362?new_pdf=1 works fine. Without new_pdf=1 it does time out though, so I'll just assume I got confused and tested it on Electron, not Proton. So apparently Electron hangs in the presence of @import rules, but it's about to be decommissioned, so we probably don't care.

Tgr added a comment.May 15 2019, 8:36 PM

On the other hand the external style is not applied, even though the URL filtering does not apply to it. Not sure what's going on there. https://eo.wikipedia.beta.wmflabs.org/api/rest_v1/page/pdf/Test-T213362?new_pdf=1 does work as expected, at least.

Tgr closed this task as Resolved.May 15 2019, 8:40 PM

Confirmed on beta that the patch is working as expected. Not sure why in production @import gets filtered out anyway, but it's probably not a good use of time to try debugging why something worked correctly, now that the reason it theoretically shouldn't have is fixed.

Mentioned in SAL (#wikimedia-operations) [2019-05-15T20:42:25Z] <tgr@deploy1001> Started deploy [proton/deploy@9373c42]: Add gistcdn.githack.com to host blacklist (T213362)

Mentioned in SAL (#wikimedia-operations) [2019-05-15T20:45:07Z] <tgr@deploy1001> Finished deploy [proton/deploy@9373c42]: Add gistcdn.githack.com to host blacklist (T213362) (duration: 02m 41s)