Hey @Jdforrester-WMF et al -

The Security-Team would like to proceed on this work in hopes of delivering some application security review report deliverables by the end of the current quarter (December 31st, 2021). We would like to perform our standard hybrid security review against the following codebases:

1. ext:WikiLambda (doc) (@mmartorana)
2. function-orchestrator (@Mstyles)
3. function-evaluator (@sbassett)

A few notes/qualifiers:

1. These will be application security reviews and will focus primarily upon the aforementioned codebases largely in isolation, as opposed to a pen-test of the entire system and standard Wikimedia production infrastructure that SRE, Release-Engineering-Team, et al manage. If there is a desire to test at these layers in a more holistic manner, we can explore such options in the future.
2. We can also plan to review the function-schemata codebase, though I'm not sure exactly how that will be used within a production capacity or what its visibility/impact is upon the project. Also - we aren't missing any other relevant codebases, correct?
3. We should agree on some stable commit SHAs or branches to use for these reviews - whichever is easiest for the Abstract Wikipedia to manage. We'd like to review code that is as stable and as close to production as it can be, at this time.

Thanks!

Sorry for the late reply! Somehow missed this.

In T289322#7508193, @sbassett wrote:

Hey @Jdforrester-WMF et al -

The Security-Team would like to proceed on this work in hopes of delivering some application security review report deliverables by the end of the current quarter (December 31st, 2021). We would like to perform our standard hybrid security review against the following codebases:

1. ext:WikiLambda (doc) (@mmartorana)
2. function-orchestrator (@Mstyles)
3. function-evaluator (@sbassett)

A few notes/qualifiers:

1. These will be application security reviews and will focus primarily upon the aforementioned codebases largely in isolation, as opposed to a pen-test of the entire system and standard Wikimedia production infrastructure that SRE, Release-Engineering-Team, et al manage. If there is a desire to test at these layers in a more holistic manner, we can explore such options in the future.

Ack

1. We can also plan to review the function-schemata codebase, though I'm not sure exactly how that will be used within a production capacity or what its visibility/impact is upon the project.

It's used as a sub-module of the three main repos list above.

Also - we aren't missing any other relevant codebases, correct?

Correct.

1. We should agree on some stable commit SHAs or branches to use for these reviews - whichever is easiest for the Abstract Wikipedia to manage. We'd like to review code that is as stable and as close to production as it can be, at this time.

Thanks!

Sorry – is this still something that's needed?

In T289322#7620050, @Jdforrester-WMF wrote:

1. We should agree on some stable commit SHAs or branches to use for these reviews - whichever is easiest for the Abstract Wikipedia to manage. We'd like to review code that is as stable and as close to production as it can be, at this time.

Thanks!

Sorry – is this still something that's needed?

I believe @Mstyles, @mmartorana and myself have been working with recent commits on master. So if that's fairly reflective of what should make it to production soon, then I think we should be ok.

In T289322#7620059, @sbassett wrote:

In T289322#7620050, @Jdforrester-WMF wrote:

1. We should agree on some stable commit SHAs or branches to use for these reviews - whichever is easiest for the Abstract Wikipedia to manage. We'd like to review code that is as stable and as close to production as it can be, at this time.

Thanks!

Sorry – is this still something that's needed?

I believe @Mstyles, @mmartorana and myself have been working with recent commits on master. So if that's fairly reflective of what should make it to production soon, then I think we should be ok.

Excellent, thanks.

Security Review Summary - T289322 - 2022-01-13
Last commit reviewed: 36511a7

Summary

Overall, the current code under consideration...
has an overall risk rating of: low

Vulnerable Packages - Production

The majority of the vulnerabilities come from service runner, so overall I'm going to rate this as a medium risk

Vulnerable Packages - Development
none

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

Other Vulnerable Code
As found via https://semgrep.dev/, https://snyk.io/vuln:

1. Snyk
   • • Potential XSS - need to escape HTML, details
   • routes/testData.js - line 18
   • routes/spec.js - line 19
     • Path Traversal - details
   • routes/testData - line 21
   • routes/spec.js - line 22
2. Semgrep
   • • Path Traversal - details
   • app.js - line 145
     • Non Literal Require - details
   • app.js - line 151

Overview
Overall the code looks pretty good. A lot of the vulnerable packages were from service-runner
as mentioned above. The other vulnerable code has some concerns that are easily remedied.
I would suggest trying to address some of those issues. Due to no critical vulnerabilities
and the majority of the vulnerabilities that are there coming from service-runner, I'm giving
this project a risk rating of low.

Security Review Summary - T289322 - 2022-01-14
Last commit reviewed: f8bfba32ddc266ead8a8bbd134e63ec669defba9

Summary

Overall, the code looks good because there are only a couple of findings that are easy fixable. However, the security posture of the WikiLamda extension is rather concerning because it has many outdated dependencies that may pose a security risk, with an overall rating of: high.

Vulnerable Packages

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

Static Analysis Findings

Using semgrep and snyk, some low impact issues have been found.

1. Math.random() is a cryptographically weak random number generator:

Path: resources/ext.wikilambda.edit/store/modules/zImplementations.js

155:    ( Math.floor( Math.random() * 100 ) + 1 );

2. Path Traversal

Unsanitized input from data from a remote resource flows into file_get_contents, where it is used as a path. This may result in a Path Traversal vulnerability and allow an attacker to read arbitrary files:

Path: includes/Hooks.php
333:	$data = file_get_contents( $initialDataToLoadPath . $filename );

3. The external library Ace has some security warnings, mainly due to the usage of unsecure methods like innerHTML/outerHTML, Math.random(), string comparisons using unsafe operators like ===, !==, != and ==, unsafe regex, etc... These are reported only for completeness as they affect an external library.

Path: resources/lib/ace/src/ace.js

21172:    el.innerHTML = gutterAnno.text.join("<br>");
--------------------------------------------------------------------------------
2403:        this.getElement().innerHTML = html;
--------------------------------------------------------------------------------
20879:            w.el.innerHTML = w.html;
--------------------------------------------------------------------------------

Path: resources/lib/ace/src/ext-keybinding_menu.js

169:            el.innerHTML = '<h1>Keyboard Shortcuts</h1>' + commands + '</div>';

Path: resources/lib/ace/src/ext-language_tools.js

1738:            tooltipNode.innerHTML = item.docHTML;

Path: resources/lib/ace/src/ext-prompt.js

1769:            tooltipNode.innerHTML = item.docHTML;

Path: resources/lib/ace/src/ext-static_highlight.js

129:        el.innerHTML = highlighted.html;

Path: resources/lib/ace/src/ext-textarea.js

517:    settingDiv.innerHTML = table.join("");

Path: resources/lib/ace/src/keybinding-vim.js

726:      dialog.innerHTML = template;

Risk low.

Thanks! Addressing some of these dated dependencies in a patch for eslint* and one for mediawiki-wdio (and one for stylelint-config-wikimedia that's waiting on getting a new release out).

In T289322#7623442, @Jdforrester-WMF wrote:

Thanks! Addressing some of these dated dependencies in a patch for eslint* and one for mediawiki-wdio (and one for stylelint-config-wikimedia that's waiting on getting a new release out).

Hey @Jdforrester-WMF - yes, the service-runner dependencies (including the more recent T298854, for which we don't have a great solution rolled out just yet) are definitely going to increase the risk for the relevant Abstract Wikipedia repos, for now. Good to see a new release is due out that addresses some of these issues. I also call these out in my function-evaluator review, which I hope to have posted this week.

Hey @Jdforrester-WMF - apologies for the lengthy delay on this review. Please let me know if you have any questions. I also plan to briefly stop by the architecture meeting this coming Monday (2022-02-07) to report on these results and address any questions, though I may be a bit late due to a prior meeting.

Security Review Summary - T289322 - 2022-02-04
Last commit reviewed: 0bc358a8782a27fffdc4091e63bcaf546d5a3215

Summary

Overall, the the function-evaluator service appears to be in good shape with some known dependency issues and of course the inherent risk of executing arbitary code. For now, this review will assign an overall risk rating of: medium, to be accepted at the manager/director level in accordance with the current risk management framework.

Vulnerable Packages - Production

As reported by npm audit, snyk, auditjs ossi and scan, and then de-duped. These are all similar to the vulns reported above for the function-orchestrator codebase, and which are largely centered around the service-runner dependecy. Which, as mentioned above, are being somewhat addressed within open pull requests. In addition to other, related isses like T298854, which still need to be resolved.

Vulnerable Packages - Development

Non-critical dev packages are low risk.

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

TLS
I assume the orchestrator and evaluator services will be TLS-terminated via the current, standard approach. low risk.

Security Headers
While service-template-node's default csp config is likely fine, especially within the context of the evaluator service, which should never be public-facing, it might not hurt to restrict said policy further (basically default-src 'none'), especially if the orchestrator will have public-facing portions (I forget if this is part of the current architecture.) low risk.

HTTP and other protocol Leaks

1. None found via custom semgrep rules. low risk.

DoS Vectors (including ReDoS)

1. We can let the Performance-Team focus on any remaining issues within this vulnerability category within the still-open performance review: T261341. no risk, for now.

Static Analysis Findings

1. lockfile-lint 4.6.2 returned no results. low risk
2. git secrets, as run with default AWS patterns and rudimentary secret keyword rule sets, returned only false positives. low risk
3. gitleaks returned no results. low risk
4. whispers 1.5.3 returned mostly MINOR, non-issue results, and one MAJOR issue for the exec call within executors/python3/executor.py, which was found by multiple tools and is likely the largest risk within this codebase. low risk
5. scan scan:latest returned no results. low risk
6. semgrep 0.80.0 which was run with the following rule sets:
   1. p/typescript: This policy returned no results. low risk
   2. p/nodejscan: This policy returned no results. low risk
   3. p/secrets This policy returned no results. low risk
   4. p/ci: This policy returned no results. low risk
   5. p/security-audit: This policy returned no results. low risk
   6. p/supply-chain: This policy (a bidi rule) returned no results. low risk
   7. p/owsap-top-ten: This policy returned no results. low risk
   8. p/python: This policy returned no results. low risk
   9. p/r2c-ci: This policy returned no results. low risk
   10. p/insecure-transport: This policy returned no results. low risk

The following semgrep policies all generated results, which I've included as supplemental files at the end of this review.

1. p/command-injection
2. p/eslint-security-plugin
3. p/expressjs
4. p/gitlab-bandit
5. p/gitlab-eslint
6. p/javascript
7. p/nodejs
8. p/owasp-top-ten
9. p/r2c-security-audit
10. p/security-audit
11. p/xss

The general summary of the above policy results are:

1. Several javascript bracket object notation accesses with dynamic data, which I would rate as low risk.
2. A regexp which uses dynamic data to build its pattern on line 64 of app.js. This is mostly a false positive as the variable in question is coming from conf and is therefore low risk.
3. A require on line 151 of app.js which uses dynamic data. Though within the context of how this is used to load service-runner routes, I'd rate this as low risk.
4. Some Object.assign calls with dynamic data. These are used to set various response headers, which could likely be filtered better, but for which I would still say are low risk.
5. The res.send calls on lines 24 and 48 in routes/ex.js, which return an array with no elements and an empty text variable, and as noted within the code, neither should ever be reached. This is obviously low risk.

And now the serious issues, which also happen to be the primary functionality of the evaluator service:

1. The eval call within executors/javascript/executor.js
2. The exec call within executors/python3/executor.py
3. The spawn call within src/subprocess.js

This functionality has to be a medium risk for now in an effort to balance the myriad precautions being taken with the likely unknowns of the ZObject spec and Abstract Wikipedia architecture. We had rated related issues during the threat modeling exercie as critical/high risks, with a number of mitigations to be implemented (sandboxing, etc.) to reduce that risk, though given the complexity and likely high amount of uncertainty, I don't personally see a path for this to ever become low risk.

Supplemental Materials
Please see attached:

1. 2022-function-evaluator-semgrep-p-command-injection.txt1 KBDownload
2. 2022-function-evaluator-semgrep-p-eslint-security.txt977 BDownload
3. 2022-function-evaluator-semgrep-p-expressjs.txt5 KBDownload
4. 2022-function-evaluator-semgrep-p-gitlab-bandit.txt814 BDownload
5. 2022-function-evaluator-semgrep-p-gitlab-eslint.txt4 KBDownload
6. 2022-function-evaluator-semgrep-p-javascript.txt3 KBDownload
7. 2022-function-evaluator-semgrep-p-nodejs.txt3 KBDownload
8. 2022-function-evaluator-semgrep-p-owasp-top-ten.txt3 KBDownload
9. 2022-function-evaluator-semgrep-p-r2c-security-audit.txt1 KBDownload
10. 2022-function-evaluator-semgrep-p-security-audit.txt1 KBDownload
11. 2022-function-evaluator-semgrep-p-xss.txt535 BDownload

@Jdforrester-WMF, @dr0ptp4kt, @DVrandecic et al -

Just to follow up here on the the three reviews above:

1. The review for the function-orchestrator service (T289322#7621518) was found to have an overall low risk, which is automatically accepted, requires no further action and will not be added to our risk register. Of course we certainly would encourage at least a review of the found issues and the creation of a mitigation plan.
2. The review for the WikiLambda extension (T289322#7623085) was found to have an overall high risk, largely due to certain critical and high vulnerabilities found within various npm dev dependencies. I'm not entirely sure this warrants an overall high risk, given that these are dev dependencies, but I would assume this risk can be adjusted to medium or low by updating the affected packages. And again, we certainly would encourage further review of the additional, found issues and the creation of a mitigation plan. For now, this will be added to our risk register as a high risk owned by @dr0ptp4kt and @DVrandecic.
3. The review for the function-evaluator service (T289322#7684639) was found to have an overall medium risk. The package vulnerability issues will likely be mostly (completely?) mitigated by the proposed update to service-runner 3.x.x (T302017) but I'm not certain the overall risk can be reduced much further given the findings around the eval/exec/spawn issues, even though a number of security controls and mitigations are being employed to minimize this known risk, which was exhaustively discussed during various architecture and threat-modeling sessions. For now, this will be added to our risk register as a medium risk owned by @dr0ptp4kt and @DVrandecic.

I've also created T302472: Application Security Review Request : Abstract Wikipedia function-schemata as a non-blocking, additional review and maintenance task. Please let me know if you have any questions regarding that task or any of the determinations above. Thanks.

@sbasset just wanted to say thank you!