Page MenuHomePhabricator

Security Readiness Review for push notifications infrastructure
Closed, ResolvedPublic

Description

Parent task for more details T251190: Security Request For Service - Push Notifications

Project Information

Description of the tool/project:

The project consists of the following:

  • Push subscription management and notification request support in the Echo extension; and
  • A standalone service for batching requests for submission to third party push providers.

Dependencies
Various npm modules. Modules to be used in production by the service and not already present in the service template include:

Has this project been reviewed before?
No

Working test environment
This code is deployed to the Beta Cluster. A custom docker-compose setup for testing can be created if needed.

Post-deployment
The software components created in the course of this work will be maintained primarily by the Product Infrastructure team.

Related Objects

Event Timeline

Mholloway updated the task description. (Show Details)
Aklapper changed the visibility from "All Users" to "Public (No Login Required)".Mar 3 2020, 6:07 AM
Jcross added a subscriber: Jcross.

There is no code to review or RFC at this time and we are unable to move forward until they exist. At this time we can and will move forward with the minishlink/web-push review: https://phabricator.wikimedia.org/T246714

sbassett changed the task status from Open to Stalled.EditedMay 12 2020, 4:53 PM
sbassett added a subscriber: sbassett.

@Mholloway - can we resolve this task for now given that T246718 was declined and there seems to be an entirely new approach to this project (expanding Echo) that's still being discussed within an open RFC? Also, not sure how relevant T246714 and T246810 are anymore given the new approach. If those libs are no longer likely to be used, we should resolve those tasks as invalid. Thanks.

@sbassett, I don't think I'd describe putting the same code in Echo vs. a new extension as an "entirely new approach." In any case, it's been my experience that many teams (including this one) appreciate having a tech review task opened early as a placeholder to assist with planning. If it's not useful to you, feel free to close it.

@sbassett, I don't think I'd describe putting the same code in Echo vs. a new extension as an "entirely new approach." In any case, it's been my experience that many teams (including this one) appreciate having a tech review task opened early as a placeholder to assist with planning. If it's not useful to you, feel free to close it.

I think our team is fine with advanced notice and the filing of a security review task. It's just that when the project is still within an earlier phase (outstanding RFC, etc.) and a far-off deployment date which may be likely to change, it's not quite as useful to us. For our security reviews, we really need the code to be in as close to a production-ready state as possible. Apologies if this isn't immediately clear within our SOP. I'm going to move this to our back orders column for now until the project and code are a bit further along, at which point we'll want to agree upon what commit shas we should review (basically a freezing point for the code for this review).

Mholloway changed the task status from Stalled to Open.Jul 21 2020, 7:53 PM

Flipping this back to 'open' since it's no longer blocked on development.

sbassett added a project: user-sbassett.
sbassett moved this task from Back Orders to In Progress on the secscrum board.
sbassett moved this task from Backlog to In Progress on the user-sbassett board.

@Mholloway - I assume the "Relevant files/directories" for Echo mentioned within the task description are all on master? That branch doesn't appear very volatile for the time being, so I assume it's stable enough for a review.

Hi @sbassett, there's one patch I've just uploaded to Gerrit which will update the push-related code in Echo: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/619570. Yes, the rest is on master.

Additionally, we met as a team with Gilles today to discuss the performance review feedback. Pending further discussion tomorrow, I anticipate that the architecture will remain the same as it is now, but some refinements will be added on the service side.

Hi @sbassett, there's one patch I've just uploaded to Gerrit which will update the push-related code in Echo: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/619570. Yes, the rest is on master.

Ok, thanks. Looks like https://gerrit.wikimedia.org/r/619570 is maybe more experimental at this stage? At least that's what I'm getting from your -2 and comments.

Additionally, we met as a team with Gilles today to discuss the performance review feedback. Pending further discussion tomorrow, I anticipate that the architecture will remain the same as it is now, but some refinements will be added on the service side.

Sounds good.

Is there a new target deployment date for this? I've been looking at the pieces of code which are most complete and could probably have a review completed by the middle of next week if that works.

@sbassett, Very sorry for the delayed response. We were discussing internally whether the work related to T259148 (involving both Echo and the service) was a blocker for initial launch, and therefore for security readiness review. We just reached consensus that it isn't a blocker for either.

With that, I don't expect any further changes to the push-related code in Echo, and the only additional patches that will be coming in on the service will be adding metrics, so I don't anticipate that those will introduce any security concerns.

Also, since we've run into a couple of late-breaking delays in the past couple of weeks, we decided to push the target public launch date back to September 21.

Also, since we've run into a couple of late-breaking delays in the past couple of weeks, we decided to push the target public launch date back to September 21.

Ok, sounds good. I should be able to have the entire review completed by early September as long as the code bases do not become more volatile. Also is https://gerrit.wikimedia.org/r/619570 close to being merged?

Thanks, @sbassett, that sounds good. https://gerrit.wikimedia.org/r/619570 is ready to land from my perspective, and I've asked my teammates to make it a review priority.

Hi @sbassett, I wanted to check in with you on the current status of this review. There is one substantial patch for the push service which would be best to land before launching, but which we could wait on until post-launch refinements if necessary: https://gerrit.wikimedia.org/r/c/mediawiki/services/push-notifications/+/622376. (It looks large, but the number of lines of code affected is somewhat inflated by what amount to formatting changes.) I know we discussed your looking at the Echo extension first, so I just wanted to see if we still have at least a day or two to get this reviewed and merged before you move on to the push service. Thanks again.

@Mholloway - if you could get c622376 reviewed and merged this week, I could focus on the entire node service next week and hopefully complete the review by the end of next week. It would be best for me to review the code as close to what it will look like in production as possible, so having that code merged seems like a good idea.

Security Review Summary - T246712 - 2020-09-04
Last commits reviewed:

  • Echo: ce3e271f7734a742ffda0bf76c24a8e33bdba40e
  • push-notifications: 6a9c691f94aaf35c2c90c5063b541b64b018edda

Summary

Overall, the relevant pieces incorporated into Echo look good - nothing particularly egregious that I found. I'd currently give that a risk rating of low.

The push-notifications service code looks good as well, with some issues detailed below. Given some of the vulnerabilities within various production packages, this will have an overall risk rating of medium.

Vulnerable Packages - Production

VulnerabilityPackageNotesServiceRisk
Many: CVE-2012-6708, CVE-2015-9251, CVE-2019-11358, CVE-2020-11022/src/node_modules/clarinet/test/lib/jquery.jsN/Aretirejs medium
Prototype Pollutionminimist <0.2.1, >=1.0.0 <1.2.3N/Anpm low
Prototype Pollution (CVE-2019-10744)lodash <4.17.19N/Anpm low
Remote Code Execution (RCE)bunyan@1.8.12https://snyk.io/vuln/SNYK-JS-BUNYAN-573166snyk test medium
Prototype Pollutionjson-bigint@0.3.0https://snyk.io/vuln/SNYK-JS-JSONBIGINT-608659snyk test high
Prototype Pollutionnode-forge@0.7.4https://snyk.io/vuln/SNYK-JS-NODEFORGE-598677snyk test high
Prototype Pollutionlodash@4.17.15https://snyk.io/vuln/SNYK-JS-LODASH-567746snyk test medium
Regular Expression Denial of Servicems@0.7.3https://snyk.io/vuln/npm:ms:20170412snyk test low
Prototype Pollutionajv@6.12.2https://snyk.io/vuln/SNYK-JS-AJV-584908snyk test high
Prototype Pollutionlodash@4.17.15https://snyk.io/vuln/SNYK-JS-LODASH-590103snyk test high
Prototype Pollutionlodash@4.17.15https://snyk.io/vuln/SNYK-JS-LODASH-608086snyk test high
Prototype Pollutionminimist@1.2.0https://snyk.io/vuln/SNYK-JS-MINIMIST-559764snyk test medium
Regular Expression Denial of Servicewebsocket-extensions@0.1.3https://snyk.io/vuln/SNYK-JS-WEBSOCKETEXTENSIONS-570623snyk test high

Outdated Packages
As reported via npm outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)
Risk: none

PackageCurrentWantedLatest
@types/bluebird3.5.313.5.323.5.32
@types/concurrently5.2.05.2.15.2.1
@types/eslint6.8.16.8.17.2.2
@types/express4.17.64.17.84.17.8
@types/js-yaml3.12.43.12.53.12.5
@types/mocha7.0.27.0.28.0.3
@types/node13.13.613.13.1614.6.3
@typescript-eslint/eslint-plugin2.33.02.34.04.0.1
@typescript-eslint/parser2.33.02.34.04.0.1
ajv6.12.26.12.46.12.4
bunyan1.8.121.8.141.8.14
concurrently5.2.05.3.05.3.0
domino2.1.52.1.62.1.6
eslint-config-wikimedia0.10.10.10.10.17.0
eslint-plugin-jsdoc4.8.44.8.430.3.1
eslint-plugin-json1.4.01.4.02.1.2
firebase-admin8.12.18.13.09.1.1
js-yaml3.13.13.14.03.14.0
mocha5.2.05.2.08.1.3
nock12.0.312.0.313.0.4
nyc14.1.114.1.115.1.0
openapi-schema-validator3.0.33.0.37.0.1
service-runner2.8.0gitgit
sinon9.0.29.0.39.0.3
swagger-ui-dist3.25.13.32.53.32.5
ts-node8.10.18.10.29.0.0
typescript3.9.23.9.74.0.2
uuid3.4.03.4.08.3.0

TLS

  • I assume the push-notifications service will require TLS per the trend in T235411. Risk: low.

Security Headers

  • Given that push-notifications will be a private, internal, RESTful service accessed only by Wikimedia production infrastructure, security headers aren't quite as much of a concern here. But it likely wouldn't hurt to tighten the defaults. media-src, img-src and style-src could all be set to 'none', I believe. I'd also recommend adding base-uri: 'self' to the content-security-policy header, and possibly removing x-webkit-csp and x-content-security-policy since those headers are deprecated. It also wouldn't be a bad idea to enable hsts if my assumption above that this service will be forced over TLS is correct. And finally setting access-control-allow-origin to whatever the internal hostname of the service ends up being. Risk: low.

Static Analysis Findings

  • njsscan found an unvalidated regular expression on line 32 within test/utils/assert.js. This isn't much of a concern, given that 1) these are tests 2) the way it's used within features/app/spec.ts and features/info/info.ts is fine, it might be a good idea to sanitize expectedRegexString based upon expected values within the context of these tests, if possible (though regular expression sanitization can be difficult or even impossible at times). Risk: low.

Miscellanous Issues/Points of Discussion/Nits

sbassett moved this task from In Progress to Waiting on the user-sbassett board.

@sbassett, thanks again for the review. I believe that all comments have been addressed; the only remaining issue is an npm package with a low-severity vulnerability that is being pulled in by service-runner, which is probably out of scope for us here. Can we call this task resolved?

sbassett moved this task from Waiting to Our Part Is Done on the secscrum board.

@Mholloway - yes, this looks good and I'm fine with the service-runner issues since that's been a problem for some time now. Given the mitigations, I'd give this a new overall risk rating of low.