Page MenuHomePhabricator

Security review of the MobileApps service
Closed, ResolvedPublic

Description

The MobileApps service is about to enter production. It should be reviewed by the Security team.

Homepage: https://www.mediawiki.org/wiki/Wikimedia_Apps/Team/RESTBase_services_for_apps
Service request task: T105538
Repository: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/services/mobileapps

Event Timeline

mobrovac raised the priority of this task from to High.
mobrovac updated the task description. (Show Details)
mobrovac added subscribers: mobrovac, csteipp, dpatrick and 2 others.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 14 2015, 12:05 AM
csteipp moved this task from Backlog to Ready on the Security-Team board.
mobrovac moved this task from Backlog to Blocked on the Services board.Aug 14 2015, 12:06 AM
Se4598 renamed this task from Security review of the MoblieApps service to Security review of the MobileApps service.Aug 20 2015, 5:46 PM
Se4598 set Security to None.
csteipp moved this task from Ready to In Progress on the Security-Team board.Aug 31 2015, 9:49 PM

Sorry it's taken a while to get this done.

@bearND, can you catch me up on the background for why the www subdirectory is included in the deployment? Also, in production, I believe this is accessed through restbase, so it's on the main project domain, right? Or was this setup on a separate domain?

@csteipp The www folder is only used indirectly to generate the JS and CSS bundles. The output goes to the static folder.

The generated JS and CSS bundles are only used by the "mobile-html" route, which is quickly becoming obsolete. The new routes (mobile-html-sections, etc.) don't use the served JS and CSS bundles from the service. Instead they get their JS and CSS bundled within the apps.

@csteipp:
@Mholloway will be my backup when I'm out of the office, BTW.

@bearND or @Mholloway, can you let me know what the production url of this is?

@bearND or @Mholloway, can you let me know what the production url of this is?

To clarify, the service itself has no public interface. All of its routes are exposed through RESTBase

The only major issue I'm seeing is that this service is logging full headers in lib/util.js. If restbase ever forwards both cookies and xff on to the service, then the service would be handling sensitive data according to our privacy policy. It would be better to whitelist the headers you really need logged.

The service is also stripping rel=nofollow from wikitext-generated links, which gives spammers some incentive to link-spam us. You're correctly disallowing spidering in robots.txt, so google shouldn't pick it up, but it would be nice to allow those to pass through. Is that possible?

It would be nice to further restrict the CSP headers for media, img, and style with something like

media-src 'self' upload.wikimedia.org; img-src 'self' upload.wikimedia.org; style-src 'self';
csteipp moved this task from In Progress to Done on the Security-Team board.Sep 4 2015, 8:56 PM
GWicke added a subscriber: GWicke.EditedSep 4 2015, 9:14 PM

The only major issue I'm seeing is that this service is logging full headers in lib/util.js. If restbase ever forwards both cookies and xff on to the service, then the service would be handling sensitive data according to our privacy policy. It would be better to whitelist the headers you really need logged.

The only header forwarded from RB should normally be the cookie, and only for private wikis. I don't see why that should be logged, though, so +1 for whitelisting or just wholesale omitting headers.

The service is also stripping rel=nofollow from wikitext-generated links, which gives spammers some incentive to link-spam us. You're correctly disallowing spidering in robots.txt, so google shouldn't pick it up, but it would be nice to allow those to pass through. Is that possible?
It would be nice to further restrict the CSP headers for media, img, and style with something like

media-src 'self' upload.wikimedia.org; img-src 'self' upload.wikimedia.org; style-src 'self';

This service is only exposed through RESTBase. CSP headers are passed through if supplied by the backend service. If they are omitted, then relatively restrictive defaults are used. I would actually suggest removing the CSP headers in the backend service, so that we can manage public CSP headers centrally.

RESTBase uses robots.txt to suppress indexing for https://rest.wikimedia.org/, but this entry point is deprecated. The robots.txt used by the project domains needs to be updated to disallow indexing below /api/rest_v1/. A patch for this is in https://gerrit.wikimedia.org/r/236200.

This service is only exposed through RESTBase. CSP headers are passed through if supplied by the backend service. If they are omitted, then relatively restrictive defaults are used. I would actually suggest removing the CSP headers in the backend service, so that we can manage public CSP headers centrally.

It looks like this service is supplying csp headers, unless I'm missreading-- https://git.wikimedia.org/blob/mediawiki%2Fservices%2Fmobileapps.git/59eae54af71d5fed5ced17a094c1f22c80e0e2ff/app.js#L20

Yup, I think my wording wasn't as clear as it could have been. I think the main options are a) tighten the CSP headers emitted by the service, as you suggest, or b) stop emitting them & let RESTBase handle them. I have a slight preference for b), but we can always decide to unconditionally override CSP headers later, so it doesn't matter too much.

The only header forwarded from RB should normally be the cookie, and only for private wikis. I don't see why that should be logged, though, so +1 for whitelisting or just wholesale omitting headers.

This is inherited from the service-template-node . The reasoning was that particular services might have some custom, special headers (alongside X-Request-ID) they might want logged. I'm on board for allowing each service to specify the list of headers to log. Hence, this should not be dealt with in this specific service, but rather on the service-template-node level.

Note, however, that in practice this is not an issue at all. Full requests are logged at the trace level, while in production the lowest ever logging level used is info.

This service is only exposed through RESTBase. CSP headers are passed through if supplied by the backend service. If they are omitted, then relatively restrictive defaults are used. I would actually suggest removing the CSP headers in the backend service, so that we can manage public CSP headers centrally.

I'm really in favour of allowing each service to specify their own CSP headers due to the variance we can have. This is already possible by setting a configuration directive on a per-service basis in ops/puppet. If none are set, defaults are inherited from the service-template-node, which might be more strict if needed. I do agree that RESTBase providing very strict defaults is good, but I see that only as a last resort.

GWicke added a comment.EditedSep 7 2015, 7:15 PM

I do agree that RESTBase providing very strict defaults is good, but I see that only as a last resort.

It is harder to maintain CSP headers per content type / entry point across many services than doing so centrally. If we aren't careful, then we'll likely end up with a lot of duplicated effort and accidental variance. Auditing is also harder and more labor intensive if we have to keep an eye on each service individually.

So, I think there are good reasons for a blanket approach to CSPs, either by omitting them in the backend service, or by overriding them in RESTBase. So far, I am not aware of any HTML or SVG emitting end points that would want to load resources from random third party sites, so overriding all CSP headers (especially for html and svg content types) with restrictive ones in RESTBase might be an easy thing to do.

We should probably take the conversation about RESTBase vs. service template for setting CSP headers to its own discussion: T111820

Change 236200 had a related patch set uploaded (by Alex Monk):
Disallow indexing for /api/

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

Change 236200 merged by jenkins-bot:
Disallow indexing for /api/

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

Logging being addressed with T111709

@Mholloway, is there strong objection to having rel=nofollow on external links?

Mholloway added a comment.EditedSep 10 2015, 3:51 PM

@csteipp No objection from me.

ETA: I assumed this was happening in the template but on further investigation it looks like it's specific to our service:

https://github.com/wikimedia/mediawiki-services-mobileapps/blob/master/lib/transforms.js#L106

Will patch this pm.

Mholloway closed this task as Resolved.Jan 25 2016, 4:17 PM
Mholloway claimed this task.