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.
Description
Details
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | ovasileva | T181079 [GOAL] Provide an expanded reading experience by improving the ways that users can download articles of interest for later consumption | |||
Resolved | None | T181084 [EPIC] Deploy the mediawiki-services-chromium-render service (Proton) | |||
Resolved | • holger.knust | T210651 Switch all PDF render traffic to new Proton service | |||
Resolved | • Tgr | T213362 Limit what URLs Proton can access |
Event Timeline
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.
Yeah, the task should have been phrased in terms of a goal, not an implementation. Fixed.
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.
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.
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.
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.
@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 [[ https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#event-response | 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?
Change 489101 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/vagrant@master] Add request whitelist to proton
Change 489102 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/services/chromium-render@master] Add Puppeteer URL whitelist regexp
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
Change 489101 abandoned by Gergő Tisza:
Add domain blacklist to proton
Reason:
The corresponding proton patch was merged so I'll just squash this.
Change 494867 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/services/chromium-render/deploy@master] Set blacklist regex
Change 494867 merged by Mholloway:
[mediawiki/services/chromium-render/deploy@master] Set blacklist regex
@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?
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
Change 510562 merged by Mholloway:
[mediawiki/services/chromium-render/deploy@master] Add gistcdn.githack.com to host blacklist
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.
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.
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)
List of blocked URLs: https://logstash.wikimedia.org/goto/49c4504be0d68774d8ec00c0153d4465