Page MenuHomePhabricator

Security Readiness Review For Toolhub
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project:
The Toolhub project is an effort to catalog the various software tools used in the Wikimedia movement. It seeks to fulfill the core desires expressed by T115650: Create an authoritative and well promoted catalog of Wikimedia tools by providing a storage database, HTTP API, and user interface for collecting, creating, displaying, and searching 'toolinfo' records describing various tools invented by, maintained by, or simply useful to the Wikimedia community.

Description of how the tool will be used at WMF:
Toolhub is expected to be deployed into the 'production' Kubernetes cluster with connections to database and other supporting services hosted on Wikimedia Foundation infrastructure. This service will then be exposed as https://toolhub.wikimedia.org/ to the general public. The app will execute periodic jobs to populate its database with a combination of externally sourced toolinfo.json data and locally maintained data. The app also exposes a HTTP API and local OAuth provider which can be used by 3rd parties (for example Toolforge tool maintainers) to build tools which interact with the Toolhub managed data. This API is used by the SPA frontend delivered as a core feature of Toolhub.

Dependencies

  • Python 3.7.x
  • Django 2.2.x
  • Various python libraries documented in the '[tool.poetry.dependencies]' section of the project's pyproject.toml config file
  • NodeJS 10.x
  • Vue.js 2.x
  • Vuetify 2.x
  • Various npm libraries documented in the 'dependencies' section of the project's package.json config file
  • MySQL/MariaDB
  • Memcached
  • Redis
  • Elasticsearch
  • MediaWiki OAuth 2.0

Has this project been reviewed before?
No

Working test environment
Toolhub's development environment is designed to allow you to test and run
everything from inside of a collection of Docker containers managed with
Docker Compose. The git repo contains a Makefile in its root directory that
should make working with docker-compose easier.

$ git clone "https://gerrit.wikimedia.org/r/wikimedia/toolhub"
$ cd toolhub
$ make init

n.b. There is a webpack build step that occurs within the nodejs_1 container. This may take up to 5 minutes to complete under certain circumstance. So if you see a webpack-related error when attempting to access localhost:8000, you likely just need to wait a few minutes. As noted by @bd808 below, the status of this process can be tracked by running make tail after make init completes.

Post-deployment
Developer-Advocacy will continue to own the project following deployment to the Wikimedia Foundation's 'production' Kubernetes cluster with @bd808 as the primary contact.

Details

Author Affiliation
WMF Technology Dept

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 Transcript

Toolhub is currently under active development with many remaining features to be implemented prior to our expected production deployment in June 2021. I wanted to get this request for review filed sooner rather than later however as deploying during the Foundation's fiscal year 2020/2021 is a named goal in the annual plans of the Developer-Advocacy team and Technology department.

For scheduling, I would ideally like to have a target launch date in late June 2021, and then work backwards from that to pick a date that feels comfortable for the Security-Team to do the review and give a short window for remediation prior to launch. The more time that the Toolhub team has to add features before launch the better, but of course we need to balance that with this important milestone.

Hi @bd808 - thank you for submitting this early! We will review in our call tomorrow and be in touch with any questions or concerns we have about getting you in queue.

Jcross triaged this task as Medium priority.Feb 2 2021, 4:56 PM
Jcross moved this task from Incoming to Q1: 2021 Planning Queue on the secscrum board.

@bd808 we have placed you in queue for next quarter, with work expected to begin in May for a late June launch date. Please note that all relevant code should be in a production-ready state (close to deployment with low volatility) to maintain this estimated timeline per our SOP: https://www.mediawiki.org/wiki/Security/SOP/Security_Readiness_Reviews

Please feel free to contact us with any questions, and we will be in touch as your review date draws closer.

Hello @bd808-

I and one of our vendors are having an issue building and running the docker-based development environment as described within the task description:

$ git clone "https://gerrit.wikimedia.org/r/wikimedia/toolhub"
$ cd toolhub
$ make init

I can get the make init to complete successfully, but when I try to access the web app at http://localhost:8000, I see the following error:

OSError at /
Error reading /srv/app/vue/dist/webpack-stats.json. Are you sure webpack has generated the file and the path is correct?
Request Method:	GET
Request URL:	http://localhost:8000/
Django Version:	2.2.17
Exception Type:	OSError
Exception Value:	
Error reading /srv/app/vue/dist/webpack-stats.json. Are you sure webpack has generated the file and the path is correct?
Exception Location:	/opt/lib/poetry/toolhub-2uZo5AhP-py3.7/lib/python3.7/site-packages/webpack_loader/loader.py in load_assets, line 31
Python Executable:	/opt/lib/poetry/toolhub-2uZo5AhP-py3.7/bin/python3
Python Version:	3.7.3
Python Path:	
['/srv/app',
 '/usr/lib/python37.zip',
 '/usr/lib/python3.7',
 '/usr/lib/python3.7/lib-dynload',
 '/opt/lib/poetry/toolhub-2uZo5AhP-py3.7/lib/python3.7/site-packages']

Am I missing something here? Some additional env config or build steps maybe? This was on MacOS Mojave 10.14.6 with Docker Engine v20.10.5.

Am I missing something here? Some additional env config or build steps maybe? This was on MacOS Mojave 10.14.6 with Docker Engine v20.10.5.

Try running make tail (or make init tail for first run) and wait until you see something like "nodejs_1 | DONE Compiled successfully in 526ms" in the logs showing that the initial webpack compile step has finished. This probably takes ~5 minutes after first starting the stack.

I will try spinning up a clean instance of the dev environment on my personal laptop too to see if I can recreate some bootstrapping problem that may have crept into the system since I last threw out all of my local state on my work laptop.

@bd808 - Ah, the "wait a few minutes" trick seems to have worked, thanks! I'll add a note to the task description above.

Hey @bd808 - I know you had mentioned on IRC that your team was deciding on whether to pursue another round of new features for Toolhub this quarter or to just freeze the code for a bit. Were you closer to making a decision? The Security-Team tentatively has this review scheduled for this quarter (Q4 2021) but we can bump it out if you decide to pursue more features this quarter. Thanks.

sbassett changed the task status from Open to Stalled.Apr 6 2021, 9:01 PM

Hey @bd808 - I know you had mentioned on IRC that your team was deciding on whether to pursue another round of new features for Toolhub this quarter or to just freeze the code for a bit. Were you closer to making a decision? The Security-Team tentatively has this review scheduled for this quarter (Q4 2021) but we can bump it out if you decide to pursue more features this quarter. Thanks.

@Bmueller and I had additional discussions today, but have not reached a firm decision on the need to either reduce the planned feature set or add more time before our expected deploy. We will be continuing the discussion this week, and should reach an agreement by the end of US west coast day 2021-04-09 at the latest.

To make sure everyone's expectations are clear, either way we will be adding additional code features prior to security review readiness. The debate is if we stop at adding the patrolling workflow which is still to be implemented, or continue on to also implement list management and annotations. My current expectation is that we would be able to go pencils down for a review to begin in mid-May if we decide that the deployment date is of higher importance than the list of features.

@bd808 - sounds good, we'll await an update from your team. We'll also plan to keep this in our Q4 column for now, with the assumption that we'll complete the review towards the end of this quarter. If the project gets pushed back to Q1 or Q2, that shouldn't be a problem.

@bd808 - sounds good, we'll await an update from your team. We'll also plan to keep this in our Q4 column for now, with the assumption that we'll complete the review towards the end of this quarter. If the project gets pushed back to Q1 or Q2, that shouldn't be a problem.

I do not have this all written up in nice words for betterworks yet, but here is our new (and I think much more realistic) plan:

  • Continue feature development work on Toolhub for the entire April-June 2021 quarter
  • Enter code freeze no later than 2021-07-02 to allow for this security review and a hoped for design review to occur in July 2021.
    • We will keep the project team busy in July working on end user documentation and other activities outside of the reviewable code base.
  • Plan for a production deployment in early August following reviews and remediation.

Ok, thanks, @bd808. I'll backlog this review for now with the intention of scheduling its completion for Q1 2021.

Q1 2021 ended last month; I guess that was a typo?

Q1 2021 ended last month; I guess that was a typo?

I'm pretty certain that was Q3 2021, as the Foundation's fiscal year begins anew each July 1st. So any "first" quarter must begin on July 1st. This can be further confirmed by the dates referenced within recent financial regulatory reports, as they detail reported finances through June 30th each year.

@sbassett: Thanks for the clarification. I didn't see a reference to some "Financial Year" previously; it's helpful to make such things explicit for readers in public communication to avoid misunderstandings.

Ok, thanks, @bd808. I'll backlog this review for now with the intention of scheduling its completion for Q1 2021.

@sbassett, I'm trying to figure out when to code freeze for your team's review. I'm in that "fun" position where I now know that there is no way we will get all the features we want into the initial release. I want to get as much in as we can, but not at the expense of pushing the initial release further into August.

Is there a reasonable way for us all to come to a rough schedule for the review? Knowing when the review can start based on availability of reviewers and a reasonable guess about how long the reviewers will need the main branch frozen for would help me figure out what other things Srishti and I can work on while that happens. It will also help me communicate with others about the follow on dependencies that will be unlocked by the security review.

@bd808 - Our quarterly security review planning session (per the SOP) will be next Thursday, July 1st, 2021, where @Reedy, @Mstyles and I will review incoming and back-ordered review requests and schedule them for the upcoming quarter. We were definitely going to schedule this review for the upcoming quarter and we can likely be flexible regarding when you'd like said review delivered. For a review like this, it would probably take the Security-Team at least 2 to 3 weeks to turn it around, so as long as we had that lead time, we'd be happy to work with you and your team on whatever estimated completion date works best.

@bd808 - Our quarterly security review planning session (per the SOP) will be next Thursday, July 1st, 2021, where @Reedy, @Mstyles and I will review incoming and back-ordered review requests and schedule them for the upcoming quarter.

Thanks for this additional clarification @sbassett. @Bmueller and I talked and realized that ideally we would like to deploy Toolhub on or before Thursday 2021-08-12 so that the app is available to the community during Wikimania. @srishakatux and I then did some "work backwards from the target date" thinking to see if that would be remotely possible. If we assume the worst case 3 weeks for review plus 1 week for remediation and re-validation a Thursday 2021-07-15 start date would be the latest that the security review should start.

We would be happy to jointly agree with your team on a git hash to be reviewed on Monday 2021-07-12. The tree at that point would include all major user facing features desired for the 1.0 launch, but likely would not include all of the bits needed for the Kubernetes deployment like monitoring endpoints and helm charts.

We would be happy to jointly agree with your team on a git hash to be reviewed on Monday 2021-07-12. The tree at that point would include all major user facing features desired for the 1.0 launch, but likely would not include all of the bits needed for the Kubernetes deployment like monitoring endpoints and helm charts.

Sounds good. We should be able to accommodate this proposed start date for the review and confirm as much after our planning session this Thursday.

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

I'll be running with this review for now, though it might become a bit of a group review.

  • Estimated start date: Thursday 2021-07-15.
  • Estimated completion date: 2021-08-12.
  • Code freeze commit hash to be provided by: Monday 2021-07-12.
sbassett changed the task status from Stalled to Open.Jul 1 2021, 6:16 PM
  • Code freeze commit hash to be provided by: Monday 2021-07-12.

I have tagged rWTHUd9e475d1ff13: api: Add endpoints for favorite tools as v0.1.0 for this purpose.

@bd808 - just an update here: I'd like to have this review wrapped up by tomorrow, 2021-08-06. But in all honesty, it will likely stretch into Monday, 2021-08-09. I will absolutely plan to have it completed by Monday, 2021-08-09, EOB CST.

Security Review Summary - T273020 - 2021-08-09
Last commit reviewed: d9e475d1ff13

Summary

Overall, the current Toolhub code looks reasonably secure with issues described below. While I did not perform an exhaustive manual analysis of the code, I assume that most general security best practices were followed during development, including specific security recommendations from both Django and VueJS. I would currently rate the overall risk of this application as: Medium. See a summary of the WMF's risk management policy on officewiki, or a very brief public summary here: T249039#6309061.

Vulnerable Packages - Production - node/js

None: as verified with auditjs, snyk and npm audit. Still, I'd note that these dependencies add approximately 557,753 lines of additional code to Toolhub's codebase, thus dramatically increasing complexity and potential risk. And with dev dependencies, that figure becomes approximately 5,473,383 lines of code. Risk: Low.

Vulnerable Packages - Production - python

The following vulnerabilities were found for installed packages via python's safety db. Risk: Medium.

PackageAffected Version(s)Installed VersionVulnerability
cryptography<3.33.2.1Cryptography 3.3 no longer allows loading of finite field Diffie-Hellman parameters of less than 512 bits in length. This change is to conform with an upcoming OpenSSL release that no longer supports smaller sizes. These keys were already wildly insecure and should not have been used in any application outside of testing.
cryptography<3.3.23.2.1In the cryptography package before 3.3.2 for Python, certain sequences of update calls to symmetrically encrypt multi-GB values could result in an integer overflow and buffer overflow, as demonstrated by the Fernet class. See: CVE-2020-36242.
django==2.2.172.2.17Django 2.2.18 fixes a security issue with severity "low" in 2.2.17 (CVE-2021-3281).
django>=2.0.0a1,<2.2.242.2.17Django 2.2.24, 3.1.12, and 3.2.4 includes a fix for CVE-2021-33203: Django before 2.2.24, 3.x before 3.1.12, and 3.2.x before 3.2.4 has a potential directory traversal via django.contrib.admindocs. Staff members could use the TemplateDetailView view to check the existence of arbitrary files. Additionally, if (and only if) the default admindocs templates have been customized by application developers to also show file contents, then not only the existence but also the file contents would have been exposed. In other words, there is directory traversal outside of the template root directories.
django>=2.2,<2.2.182.2.17In Django 2.2 before 2.2.18, 3.0 before 3.0.12, and 3.1 before 3.1.6, the django.utils.archive.extract method (used by "startapp --template" and "startproject --template") allows directory traversal via an archive with absolute paths or relative paths with dot segments. See CVE-2021-3281.
django>=2.2,<2.2.212.2.17In Django 2.2 before 2.2.21, 3.1 before 3.1.9, and 3.2 before 3.2.1, MultiPartParser, UploadedFile, and FieldFile allowed directory traversal via uploaded files with suitably crafted file names.
django>=2.2.0a1,<2.2.242.2.17Django 2.2.24, 3.1.12, and 3.2.4 includes a fix for CVE-2021-33571: In Django 2.2 before 2.2.24, 3.x before 3.1.12, and 3.2 before 3.2.4, URLValidator, validate_ipv4_address, and validate_ipv46_address do not prohibit leading zero characters in octal literals. This may allow a bypass of access control that is based on IP addresses. (validate_ipv4_address and validate_ipv46_address are unaffected with Python 3.9.5+).
django>=2.2a1,<2.2.202.2.17In Django 2.2 before 2.2.20, 3.0 before 3.0.14, and 3.1 before 3.1.8, MultiPartParser allowed directory traversal via uploaded files with suitably crafted file names. Built-in upload handlers were not affected by this vulnerability.
django>=2.2a1,<2.2.222.2.17In Django 2.2 before 2.2.22, 3.1 before 3.1.10, and 3.2 before 3.2.2 (with Python 3.9.5+), URLValidator does not prohibit newlines and tabs (unless the URLField form field is used). If an application uses values with newlines in an HTTP response, header injection can occur. Django itself is unaffected because HttpResponse prohibits newlines in HTTP headers.
pyyaml<5.45.3.1A vulnerability was discovered in the PyYAML library in versions before 5.4, where it is susceptible to arbitrary code execution when it processes untrusted YAML files through the full_load method or with the FullLoader loader. Applications that use the library to process untrusted input may be vulnerable to this flaw. This flaw allows an attacker to execute arbitrary code on the system by abusing the python/object/new constructor. This flaw is due to an incomplete fix for CVE-2020-1747.
urllib3>=1.26.0,<1.26.41.26.2Urllib3 1.26.4 includes a fix for CVE-2021-28363: The urllib3 library 1.26.x before 1.26.4 for Python omits SSL certificate validation in some cases involving HTTPS to HTTPS proxies. The initial connection to the HTTPS proxy (if an SSLContext isn't given via proxy_config) doesn't verify the hostname of the certificate. This means certificates for different servers that still validate properly with the default urllib3 SSLContext will be silently accepted.

Vulnerable Packages - Development - node/jst

npm audit found several development dependency vulnerabilites, 147 to be exact. They break down as 142 moderate and 5 high from 2,667 scanned packages. According to the tool, npm audit fix can be used to automatically upgrade the vast majority to secure versions, while 14 require manual review. While development dependency vulnerabilities typically pose a substantially smaller risk than those found within production dependencies, the risk is not zero, especially for development tools used to build production artifacts like vue-cli-service. Just scanning the results, I'd note that a large volume of these appear to be for the @vue/cli-service dependency, so bumping that to a more recent version (if feasible) would substantially reduce this risk. For now, this will be rated as a Medium Risk.

Outdated Packages - node/js
As reported via npm outdated:
(No explicit vulnerabilities reported, simply noting for completeness' sake.)
Risk: None.

PackageCurrentWantedLatest
@casl/abilityMISSING5.3.15.3.1
@casl/vueMISSING1.2.22.1.1
@wikimedia/language-data1.0.01.0.31.0.3
banana-i18n2.0.02.2.02.2.0
core-jsMISSING3.16.13.16.1
swagger-client3.12.23.15.03.15.0
vue-fragMISSING1.1.51.1.5
vue-i18n8.22.48.25.08.25.0
vue-metaMISSING2.4.02.4.0
vue-router3.4.93.5.23.5.2
vuetify2.4.32.5.82.5.8
vuex3.6.03.6.23.6.2
@wikimedia/jsonschema-tools0.9.00.9.00.10.4
chart.js2.9.42.9.43.5.0
stylelint-config-wikimedia0.10.30.10.30.11.1
vue2.6.122.6.142.6.14
vue-template-compiler2.6.122.6.142.6.14
webpack-bundle-tracker0.4.30.4.31.2.0

Security Headers
A few security headers are not being served, at least when I run the docker-compose environment locally. Probably the most important missing header is HSTS, and possibly CSP, though the latter likely isn't necessary for Toolhub. Risk: Low.

SAST Findings

For the application's python/django code, I ran the bandit (default rule set) and semgrep (django, python, ci, security-audit, generic, contrib rule sets) static analysis tools. A few findings resulted from these scans, most seemingly low- or no risk. The following result is the only one I found potentially worthy of review:

FileLine(s)IssueRisk
apps/crawler/tasks.py168By default, 'requests' calls wait until the connection is closed. This means a 'requests' call without a timeout will hang the program if a response is never received. Consider setting a timeout for all 'requests'. Medium

For the application's node/js/vue code, I ran njsscan (default rule set) and semgrep (javascript, ci, security-audit rule, generic, contrib sets) static analysis tools. The following are the only results I found potentially worthy of review:

FileLine(s)IssueRisk
vue/templates/vue/base.html20Translated strings will not be escaped when rendered in a template. This leads to a vulnerability where translators could include malicious script tags in their translations. Consider using force_escape to explicitly escape a transalted text. Medium
vue/templates/vue/main.html9Translated strings will not be escaped when rendered in a template. This leads to a vulnerability where translators could include malicious script tags in their translations. Consider using force_escape to explicitly escape a transalted text. Medium

Additionally, this issue was surfaced, which is obviously only a problem if the secret were to be exposed publicly while also being used within a production capacity:

FileLine(s)IssueRisk
bin/make_env.sh21Generic Secret detected High

There were also a few Docker-related recommendations from semgrep's generic/security-audit rule sets, but those are outside the scope of an application security review. I'd be happy to provide these as a supplemental paste though, if that was desired.

DAST Findings
I was not able to run any common FOSS DAST tooling (Zap, Arachni, Wapiti et al) against Toolhub at this time, but I may try to perform such scans in the near future, time permitting. Given the issues found and the overall state of the code, I do not personally believe such scans are of critical importance at this time.

Build/Test steps
Currently, the code uses vue-cli-services to build its dist artifacts, which by default uses certain webpack dependencies. Given the complexity and known code quality issues of webpack, this will be categorized as a Medium Risk. I'm not certain of the end destination of Toolhub, but if it is to be hosted within "Wikimedia production", then the Security-Team (@Reedy, @Mstyles or myself) should likely be apprised of any webpack-related builds that go through gerrit, e.g. https://gerrit.wikimedia.org/r/641052 and https://gerrit.wikimedia.org/r/708629. Otherwise, the current recommended mitigation is to use vite/rollup as an alternative to webpack (e.g. T276366).

DoS Vectors
I was not able to complete any exhaustive performance-testing for this application, but did not see anything within the code that seemed immediately concerning, save the one static analysis finding re: python requests. I would further advise having the Performance-Team conduct a performance review prior to deployment, if such applications fall within their purview.

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

Thank you for the report of results @sbassett! I have made some subtasks for follow up which are linked below in my feedback on the report.

A TL;DR version of my assessment of the report is:
Required interventions

Desired improvements

Security Review Summary - T273020 - 2021-08-09
Last commit reviewed: d9e475d1ff13

Vulnerable Packages - Production - python
The following vulnerabilities were found for installed packages via python's safety db. Risk: Medium.

Vulnerable Packages - Development - node/jst

npm audit found several development dependency vulnerabilites, 147 to be exact. They break down as 142 moderate and 5 high from 2,667 scanned packages. According to the tool, npm audit fix can be used to automatically upgrade the vast majority to secure versions, while 14 require manual review. While development dependency vulnerabilities typically pose a substantially smaller risk than those found within production dependencies, the risk is not zero, especially for development tools used to build production artifacts like vue-cli-service. Just scanning the results, I'd note that a large volume of these appear to be for the @vue/cli-service dependency, so bumping that to a more recent version (if feasible) would substantially reduce this risk. For now, this will be rated as a Medium Risk.

The Toolhub package-lock.json lists @vue/cli-service as being pinned to version 4.5.13 which appears to be the most recent 4.x release upstream at https://github.com/vuejs/vue-cli/releases.

A number of the other issues reported by npm audit have been fixed already by LibUp provided patches which have landed since the reviewed tag was set:

A fresh run of npm audit on the current HEAD reports "found 48 vulnerabilities (47 moderate, 1 high) in 2665 scanned packages". These seem largely to be related to glob-parent (https://npmjs.com/advisories/1751) and path-parse (https://npmjs.com/advisories/1773) versions pinned by other direct and indirect dependencies. I expect LibUp will sort those out for us as soon as the upstreams provide fixes.

Outdated Packages - node/js
As reported via npm outdated:
(No explicit vulnerabilities reported, simply noting for completeness' sake.)
Risk: None.

Some of these look like they should be easy to apply, but a few do need either careful testing or are known to require code changes to support upstream breaking changes. T279394: Upgrade chart.js to 3.x is one that I know of needing extra work beyond a npm update.

Security Headers
A few security headers are not being served, at least when I run the docker-compose environment locally. Probably the most important missing header is HSTS, and possibly CSP, though the latter likely isn't necessary for Toolhub. Risk: Low.

The development environment does not include TLS security at all, so setting a Strict-Transport-Security header there is not possible. I agree however that a HSTS header in the production deployment is highly desirable. I created T288557: Ensure that production deployment includes a Strict-Transport-Security header for toolhub.wikimedia.org to track that need.

A Content-Security-Policy header has been added since the reviewed git tag:

A few other header/config related security settings are gated by setting REQUIRE_HTTPS=1 in the environment for the running application:

  • http -> https redirect
  • Session cookie marked as 'secure' only
  • CSRF cookie marked as 'secure' only

SAST Findings

For the application's python/django code, I ran the bandit (default rule set) and semgrep (django, python, ci, security-audit, generic, contrib rule sets) static analysis tools. A few findings resulted from these scans, most seemingly low- or no risk. The following result is the only one I found potentially worthy of review:

FileLine(s)IssueRisk
apps/crawler/tasks.py168By default, 'requests' calls wait until the connection is closed. This means a 'requests' call without a timeout will hang the program if a response is never received. Consider setting a timeout for all 'requests'. Medium

For the application's node/js/vue code, I ran njsscan (default rule set) and semgrep (javascript, ci, security-audit rule, generic, contrib sets) static analysis tools. The following are the only results I found potentially worthy of review:

FileLine(s)IssueRisk
vue/templates/vue/base.html20Translated strings will not be escaped when rendered in a template. This leads to a vulnerability where translators could include malicious script tags in their translations. Consider using force_escape to explicitly escape a transalted text. Medium
vue/templates/vue/main.html9Translated strings will not be escaped when rendered in a template. This leads to a vulnerability where translators could include malicious script tags in their translations. Consider using force_escape to explicitly escape a transalted text. Medium

Additionally, this issue was surfaced, which is obviously only a problem if the secret were to be exposed publicly while also being used within a production capacity:

FileLine(s)IssueRisk
bin/make_env.sh21Generic Secret detected High

Making the OAuth private key for https://meta.wikimedia.org/wiki/Special:OAuthListConsumers/view/bf2b4acc4c6c7b5a34a2ef68ec071b79 public by including it in the generated development environment was a deliberate decision. It is documented in the description of the grant itself. The required callback url for the grant is http://localhost:8000/social/complete/wikimedia/ which makes it impossible to use in a production environment.

The same thing has been done with the https://meta.wikimedia.org/wiki/Special:OAuthListConsumers/view/11dec83f263af1b9f480488512556cb1 grant which is made public in the Helm chart for Toolhub. That grant is also tied to localhost (or at least custom DNS resolver) via it's use of http://toolhub.test/social/complete/wikimedia/ (.test is a globally reserved top level domain).

There were also a few Docker-related recommendations from semgrep's generic/security-audit rule sets, but those are outside the scope of an application security review. I'd be happy to provide these as a supplemental paste though, if that was desired.

I would be interested in seeing them. Some of the recommendations may be things that we judge to be of low value in a development environment, but without knowing what the recommendations are that is just speculation.

Build/Test steps
Currently, the code uses vue-cli-services to build its dist artifacts, which by default uses certain webpack dependencies. Given the complexity and known code quality issues of webpack, this will be categorized as a Medium Risk. I'm not certain of the end destination of Toolhub, but if it is to be hosted within "Wikimedia production", then the Security-Team (@Reedy, @Mstyles or myself) should likely be apprised of any webpack-related builds that go through gerrit, e.g. https://gerrit.wikimedia.org/r/641052 and https://gerrit.wikimedia.org/r/708629. Otherwise, the current recommended mitigation is to use vite/rollup as an alternative to webpack (e.g. T276366).

The application is targeted for "production" deployment, specifically as a collection of Helm managed Kubernetes pods (T280881: New Service Request Toolhub).

A webpack build is part of each commit, but not routed through a Gerrit review. The PipelineLib configuration for Toolhub publishes a new Docker image to https://docker-registry.wikimedia.org/ as a post-merge step for each Gerrit change. This image includes the results of running npm run-script build:vue to produce javascript "bundles" which implement the Vue UI.

I have created T288551: Investigate replacing vue-cli with vite and webpack with rollup for Toolhub to track investigation of replacing vue-cli and webpack in the build chain for the Vue assets.

Thank you for the report of results @sbassett! I have made some subtasks for follow up which are linked below in my feedback on the report.

You're welcome and thanks for filing all of these tasks. I see that many are now resolved or in the process of being resolved, which will likely reduce the overall risk of this incarnation of Toolhub to low. I'll keep tracking these and let you know if I have any concerns.

Making the OAuth private key for https://meta.wikimedia.org/wiki/Special:OAuthListConsumers/view/bf2b4acc4c6c7b5a34a2ef68ec071b79 public by including it in the generated development environment was a deliberate decision.

Fair enough.

I would be interested in seeing them. Some of the recommendations may be things that we judge to be of low value in a development environment, but without knowing what the recommendations are that is just speculation.

Ok, I'll try to provide a paste of these next week.

I have created T288551: Investigate replacing vue-cli with vite and webpack with rollup for Toolhub to track investigation of replacing vue-cli and webpack in the build chain for the Vue assets.

Sounds good. From the experience of those working upon Wikidata Query Builder, I believe this should be a fairly painless process that will drastically reduce the volume and risk surrounding numerous npm dependencies, etc.

Sorry this was a bit late, but here's the paste of the Dockerfile recommendations from various semgrep rule sets (namely general and ci): P17070. Most of these seem less security-related and more Dockerfile best practices IMO. I tried to make this output a bit more human-friendly, but it's still kind of messy. Also, these were run against the more recent commit b9109e99b8.

{P17070}

Comments on the warnings:

  • rule:security.semgrep-rules.generic.dockerfile.best-practice.missing-image-version -- linter complaint about not using an explict image tag and instead falling back to the default "latest" which makes repeatable builds difficult. Using exact version tags for all images would increase our ability to repeat and verify builds after the fact, but would also necessitate version bumps to be propagated downwards to all dependent builds when we rebuilt an upstream container to apply security patches. For now I am inclined to leave this alone.
  • rule:security.semgrep-rules.generic.dockerfile.best-practice.missing-no-install-recommends -- linter complaint about possible image bloat due to recommended package installation. Would require fixing in Blubber which is actually generating the Dockerfile contents. This seems like a reasonable thing to do, but it could also break existing containers on rebuilt as it would potentially alter the installed package list. Tracked as T289596: Consider adding `--no-install-recommends` to generated `apt-get install ...` commands. Turns out that this is a false positive as long as Wikimedia base images are used. Our base images include config in /etc/apt/apt.conf.d that handles this for all apt-get invocations.
  • rule:security.semgrep-rules.generic.dockerfile.best-practice.remove-package-cache -- Another false positive when using Wikimedia base images which contain a /etc/apt/apt.conf.d/docker-clean file that cleans caches via Post-Invoke directives. (Does this come from debimgbuilder/builder.py or somewhere else?)
  • rule:security.semgrep-rules.generic.dockerfile.best-practice.set-pipefail -- Appears to be triggered by RUN (getent group "<GID>" || groupadd -o -g "<GID>" -r "<USER>") statements generated by Blubber. This seems to be a false positive as this is a logical OR and not a pipeline.
  • rule:security.semgrep-rules.generic.dockerfile.correctness.alias-must-be-unique -- False positive. The aliases used in the Dockerfile output are unique (prep and prep-nodejs). Feels likely to be a parsing bug in the linter that is splitting prep-nodejs into multiple tokens, but I have not looked in the source to verify.

@sbassett would you like to re-check any of the things that I have attempted to fix before Toolhub is deployed into production? I have just done my first deployment into the staging cluster and hope to move to production in the week of 2021-09-13 pending resolution of T288832: Content license for user supplied Toolhub data.

@sbassett would you like to re-check any of the things that I have attempted to fix before Toolhub is deployed into production?

I can, sure. I assume HEAD on master is fine to look at? Or is there a better deployment tag/branch? I trust that everything within the closed subtasks for this review is in there and ready to go. And I'd be shocked if numerous security advisories have been released since the last review for dependencies, etc. But I can have another quick look, though similar to the still-open (and longer term IMO) subtasks, this should not block on deployment.

@sbassett would you like to re-check any of the things that I have attempted to fix before Toolhub is deployed into production?

I can, sure. I assume HEAD on master is fine to look at?

Yes, HEAD on the main branch is reviewable and the code destined for release. There may be a small amount of churn as I try and work through issues with the initial deployment into the staging cluster, but should be limited to config changes.

But I can have another quick look, though similar to the still-open (and longer term IMO) subtasks, this should not block on deployment.

Great news, thank you.

@bd808 - Ok, I've had a look up to 3adce4cc73b on main:

  1. I've confirmed that all relevant commits from certain tasks directly related to this review (T288532, T288536, T288539, T288542) are indeed on main.
  2. I re-ran various analysis tools used within the original review against main. I did not find any new vulnerabilities within the python dependencies. I did still find a few outdated npm development packages, but without explicit vulnerabilities that's more of an informational level risk (basically none). And I found two npm development packages (1, 2) with vulnerabilities, both of which are dependencies of various @vue/cli sub-packages. But mitigating these is likely constrained by @vue/cli v4.5.13 still being the latest, stable tag, and so I'm not sure there's much that we can/should do about that at this time, unless LibUp can find a way to safely force them through gerrit.

Given the above findings (or lack thereof) and the progress being made on T288551, I would now rate this code as low risk for production deployment.

@bd808 - Do you mind if we resolve this task given the long-running sub-tasks still attached? I don't mind keeping this task open for those, but if it's not a big deal, it'd be nice to resolve this task and move it on our board while keeping the sub-tasks open. Thanks.

@bd808 - Do you mind if we resolve this task given the long-running sub-tasks still attached? I don't mind keeping this task open for those, but if it's not a big deal, it'd be nice to resolve this task and move it on our board while keeping the sub-tasks open. Thanks.

Sounds good to me. Thanks for all of the feedback @sbassett.