Page MenuHomePhabricator

Security Review For Talk pages project
Closed, ResolvedPublicSecurity

Description

Project Information

Description of the tool/project

DiscussionTools is a MediaWiki extension to facilicate participation in talk page discussions. It parses a talk page for discussion threading, grouping and authorship according to established syntactical conventions. It provides a convenient way to reply "inline" to a comment, without having to reproduce those syntactical conventions correctly or even be aware of them.

DiscussionTools is written in client-side Javascript. There will also be a talk page parser in server-side PHP for reasons of performance and functionality.

See https://www.mediawiki.org/wiki/Talk_pages_project/Updates#Project_goals for more info, or T235592 for mockups.

Description of how the tool will be used at WMF

It will be made available on wikis as a means of participating in talk page discussions.

Dependencies

Wikimedia REST API to load talk pages / save comments.

VisualEditor extension for loading/saving (and potentially for parsing talk
pages and authoring comments in visual mode).

Moment Timezone to manipulate timestamps.

OOUI widget library.

Less for Stylesheets.

Has this project been reviewed before?

No.

Working test environment

See T235592 for mockups. See https://en.wikipedia.beta.wmflabs.org/wiki/Talk:Dog for deployed code

Post-deployment

Maintained by Editing team

Event Timeline

dchan created this task.Jan 7 2020, 5:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 7 2020, 5:21 PM
Jcross triaged this task as Low priority.Jan 7 2020, 6:25 PM
Jcross added a subscriber: Jcross.Feb 7 2020, 5:22 PM

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

sbassett claimed this task.Feb 7 2020, 5:22 PM
sbassett added a project: user-sbassett.
sbassett moved this task from Incoming to In Progress on the Security Readiness Reviews board.
sbassett moved this task from Backlog to In Progress on the user-sbassett board.

@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.

@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.

sbassett raised the priority of this task from Low to Needs Triage.Feb 18 2020, 9:18 PM
sbassett set Security to Software security bug.
sbassett changed the visibility from "Public (No Login Required)" to "Custom Policy".
sbassett changed the subtype of this task from "Task" to "Security Issue".

Vulnerabilities found for beta and deployed ext:DiscussionTools

Restricted Application added a project: Security. · View Herald TranscriptFeb 18 2020, 9:18 PM
sbassett moved this task from Incoming to Watching on the Security-Team board.EditedFeb 18 2020, 9:20 PM

@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.

sbassett triaged this task as Medium priority.Feb 18 2020, 9:20 PM
sbassett removed a project: Security-Team.
sbassett added a comment.EditedFeb 18 2020, 9:22 PM

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)
  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.

PackageCurrentWantedLatest
grunt-stylelint0.13.00.13.00.14.0

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

  1. 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.

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.

sbassett added a comment.EditedFeb 19 2020, 8:05 PM

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.
matmarex added a comment.EditedFeb 20 2020, 7:07 PM

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).

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.

sbassett moved this task from In Progress to Waiting on the user-sbassett board.Feb 25 2020, 9:40 PM

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 closed this task as Resolved.Mar 6 2020, 7:16 PM
sbassett moved this task from In Progress to Our Part Is Done on the Security Readiness Reviews board.

I don't believe there are any actionable items left here, so I'm going to resolve this task for now.

sbassett moved this task from Waiting to Done on the user-sbassett board.Mar 6 2020, 7:16 PM

@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.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 8 2020, 1:59 AM