Hi @Mholloway -

The team has taken a look at this feels we can provide a basic review by your target deployment date, but that a more in-depth look may need to happen post-launch. We hope that this will work for you and your team - please let us know if you have any questions or concerns and we'll be in touch as we move forward.

Cheers! Jennifer

Hi @Jcross, thanks for the heads-up. That sounds OK to me, but I've started a convo on the parent ticket to confirm that'll suffice for production deployment.

Hi @sbassett, for planning purposes, do you have some idea of when this might be complete?

@Mholloway - Hey, sorry for the delay on this - I just emailed Amanda this morning about it. I think I can have a general review by the end of next week, so 2019-11-01. Let me know if that blocks deploys or quarterly goals or anything.

Thanks for the update, @sbassett. I think that will work, provided we're speedy with any needed remediations (which I plan to be!). Our main concern is to deploy by the week of Nov. 4 at the latest, since the following week many engineers will be out for Wikimedia Technical Conference, and following that various teams have offsites.

Security Review Summary - T227346 - 2019-11-01
Overall, the MachineVision extension looks pretty good. There are a handful of issues below, but nothing I would categorize as above low for right now.

Vulnerable Packages
As reported by npm audit and retirejs:

1. jquery 3.3.1, prototype pollution
   • dependency of wikimedia/mw-node-qunit
   • https://nodesecurity.io/advisories/796
   • Affected files (post npm install):
     • node_modules/jquery/dist/jquery.js
     • node_modules/jquery/dist/jquery.min.js
     • node_modules/jquery/dist/jquery.slim.js
     • node_modules/jquery/dist/jquery.slim.min.js
     • node_modules/jquery/package.json

Outdated Packages
As reported via npm outdated:

No explicit vulnerabilities reported, simply noting for completeness' sake.

Other General Security Issues

1. While it's fairly common for MediaWiki maintenance scripts (e.g. populateFreebaseMapping.php, fetchSuggestions.php, createFileListFromCategoriesAndTemplates.php, createFileListFromGlobalImageLinks.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. The same also applies for any file names/paths passed to any of PHP's file system functions (e.g. file_put_contents() within createFileListFromCategoriesAndTemplates.php and createFileListFromGlobalImageLinks.php.)
2. While no i18n messages came back with any potentially dangerous html, it is an attack vector we've seen attempted before (happy to provide more details off-Phab). So while using ->text() in SpecialSuggestedTags.php (line 31) and MachineVisionEntitySaveException.php (line 19) should be fine, I think we'd recommend using ->parse() or ->escaped() instead, if possible. This would also be true for instances of mw.message().text() within various JavaScript widget files.
3. In src/Handler/WikidataIdHandler.php on line 68 - did the SecCheckPlugin return this as a SecurityCheck-DoubleEscaped when you were testing locally or in CI? This shouldn't be the case since only the $contents argument should be sanitized within Html::element.
4. Per this gerrit comment from Brian, to leverage MediaWiki's support of CSP, you do not want to explicitly call it on any of your pages. So you'd probably want to revert r542121 for now.

Policy Compliance

1. Per the MachineVision Concept Review (T227591), can we confirm that WMF-Legal's review of the extension/service was completed? Is there any corroborating documentation or a new privacy policy/ToU we could reference and review?

Note: the above is a fairly basic review. I didn't focus on any performance-related issues as they pertain to security, as I'm hoping that's happening in T230813. I might also play around with the ext from a black box perspective next week with @Mholloway's docker environment. At a cursory review, r547555 and r547688 both look fine to me, but I might also review those a bit more next week. But none of this should block any deployments, etc.

Change 548761 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/MachineVision@master] Hygiene: Remove unnecesarry Phan suppression

https://gerrit.wikimedia.org/r/548761

About that SecurityCheck-DoubleEscaped suppression, the rule is in fact triggered in CI if the suppression is removed:

10:07:01 <checkstyle version="6.5">
10:07:01   <file name="src/Handler/WikidataIdHandler.php">
10:07:01     <error line="68" severity="warning" message="Calling method \Html::element() in \closure_aa88a63f8555 that outputs using tainted argument $[arg #2]. (Caused by: Builtin-\Html::element) (Caused by: src/Handler/WikidataIdHandler.php +69; src/Handler/WikidataIdHandler.php +68)" source="SecurityCheck-DoubleEscaped"/>
10:07:01   </file>
10:07:01 </checkstyle>

https://integration.wikimedia.org/ci/job/mwext-php72-phan-seccheck-docker/18657/console

Change 548761 abandoned by Mholloway:
Hygiene: Remove unnecesarry Phan suppression

Reason:
Needed for CI

https://gerrit.wikimedia.org/r/548761

In T227346#5636495, @Mholloway wrote:

About that SecurityCheck-DoubleEscaped suppression, the rule is in fact triggered in CI if the suppression is removed:

Interesting. I think that may be a bug - I'll file one for the phan-taint-check-plugin project. But yes, it obviously needs to be suppressed for now.

In T227346#5636600, @sbassett wrote:

In T227346#5636495, @Mholloway wrote:

About that SecurityCheck-DoubleEscaped suppression, the rule is in fact triggered in CI if the suppression is removed:

Interesting. I think that may be a bug - I'll file one for the phan-taint-check-plugin project. But yes, it obviously needs to be suppressed for now.

Wait, it looks like this actually is a legitimate double-escape, given that attribute values within Html::element and Html::rawElement are sanitized within expandAttributes(). Playing around in maintenance/shell.php:

>>> $id = '<script>alert()</script>';
=> "<script>alert()</script>"
>>> $x = Html::element( 'a', [                                                                                                                            'href' => 'https://www.wikidata.org/wiki/' . htmlentities( $id ),                                                                                 ], 'hello' );
=> "<a href="https://www.wikidata.org/wiki/&amp;lt;script&amp;gt;alert()&amp;lt;/script&amp;gt;">hello</a>"

and without htmlentities():

=> "<script>alert()</script>"
>>> $x = Html::element( 'a', [                                                                                                                            'href' => 'https://www.wikidata.org/wiki/' . $id,                                                                                                 ], 'hello' );
=> "<a href="https://www.wikidata.org/wiki/&lt;script&gt;alert()&lt;/script&gt;">hello</a>"

Change 548920 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/MachineVision@master] Stop escaping Wikidata IDs when constructing Wikidata URLs

https://gerrit.wikimedia.org/r/548920

Thanks for digging in on that, @sbassett. I imagine the htmlentities call was originally included out of habit when constructing a URL, but in this case the $id values will only be valid Wikidata IDs (i.e., 'Q' followed by a positive integer) from the DB and thus would need no escaping (even if they weren't escaped anyway within Html::element). I've submitted a new patch to remove the htmlentities call and the Phan suppression.

Change 548920 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] Stop (double-)escaping Wikidata IDs when constructing Wikidata URLs

https://gerrit.wikimedia.org/r/548920

In T227346#5638509, @Mholloway wrote:

Thanks for digging in on that, @sbassett. I imagine the htmlentities call was originally included out of habit when constructing a URL, but in this case the $id values will only be valid Wikidata IDs (i.e., 'Q' followed by a positive integer) from the DB and thus would need no escaping (even if they weren't escaped anyway within Html::element). I've submitted a new patch to remove the htmlentities call and the Phan suppression.

Sure, thanks. Generally speaking, I personally believe it fine to be incredibly distrusting of any dynamic data, even data which we know will always be safe like Wikidata ids. If not to guard against currently unknown or zero-day attack vectors, then to guard against random users (especially privileged users) going rogue or having their accounts compromised.