Page MenuHomePhabricator

MW 1.27 and 1.28 require-dev versions of phpunit with known security issues
Closed, ResolvedPublic

Description

Per email received on security@

Hi
 
On July 27th this year, CVE-2017-9841[0] was issued noting a vulnerability in phpunit. There is actually a good read about it on vulnbusters[1]. There is a lot of convoluted instructions on using composer with MediaWiki and the need for using the –-no-dev option. Someone using composer to install the MW extension with or without an composer.local.json file would not be aware of using –-no-dev with interacting with composer. Unless I am missing the documentation on MediaWiki.org, please point it out.
 
An excellent sysadmin I work with on a project noticed an odd process running yesterday, Nov 7th. Doing some back tracing, something was installing phpunit version 4.8.24. After blocking, doing research and running composer with the –-no-dev option, the vulnerability was closed. Doing some testing with the following and using wikiapiary to make a list of wikis:
 
curl --data "<?php echo(pi());" https://some-wiki-name.org/vendor/phpunit/phpunit/src/Util/PHP/eval-stdin.php
 
On the 8th wiki I tried, I came across a vulnerable /vendor/ directory with phpunit. Two of the other 7 returned a 403, so I’m guessing they locked their /vendor/ directory. The other 5 returned a 401. Tested variety from 1.26 on. It was 1.27.1 was the one that returned a vulnerable.
 
Should MediaWiki by default use an .htaccess file in the /vendor/ directory with “Deny from all” by default?
 
Thanks
Tom Hutchison
 
[0] http://www.cvedetails.com/cve/CVE-2017-9841/
[1] http://phpunit.vulnbusters.com/

Event Timeline

Reedy created this task.Nov 10 2017, 2:04 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 10 2017, 2:04 PM
Reedy updated the task description. (Show Details)Nov 10 2017, 2:04 PM

To fix the immediate issue:

REL1_27 https://gerrit.wikimedia.org/r/390408
REL1_28 https://gerrit.wikimedia.org/r/390409
REL1_29 https://gerrit.wikimedia.org/r/390410/ (On a newer version than 3.8.24, but might aswell do it for consistency for CI)
REL1_30 https://gerrit.wikimedia.org/r/390411/ (see above)

Reedy added a comment.Nov 10 2017, 2:35 PM

CC'ing @Hutchy68 as original reporter

Reedy added a subscriber: Hutchy68.Nov 10 2017, 2:35 PM

I just found a wiki run by relatively experienced MediaWiki developers that was vulnerable to this issue, so I'm a bit worried about the impact this could cause, as this is basically a RCE caused by MediaWiki's dependencies. We socialized "composer install" pretty well, but not "composer install --no-dev".

I've emailed composer's security address explaining the issue to them, and pointed them to Reedy's feature request and whether they'd be interested in implementing the .htaccess solution upstream. Regardless we should still mitigate that since people will continue to use old composer versions.

I'm wondering if we should also include a patch to just delete that file if it exists so people can patch without dealing with composer. At the least highlight the specific file to delete very prominently in the announcement.

There's also the question of if the vendor included by the tarballs or mediawiki/core/vendor.git includes the dev dependencies.

Reedy added a comment.Nov 10 2017, 7:42 PM

There's also the question of if the vendor included by the tarballs or mediawiki/core/vendor.git includes the dev dependencies.

https://github.com/wikimedia/mediawiki-vendor contains no phpunit. It does contain some dependancies considered "dev" (json-schema for example)

And then we just bundle mediawiki-vendor into the tarballs... So they're fine too (from this specific issue, at least)

Reedy added a comment.Nov 10 2017, 7:44 PM

wikimedia/avro, wikimedia/testing-access-wrapper, wikimedia/monolog, nikic/php-parser and nmred/kafka-php too

Reedy added a comment.Nov 10 2017, 7:46 PM

Oh, and..

$ php ../composer.phar outdated
data-values/data-values               1.1.1     2.1.1     Defines the DataValue interface and some trivial implementations
diff/diff                             2.2.0     3.0.0     Small standalone library for representing differences between data structures, computing such differences, and applying...
elasticsearch/elasticsearch           5.3.0     v5.3.2   
firebase/php-jwt                      v4.0.0    v5.0.0    A simple library to encode and decode JSON Web Tokens (JWT) in PHP. Should conform to the current spec.
james-heinrich/getid3                 v1.9.14   v1.9.15   PHP script that extracts useful information from popular multimedia file formats
justinrainbow/json-schema             5.2.1     5.2.6     A library to validate a json schema.
monolog/monolog                       1.22.1    1.23.0    Sends your logs to files, sockets, inboxes, databases and various web services
nikic/php-parser                      v3.0.6    v3.1.2    A PHP parser written in PHP
nmred/kafka-php                       v0.1.5    v0.2.0.8  Kafka client for php
pimple/pimple                         v3.0.2    v3.2.2    Pimple, a simple Dependency Injection Container
psy/psysh                             v0.8.11   v0.8.14   An interactive shell for modern PHP.
serialization/serialization           3.2.2     4.0.0     Library defining a Serializer and a Deserializer interface and basic utilities
stil/gd-text                          v1.0.0    v1.1.0    A class drawing multiline and aligned text on pictures. Uses GD extension.
symfony/console                       v3.3.4    v3.3.10   Symfony Console Component
symfony/debug                         v3.3.4    v3.3.10   Symfony Debug Component
symfony/polyfill-mbstring             v1.4.0    v1.6.0    Symfony polyfill for the Mbstring extension
symfony/process                       v3.2.6    v3.3.10   Symfony Process Component
symfony/var-dumper                    v3.3.4    v3.3.10   Symfony mechanism for exploring and dumping PHP variables
wikimedia/equivset                    1.0.0     1.1.0     Visually Equivalent Set of UTF-8 Characters
zordius/lightncandy                   v0.23     v1.2.1    An extremely fast PHP implementation of handlebars ( http://handlebarsjs.com/ ) and mustache ( http://mustache.github.i...

Reedy and I discussed this on IRC and came to the idea of using update.php to detect for vulnerable versions of the file, and deleting just it, given that most people are likely to run update.php after upgrading, regardless of whether there are any database updates.

Reedy added a comment.Nov 11 2017, 1:26 AM

I was unable to find any kind of advisory from PHPUnit about the issue so I contacted Sebastian Bergmann asking if they'd be willing to issue one. In the meantime I've submitted the phpunit CVE to https://github.com/FriendsOfPHP/security-advisories/pull/236 so automatic checks should identify it (see T180278: Expand our usage of FriendsOfPHP/security-advisories as well).

Reedy closed this task as Resolved.Nov 14 2017, 11:03 PM
Reedy claimed this task.

Closing this.. It's mostly done bar some of the under followup work... which may or may not happen

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 15 2017, 12:05 AM

Change 391414 merged by Ejegg:
[mediawiki/core@fundraising/REL1_27] update.php: Remove eval-stdin.php if necessary

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

Change 391453 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@master] SECURITY: update.php: Remove eval-stdin.php if necessary

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

Change 391453 merged by Reedy:
[mediawiki/core@master] SECURITY: update.php: Remove eval-stdin.php if necessary

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

Foxtrott reopened this task as Open.Nov 20 2017, 1:30 PM
Foxtrott added a subscriber: Foxtrott.

The applied patch also blocks legitimate file access, e.g. to style files, fonts, etc. See https://github.com/cmln/chameleon/issues/55

I suggest doing something like

<FilesMatch "\.(php|inc)$">
Order allow,deny
Deny from all
</FilesMatch>

See http://httpd.apache.org/docs/current/mod/core.html#filesmatch

Anomie added a subscriber: Anomie.Nov 20 2017, 5:20 PM

I'll let someone else make the final decision, but I'm pretty sure the answer is going to be that your skin there needs to be fixed somehow to not have web-loaded resources in the vendor directory.

Legoktm closed this task as Resolved.Nov 20 2017, 6:38 PM

MediaWiki currently doesn't require vendor/ to be web accessible, so I suspect that your skin wouldn't work for many people already. In the meantime you can just write whatever you want in the vendor/.htaccess file and MediaWiki won't overwrite it, it just has to exist. @Foxtrott could you file a new issue for this? I'm not really sure about how to support this case, as we don't have anything in place yet for frontend assets (T107561: MediaWiki support for Composer equivalent for JavaScript packages).

Done.
Would be great if somebody could suggest a way how the "skin there [can] be fixed somehow to not have web-loaded resources in the vendor directory". Preferably without dumping Composer entirely.