Security Review Summary - T335051 - 2023-04-25

Overall, the current vendor code under consideration looks to be in decent shape from a security perspective. The following security analyses were performed in the review below:

• A review of the canonical vega repo. Various general assurances, vulnerable/outated package analysis and scorecard/health rating were evaluated.
• A review of Vega's primary features and security model via its canonical documentation site.
• Some standard automated security analysis (static analysis, etc.) of Vega 5.25.0 as currently shipped with ext:Graph.

The Security-Team has set an overall risk rating of Vega 5.25.0 as used with ext:Graph at: low.

General Security Information

Vulnerable Packages
As reported via npm/yarn audit (gh advisories), snyk, osv.dev, et al. These are all found within dev packages and indirect dependencies, and do not require immediate action. And the more serious ones for tar and request are within the year-old+ @definitelytyped/dtslint@0.0.111 package, which could stand to be upgraded. It might be worthwhile filing an upstream bug for that as v0.0.159 at least mitigates the tar issues. Risk: low.

Outdated Packages
As reported via npm outdated. No explicit vulnerabilities reported, simply noting for completeness' sake. There are a large number of dated dependencies here though, which is perhaps a marginally greater risk, though unsurprising given the size of vega and its sub-packages. Risk: low.

Scorecard score
6.2 / 10 - Risk: medium
(see raw output: P47294)

Snyk health status
98 / 100 - Risk: low
(see: https://snyk.io/advisor/npm-package/vega)

Potential HTTP and protocol Leaks

1. None found via this semgrep rule - Risk: low

Security Features Findings

1. Data loaded via URLs, if enabled, should continue to use allow-lists of domains (see c910613) and make use of more limiting, internal protocols such as wikiraw:, wikiapi:, et al via the Graph extension. http: should be disabled or avoided, though the exact level of enforcement is still being discussed in c913984 - Risk: low.
2. Vega expressions, primarily used with Signals (though which can also be used with filters and formulas) remain Vega's largest attack surface, though much has been done within recent versions of Vega to better guard against potential threats. First, expressions are far more restricted in their values and types via Vega's latest schema; from Vega's Expressions documentation:
   
   "The expression language is a restricted subset of JavaScript. All basic arithmetic, logical and property access expressions are supported, as are boolean, number, string, object ({}) and array ([]) literals. Ternary operators (ifTest ? thenValue : elseValue) and a special if(test, thenValue, elseValue) function are supported.
   
   To keep the expression language simple, secure and free of unwanted side effects, the following elements are not allowed: assignment operators (=, += etc), pre/postfix updates (++), new expressions, and most control flow statements (for, while, switch, etc). In addition, function calls involving nested properties (foo.bar()) are not allowed. Instead, the expression language supports a collection of functions defined in the top-level scope."
   
   This, along with numerous XSS fixes, should provide more safety from previous, outdated versions of Vega regarding the expressions functionality. Further protection would be available through the use of the vega interpreter, which should provide additional security via its means of traversing an Abstract Syntax Tree as opposed to previous parsing methods. It should be noted that this does however incur a modest performance penalty. This is being actively worked upon within c910605 and once implemented should reduce this potential vulnerability to a low risk.

Static Analysis Findings

1. lockfile-lint returned no results. Risk: low
2. gitleaks returned no results. Risk: low
3. whispers returned no results. Risk: low
4. git secrets returned no results. Risk: low
5. packj found 0 "risky" dependencies. Risk: low
6. scan.sh returned no true positive results. Risk: low
7. bearer SAST found 2 issues, see P47295. An upstream bug should likely be filed to harden this code. Risk: low
8. semgrep SAST found found 1 issue, see P47296. Risk: low

In T335051#8821712, @sbassett wrote:

Data loaded via URLs, if enabled, should continue to use allow-lists of domains (see c910613) and make use of more limiting, internal protocols such as wikiraw:, wikiapi:, et al via the Graph extension.

To clarify, do you mean that the current arbitrary URL access (well, arbitrary aside from a protocol and domain allowlist) should be disabled eventually, in favor of the internal protocols? We had debated how/if to prioritize that so I want to clarify if this is a requirement.

I have never read these security reports, could someone tell me why is canvas repeated twice and d3-array 12 times in the outdated packages table?

In T335051#8824502, @Tgr wrote:

To clarify, do you mean that the current arbitrary URL access (well, arbitrary aside from a protocol and domain allowlist) should be disabled eventually, in favor of the internal protocols? We had debated how/if to prioritize that so I want to clarify if this is a requirement.

I think the current URL access (which is basically a port of what existed in VegaWrapper) along with the various pseudo-schemes like wikiraw: should be low risk as-is. It would potentially be nice to restrict that access even further, at least within Wikimedia production, but I don't think that would substantially reduce the current risk level.

In T335051#8824727, @Base wrote:

I have never read these security reports, could someone tell me why is canvas repeated twice and d3-array 12 times in the outdated packages table?

Ah, that was due to them being depended on by several sub-packages of vega. I've added an extra column that hopefully clarifies this. Most of the vendor dependencies we review aren't structurally organized this way, so the outdated dependencies are usually obvious. But not in vega's case. Thanks for pointing that out.

In T335051#8824850, @sbassett wrote:

I think the current URL access (which is basically a port of what existed in VegaWrapper) along with the various pseudo-schemes like wikiraw: should be low risk as-is. It would potentially be nice to restrict that access even further, at least within Wikimedia production, but I don't think that would substantially reduce the current risk level.

If I read the code correctly, VegaWrapper (mw-graph-shared) only accepted custom schemes (wikiapi:/wikirest:/wikiraw:/tabular:/map:/wikifile:/wikidatasparql:/geoshape:/geoline:/mapsnapshot:). The current implementation allows arbitrary HTTPS URLs in Wikimedia domains (and no custom schemes ATM although that will probably change), so in that sense security is being relaxed. I just want to make sure there are no security concerns about that.

In T335051#8824925, @Tgr wrote:

If I read the code correctly, VegaWrapper (mw-graph-shared) only accepted custom schemes (wikiapi:/wikirest:/wikiraw:/tabular:/map:/wikifile:/wikidatasparql:/geoshape:/geoline:/mapsnapshot:). The current implementation allows arbitrary HTTPS URLs in Wikimedia domains (and no custom schemes ATM although that will probably change), so in that sense security is being relaxed. I just want to make sure there are no security concerns about that.

As previously discussed, I don't believe there are any potentially destructive operations available via query params to production MediaWiki URLs. If there are any that we'd find particularly concerning, then we'd want to potentially reconsider this risk rating.

Historically i think the concern was you load a normal page as a csv file, parse out the edit token from the html source, then leak the edit channel via a side channel to an attacker. [Otoh good luck executing csrf style attacks on a modern browser, so maybe that specific scenario is less relavent now, although similar scenarios may still matter]

Much more private stuff in the api. Which was the reasoning behind the magic header to make api response be logged out. However if general urls are exposed probably a lot of that stuff is available from get requests in the web interface. I think but am not sure you can get revdel'd revisions from the index.php interface with just a get.