Page MenuHomePhabricator

XXE Vulnerabilities in tests/phpunit/includes/ExportTest.php
Closed, ResolvedPublicSecurity

Description

In mediawiki-1.35.0-rc.3\tests\phpunit\includes\ExportTest.php, data is not parsed with enough sanitation.
The problem occurs on line 41 of https://phabricator.wikimedia.org/source/mediawiki/browse/master/tests/phpunit/includes/ExportTest.php$34 :

34		$sink = new DumpStringOutput;
35		$exporter->setOutputSink( $sink );
36		$exporter->openStream();
37		$exporter->pageByTitle( $title );
38		$exporter->closeStream();
39
40		// This throws error if invalid xml output
41		$xmlObject = simplexml_load_string( $sink );

Details

Author Affiliation
Wikimedia Communities

Event Timeline

Aklapper renamed this task from XXE Vulnearbilities to XXE Vulnerabilities in tests/phpunit/includes/ExportTest.php.Sep 16 2020, 12:57 PM
Aklapper added a project: MediaWiki-Core-Tests.
Aklapper updated the task description. (Show Details)
Aklapper added a project: Vuln-XXE.

Hello @HuaSir and thanks for the report.

While you are correct about there being little/no sanitisation of the XML in the reported line, the code itself is being used in a test, and is also using XML that is being generated immediately before by MediaWiki itself. It's not opening an arbitary file on disk or from the internet, nor would the generated XML be doing so either.

As per https://www.php.net/manual/en/function.libxml-disable-entity-loader.php

However, as of libxml 2.9.0 entity substitution is disabled by default, so there is no need to disable the loading of external entities.

I'm not saying it definitely cannot be used/abused, the chance and potential for abuse seems at best, limited.

The question is therefore whether it's worth really doing anything to improve the situation, being that it is test code. I'm going to put a patch up to wrap the simplexml_load_string in libxml_disable_entity_loader in ExportTest.php, mostly for explicit-ness as it would effectively be a no-op on any system running a supported version of MW. A quick look suggests Ubuntu 16.04 has 2.9.3 of libxml.. And that https://gitlab.gnome.org/GNOME/libxml2/-/tags/v2.9.0 is 8 years old

It's certainly possible people rebuild their own versions of libxml, set things against best practices etc.

If you've got a more specific exploit/reproduction of an actual (rather than just a theoretical) vulnerability, we'd be happy to explore it further

Reedy triaged this task as Low priority.Sep 21 2020, 3:32 PM
Reedy moved this task from Incoming to In Progress on the Security-Team board.
Reedy moved this task from Inbox to PHPUnit on the MediaWiki-Core-Tests board.
Reedy moved this task from Backlog to Work in Progress on the MediaWiki-Core-Snapshots board.

Thx. There are still potential XXE vulnerabilities. Although entity substitution is disabled by default, some people still use the old version.
In \tests\phan\bin\postprocess-phan.php, user input is also parsed directly when the mode is checkstyle:

class CheckStyleSuppressor extends Suppressor {
	/**
	 * @param string $input
	 * @return bool True do errors remain
	 */
	public function suppress( $input ) {
		$dom = new DOMDocument();
		$dom->loadXML( $input );
....
       }
}
....
switch ( $mode ) {
case 'text':
	$suppressor = new TextSuppressor();
	break;
case 'checkstyle':
	$suppressor = new CheckStyleSuppressor();
	break;
default:
	$suppressor = new NoopSuppressor( $mode );
}

$input = file_get_contents( 'php://stdin' );
$hasErrors = $suppressor->suppress( $input );

Thx. There are still potential XXE vulnerabilities. Although entity substitution is disabled by default, some people still use the old version.

Can you provide any actual real world examples where they are?

I doubt there's many people where they're running newer MW (and would benefit from the fixes).. Where they have a new enough PHP and an 8 year old version of libxml

Can you provide any actual real world examples where they are?

As we unfortunately have no further information (and as there is nothing actionable left here), I propose that this task should be closed?

sbassett closed this task as Resolved.EditedMay 17 2021, 6:43 PM
sbassett moved this task from Work in Progress to Done on the MediaWiki-Core-Snapshots board.
sbassett subscribed.

Calling this resolved (all relevant hardening change sets in gerrit are merged) and making public.

@HuaSir - would you like to be added to our security researcher hall of fame?

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".