@JTannerWMF We expect this to be in progress shortly and @sbassett will be in touch with any questions or concerns.

@JTannerWMF @dchan - I hope to have the review completed by the end of this week (2020-02-14). Please let me know if that conflicts with your current deployment schedule. Thanks.

That works thanks @sbassett

@JTannerWMF - I had a chance to work on this this week, though I don't have it quite completed. And unfortunately, I had to take a sick day today. I'm hopeful I can still get a complete review posted later, but it might not be until Tuesday. Apologies for any inconvenience.

@JTannerWMF, @dchan - I haven't fully completed the security review (still need to review some of the js modules) but I thought I'd post what I have for now below, and get the rest out later tonight or tomorrow. I've security-protected the task as the extension is live on beta and 4 additional projects.

Security Review Summary - T242134 - 2020-02-18
Last commit reviewed: 219da7203177d96a47db41177e135140d8858479

Vulnerable Packages
As reported by snyk test:

Note: the below have been fixed with the latest release of stylelint-config-wikimedia (v0.9.0), see also: T245070.

1. dot-prop@4.2.0, Prototype Pollution (medium risk)
   • Introduced by stylelint-config-wikimedia@0.8.0 > stylelint@12.0.0 > postcss-selector-parser@3.1.1 > dot-prop@4.2.0. This issue was fixed in versions: 5.1.1. See also: https://snyk.io/vuln/SNYK-JS-DOTPROP-543489.
2. kind-of@6.0.2, Information Disclosure (low risk)
   • Introduced by stylelint-config-wikimedia@0.8.0 > stylelint@12.0.0 > global-modules@2.0.0 > global-prefix@3.0.0 > kind-of@6.0.2 and 44 other path(s). This issue was fixed in versions: 6.0.3. See also: https://snyk.io/vuln/SNYK-JS-KINDOF-537849.

Outdated Packages
As reported via npm outdated:

Note: the below package has been updated with the latest release of grunt-stylelint (v0.14.0) to address some security issues, see also: T245070.

Other General Security Issues

1. While it's fairly common for MediaWiki maintenance scripts (e.g. manageForeignResources.php) include/require files with dynamically-created paths via variables like $basePath and RUN_MAINTENANCE_IF_MAIN, these get picked up by various linters/static analysis tools as potential means of remote code injection. While such vulnerabilities should be difficult to exploit in the case of MediaWiki maintenance scripts, fortifying such includes/requires with checks for valid/expected paths is still probably a worthwhile effort in defensive coding. Here's a recent example and patch: T237419, r551654. (risk: low)
2. As an extra bit of paranoia, I might recommend using ->escaped() or ->parsed() instead of ->text() within includes/DiscussionToolsData.php on lines 71 and 89, unless there is a concern about double-escaping these message values. (risk: none, as clarified below)

Adding the rest of our engineers who can support with getting this resolved.

There's a patch for the outdated packages which covers most of this: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573048

I feel a bit silly doubling the size of the maintenance script with boilerplate because of RUN_MAINTENANCE_IF_MAIN, so I haven't written that patch yet, but if there's no better way and it's becoming the standard to appease contextless static analysis checkers then 🤷🏻‍♂️.

I haven't looked at General point 2 -- @matmarex wrote that code, so I figure I should let him verify whether or not double-escaping would be an issue there.

A patch for the defensive programming on the maintenance script: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573051

In T242134#5894691, @sbassett wrote:

2. As an extra bit of paranoia, I might recommend using ->escaped() or ->parsed() instead of ->text() within includes/DiscussionToolsData.php on lines 71 and 89, unless there is a concern about double-escaping these message values.

Thanks for the suggestion, but that would be incorrect. These messages are not outputted, but rather matched against existing text to extract timestamps from it. This change would break our date parsing in all languages that include an apostrophe in month or weekday names. A quick search [1] reveals potential problems in 29 languages – the most convenient example to test with is 'kaa' (Kara-Kalpak language), where every genitive form of month names contains an apostrophe, and they are used in the default date format.

[1] "(sunday|monday|tuesday|wednesday|thursday|friday|saturday|sun|mon|tue|wed|thu|fri|sat|january|february|march|april|may_long|june|july|august|september|october|november|decembe|january-gen|february-gen|march-gen|april-gen|may-gen|june-gen|july-gen|august-gen|september-gen|october-gen|november-gen|december-gen|jan|feb|mar|apr|may|jun|jul|aug|sep|oct|nov|dec)":.*'

@sbassett I'm struggling to make sense of your recommendation for the maintenance scripts. Are we trying to defend against an attacker who can somehow control the MW_INSTALL_PATH environmental variable? Surely if someone had that capability, they would have much easier ways of running arbitrary code than making a maintenance script load it.

@matmarex from the way they phrased it, I get the impression it’s more “we agree that this is almost impossible to attack, but our analysis tools won’t shut up about it because they know nothing of the context things run in”. 😂

That continues to make no sense for me. It seems implausible that a static analysis tool can understand that silly series of complicated conditions and determine that it is safe, but can't understand that getenv is safe. The usual way to silence malfunctioning static analysis tools is to exclude individual files or individual lines from their results.

Or is this about people spamming you with spurious "security issues" and expecting to be rewarded with "bounties"? I guess I can imagine how adding some complicated code that does nothing would help in this case…

I'm leaning towards just deleting that maintenance script and doing its job manually, if it's going to be a cause for annoyance.

In T242134#5898495, @matmarex wrote:

Thanks for the suggestion, but that would be incorrect. These messages are not outputted, but rather matched against existing text to extract timestamps from it. This change would break our date parsing in all languages that include an apostrophe in month or weekday names. A quick search [1] reveals potential problems in 29 languages – the most convenient example to test with is 'kaa' (Kara-Kalpak language), where every genitive form of month names contains an apostrophe, and they are used in the default date format.

No problem, it wasn't immediately obvious within the code. During any security review we often look for standard data sinks as potential areas of concern, with the various message rendering functions meeting this criteria and often being used to generate output. Given that it would be impossible for our team to have intimate knowledge of every codebase and third party library ever developed or used within the Wikimedia universe, we sometimes have to generalize, sometimes incorrectly. Apologies if this resulted in any anger or frustration for you.

In T242134#5898618, @matmarex wrote:

That continues to make no sense for me. It seems implausible that a static analysis tool can understand that silly series of complicated conditions and determine that it is safe, but can't understand that getenv is safe. The usual way to silence malfunctioning static analysis tools is to exclude individual files or individual lines from their results.

Or is this about people spamming you with spurious "security issues" and expecting to be rewarded with "bounties"? I guess I can imagine how adding some complicated code that does nothing would help in this case…

I'm leaning towards just deleting that maintenance script and doing its job manually, if it's going to be a cause for annoyance.

Just to provide some clarity here:

1. Yes, this issue was found via a static analysis tool.
2. We would normally rate this as a low risk (which I can do within the posted review) especially within our production environments, though it isn't "0 risk".
3. Nothing generated within our security reviews should be considered mandatory to fix. Our security reviews are a list of (hopefully relevant) issues which are assigned risk. Owners/stewards of a given codebase can then decide whether to address and fix each issue or accept the risk, if acceptance is warranted, based upon our standard risk management policy. The only risks that would potentially imply serious ramifications (such as blocking deployment) would be those classified as high or critical.

Sorry if I was rude. I am unhappy that we received instructions about the changes to make, complete with example code that straight up doesn't work, but no explanation on why the changes are needed.

Whenever I report security issues, I include a demonstration of the attack, or if that is impossible, at least a good explanation of how an attack could theoretically be constructed, or if that is impossible as well, then at the very least information about which tool identified this and how one would reproduce that locally. It would feel disrespectful of the other person's time not to.

Anyway, I fixed and merged the maintenance hardening patch. If you recommend the same changes to others in the future, please include the following corrections, which make the script work again: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573051/2..4/maintenance/manageForeignResources.php (I explained their purpose in code review comments on that patch).

In T242134#5901853, @matmarex wrote:

I am unhappy that we received instructions about the changes to make, complete with example code that straight up doesn't work

I'm not sure I understand. The relevant mitigations within the example MachineVision patch provided within the review do work for that extension. I'm not seeing where I stated that this example should be copied verbatim for DiscussionTools. Apologies if it was unclear that it was merely an example of how one team chose to address this issue specific to the MachineVision extension.

but no explanation on why the changes are needed.
Whenever I report security issues, I include a demonstration of the attack, or if that is impossible, at least a good explanation of how an attack could theoretically be constructed, or if that is impossible as well, then at the very least information about which tool identified this and how one would reproduce that locally. It would feel disrespectful not to.

It might be helpful if you could clarify the specific areas where the following explanation from the review could be improved. I tend to prefer brevity within my reviews as I assume most Wikimedia developers are already very familiar with many classes of vulnerabilities and would prefer a basic description of the issue and some options for mitigation. This seems to fit within the patterns of our team's previous security reviews, including those performed well before my days at the WMF. Anyhow, I'd be happy to include more detailed descriptions of various vulnerabilities if that would be helpful. Writing actual mitigation patches might be feasible under certain circumstances, though that begins to stray beyond what our team is resourced to do and our primary mandate of assessing risk within security reviews.

While it's fairly common for MediaWiki maintenance scripts (e.g. manageForeignResources.php) include/require files with dynamically-created paths via variables like $basePath and RUN_MAINTENANCE_IF_MAIN, these get picked up by various linters/static analysis tools as potential means of remote code injection. While such vulnerabilities should be difficult to exploit in the case of MediaWiki maintenance scripts, fortifying such includes/requires with checks for valid/expected paths is still probably a worthwhile effort in defensive coding. Here's a recent example and patch: T237419, r551654. (risk: low)

Anyway, I fixed and merged the maintenance hardening patch. If you recommend the same changes to others in the future, please include the following corrections, which make the script work again: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573051/2..4/maintenance/manageForeignResources.php (I explained their purpose in code review comments on that patch).

Noted, although I'm not certain every maintenance script requires the php and phar stream wrappers to be enabled and several make use of MWException just fine. The usage of realpath() for the comparison on line 51 does appear to be necessary under certain circumstances.

Finally, to reiterate point 3 above:

Nothing generated within our security reviews should be considered mandatory to fix. Our security reviews are a list of (hopefully relevant) issues which are assigned risk. Owners/stewards of a given codebase can then decide whether to address and fix each issue or accept the risk, if acceptance is warranted, based upon our standard risk management policy. The only risks that would potentially imply serious ramifications (such as blocking deployment) would be those classified as high or critical.

Please do not feel that the Security-Team is forcing you to merge this fix in any way. I'd note the discussion on the MachineVision patch as a good example of how these issues can be further debated if there are strong opinions regarding the implementation of any mitigation.

In T242134#5905263, @sbassett wrote:

In T242134#5901853, @matmarex wrote:

I am unhappy that we received instructions about the changes to make, complete with example code that straight up doesn't work

I'm not sure I understand. The relevant mitigations within the example MachineVision patch provided within the review do work for that extension. I'm not seeing where I stated that this example should be copied verbatim for DiscussionTools. Apologies if it was unclear that it was merely an example of how one team chose to address this issue specific to the MachineVision extension.

If MW_INSTALL_PATH is not set, then the second check will always fail, and the main script will never run.

Anyway, I realize now that I should complain about this elsewhere, but I'm still disappointed that several people reviewed and tested this code, which is supposed to be security-critical, and none of them noticed this problem.

but no explanation on why the changes are needed.
Whenever I report security issues, I include a demonstration of the attack, or if that is impossible, at least a good explanation of how an attack could theoretically be constructed, or if that is impossible as well, then at the very least information about which tool identified this and how one would reproduce that locally. It would feel disrespectful not to.

It might be helpful if you could clarify the specific areas where the following explanation from the review could be improved. I tend to prefer brevity within my reviews as I assume most Wikimedia developers are already very familiar with many classes of vulnerabilities and would prefer a basic description of the issue and some options for mitigation. This seems to fit within the patterns of our team's previous security reviews, including those performed well before my days at the WMF. Anyhow, I'd be happy to include more detailed descriptions of various vulnerabilities if that would be helpful. Writing actual mitigation patches might be feasible under certain circumstances, though that begins to stray beyond what our team is resourced to do and our primary mandate of assessing risk within security reviews.

While it's fairly common for MediaWiki maintenance scripts (e.g. manageForeignResources.php) include/require files with dynamically-created paths via variables like $basePath and RUN_MAINTENANCE_IF_MAIN, these get picked up by various linters/static analysis tools as potential means of remote code injection. While such vulnerabilities should be difficult to exploit in the case of MediaWiki maintenance scripts, fortifying such includes/requires with checks for valid/expected paths is still probably a worthwhile effort in defensive coding. Here's a recent example and patch: T237419, r551654. (risk: low)

As I said, assuming that this is a false positive reported by some tools, I'd like to know the name of the tool to help me evaluate whether adding a workarond for it is worth it.

(If this was a real security problem, which I now understand it isn't, I'd also have liked to know how attacker-controlled data could end up in $basePath.)

Anyway, I fixed and merged the maintenance hardening patch. If you recommend the same changes to others in the future, please include the following corrections, which make the script work again: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573051/2..4/maintenance/manageForeignResources.php (I explained their purpose in code review comments on that patch).

Noted, although I'm not certain every maintenance script requires the php and phar stream wrappers to be enabled and several make use of MWException just fine. The usage of realpath() for the comparison on line 51 does appear to be necessary under certain circumstances.

Yes, the stream wrappers and use lines are specific to our code. Sorry if this was unclear. However, I'm almost certain that the usage of MWException is also incorrect in the original code, the class appears to not be loaded at that point, so the script crashes with a confusing error message (I've tested the problem in two different dev environments, on Windows and Ubuntu, and noticed the same problem in both).

Finally, to reiterate point 3 above:

Nothing generated within our security reviews should be considered mandatory to fix. Our security reviews are a list of (hopefully relevant) issues which are assigned risk. Owners/stewards of a given codebase can then decide whether to address and fix each issue or accept the risk, if acceptance is warranted, based upon our standard risk management policy. The only risks that would potentially imply serious ramifications (such as blocking deployment) would be those classified as high or critical.

Please do not feel that the Security-Team is forcing you to merge this fix in any way. I'd note the discussion on the MachineVision patch as a good example of how these issues can be further debated if there are strong opinions regarding the implementation of any mitigation.

Understood, this wasn't clear to me originally.

@sbassett The sec review seems to be done here, as well as the mentioned vuln seems to be fixed. Any reasons to keep this private?

@Urbanecm - yes, I'll do that now.

The maintenance script mitigations make the scripts impossible to run in WMF production, see T316548.

In T242134#8194231, @matmarex wrote:

The maintenance script mitigations make the scripts impossible to run in WMF production, see T316548.

Thanks for dealing with this bug. Per the discussions above and within the initial review, the underlying issue was rated as low risk. Per the current risk management framework, this risk category is automatically accepted by the WMF and no additional action is required if any related mitigations are removed in full or in part.