@tstarling, any idea why hhvm would (seemingly) try to expand the entities, while zend doesn't?

The source of the memory increasing seems to be when XmlTypeCheck is used (using xml_parse again), and we set elementData as a callback to handle the body of elements. This gets recalled for each expansion of the entity, so memory rises to memory_limit, and then the call is appropriately killed.

The mediawiki vagrant image doesn't set memory_limit, so that's why iSec saw it eating up 3GB of memory. In production, we set this to 300MB, so I'm fairly confident the memory

Slightly more concerning to me is that when I comment out the xml_set_character_data_handler, so xml_parse just keeps expanding and reading the xml without using more memory, the process keeps running for well beyond max_execution_time. @tstarling, are we sure max_execution_time is being enforced in production?

In T85848#1002970, @csteipp wrote:

@tstarling, any idea why hhvm would (seemingly) try to expand the entities, while zend doesn't?

And actually, zend not doing the expansion is an issue, since at least Chrome will expand the entities when interpreting the svg, so we should expand before we run the filtering.

In T85848#1002970, @csteipp wrote:

@tstarling, any idea why hhvm would (seemingly) try to expand the entities, while zend doesn't?

Sounds like a bug to me.

In T85848#1005433, @csteipp wrote:

In T85848#1002970, @csteipp wrote:

@tstarling, any idea why hhvm would (seemingly) try to expand the entities, while zend doesn't?

And actually, zend not doing the expansion is an issue, since at least Chrome will expand the entities when interpreting the svg, so we should expand before we run the filtering.

Presumably there is some option we should be setting to enable that.

In T85848#1005265, @csteipp wrote:

The source of the memory increasing seems to be when XmlTypeCheck is used (using xml_parse again), and we set elementData as a callback to handle the body of elements. This gets recalled for each expansion of the entity, so memory rises to memory_limit, and then the call is appropriately killed.

The mediawiki vagrant image doesn't set memory_limit, so that's why iSec saw it eating up 3GB of memory. In production, we set this to 300MB, so I'm fairly confident the memory

Slightly more concerning to me is that when I comment out the xml_set_character_data_handler, so xml_parse just keeps expanding and reading the xml without using more memory, the process keeps running for well beyond max_execution_time. @tstarling, are we sure max_execution_time is being enforced in production?

No, I am not sure. I couldn't find any execution time exceeded messages in hhvm.log.

Presumably there is some option we should be setting to enable that.

I haven't managed to find a way to get php to do that. Talking to Tim in person, it sounds like we might want to move processing to XMLReader instead. I'm going to move this to a subtask, since it's a flaw specific to Zend, that allows getting around the blacklist.

For this bug (DoS via billion-laughs xml), the memory and time limits are what we'll use to prevent the DoS. The more I've thought through this, I think the security patch might need to check memory_limit and max_execution_time, and throw an exception if someone is uploading an svg and either of those is set to infinite, along with a config flag to disable the check (defaulted to false) that would allow third-party users to skip the check if they want to accept the risk. That means a 3rd party that has svg/pdf uploading, and doesn't have memory/time limits set will get a (hopefully helpful) exception message until they either set limits or the config flag.

We could probably also try to set a limit if none is defined before throwing an exception, so only systems where those configs are protected will actually lose the feature.

Patch to throw an exception if limits aren't set whenever we use XMLReader with SUBST_ENTITIES set, or we're on hhvm and use xml_parse in core. I added a warning in Setup.php when hhvm is used and limits aren't set because there are a ton of extensions that use xml_parse.

In T85848#1029715, @csteipp wrote:

T85848.patch6 KBDownload

@tstarling or @Anomie, your input would be appreciated.

ini_get() returns the setting as a string, so === with integers is never going to match.

While this particular attack only affects HHVM, I wonder whether we shouldn't remove wfIsHHVM() from the check you're adding in Setup.php.

Similarly, in XmlTypeCheck.php and XMP.php, I wonder if wfIsHHVM() is really the best check versus just assuming vulnerability (in case Zend gets fixed) or processing a minimal XML document to directly see if it's going to be expanded (in case HHVM gets unfixed).

In T85848#1031832, @Anomie wrote:

While this particular attack only affects HHVM, I wonder whether we shouldn't remove wfIsHHVM() from the check you're adding in Setup.php.

Since you mentioned it, I realized that zend is still vulnerable to a slightly different version of the attack (what I had flagged to look into in T71210). So yeah, I'm going to remove the checks for hhvm.

Code review: +1 Looks sane, haven't tested.

I attempted to deploy

Looking at our puppet repo, we're setting memory_limit in modules/hhvm/manifests/init.pp, modules/mediawiki/files/php/php.ini, and modules/mediawiki/files/php/php.ini.cli, but max_execution_time we're only setting in the mediawiki php.ini files. It doesn't look like we're setting it for hhvm.

I'll update the patch to output the actual values, and then I can track down why it's not getting set.

Rebased on top of the final patch for T88310, and outputs the current limits with wfWarn in Setup.php. I'll deploy this to test briefly to see which one is triggering it.

Confirmed that we're not setting max_execution_time on testwiki, and it looks like all of our wikis.

[warning] Running without memory_limit (currently 346030080) or max_execution_time (currently 0) defined might allow some DoS attacks to succeed. [Called from include in /srv/mediawiki/php-1.25wmf18/includes/Setup.php at line 681]

@Joe, are we supposed to be setting max_execution_time for hhvm?

And another wrinkle..

When using xml_parse without an xml_set_character_data_handler callback, hhvm will spend will over max_execution_time trying to parse the xml. E.g.,

vagrant@mediawiki-vagrant:/vagrant/mediawiki/maintenance$ time hhvm blol.php
max_execution_time: 2

Done in 73.960172176361 secs
[Tue Feb 24 20:16:41 2015] [hphp] [6573:7fed8595abc0:0:000001] [834dc6:835050:9ca744:9caa13:9cf12a:9d070f:856a69:7fed7d5efec5:909e04] 
Fatal error: Maximum execution time of 2 seconds exceeded in /vagrant/mediawiki/maintenance/blol.php on line 54

real	1m14.164s
user	0m36.122s
sys	0m34.261s

We use xml_parse without a callback in MimeMagic.php, so even with max_execution_time set, hhvm continues consuming 100% of a cpu for a while after apache has returned an error, when an svg with something like this is uploaded.

I emailed Brett upstream to see what their thoughts on it are. We can always use XMLReader without entity expansion for the MimeMagic check. Or we can define a dummy xml_set_character_data_handler so hhvm checks max_execution_time more regularly. I'm leaning towards the first.

In T85848#1063822, @csteipp wrote:

We can always use XMLReader without entity expansion for the MimeMagic check. Or we can define a dummy xml_set_character_data_handler so hhvm checks max_execution_time more regularly. I'm leaning towards the first.

That doesn't work out so well... Adobe Illustrator does stuff like,

<!ENTITY ns_svg "http://www.w3.org/2000/svg">
...
<svg version="1.1" id="Layer_1" xmlns="&ns_svg;"

Which screws up MimeMagic::doGuessMimeType() becuase $xmlMimeTypes['&ns_svg;:svg'] isn't defined, for obvious reasons.

On to plan b..

I tested setting a dummy callback in XmlTypeCheck, which works great for when the entity is in the xml body. But if the entity is in an attribute value, the whole thing blows up again (dies after it's tries to allocate more than all available memory on the machine).

So I think we're stuck with using the (slightly slower) XmlReaderTypeCheck with entity expansion enabled in MimeMagic.

Which is probably the right way to do this whole things since XMLReader in hhvm uses libxml (just like zend does), which has built in limits for entity expansion, so we actually don't need to the memory_limit and max_execution_time checks before running it.

I haven't managed to get an answer from brett if FB is interested in fixing hhvm's version of xml_parse, so I'm assuming that we need to update mediawiki to avoid calling it on user-controlled xml.

So I patched MimeMagic and XMPReader in

There we very few extensions that used xml_parse directly. SMW being one exception, although I'm not sure if user controlled xml can get to their usage.

I could probably update XmlTypeCheck to use thew new SafeXmlParser, so anything that uses an XmlTypeCheck is protected by default, but I might leave that for a public patchset.

@Anomie, care to sanity check?

Code itself seems mostly ok, although I don't really know enough about xml_parse to say whether SafeXmlParser is functionally correct.

The bit in XMP.php where it has to cache the whole file before calling ->parse() is a little worrying if it's supposed to be completely interoperable though. And it's not clear how using XmlReader::open() avoids problems with partial documents in XmlReader::XML(), while it seems likely that using 'open' will use more memory for large XML documents due to the need to urlencode the input string.

Should there be a ScopedCallback or a catch-and-rethrow to ensure libxml_disable_entity_loader( $oldDisable ) gets called even if one of the callbacks throws an exception?

I see includes/media/BitmapMetadataHandler.php and includes/media/JpegMetadataExtractor.php are still calling function_exists( 'xml_parser_create_ns' ) in a few places before loading XMPReader, which should be fixed. Even better, add some sort of XMPReader::isSupported() method instead of having callers know what magic to test for.

I see Xml::isWellFormed still uses xml_parse, and the only caller seems to be the function in Parser for checking user-supplied sigs.

I don't know if maintenance/backupTextPass.inc needs fixing or not. tests/parser/parserTest.inc is theoretically ok, since parser tests shouldn't be taking user input. includes/installer/PhpBugTests.php is safe, although I don't know how useful it might be if we're removing xml_parse from use.

If you want nitpicking, there shouldn't be a space between "list" and the open-paren, much like array(). Nor between "set_error_handler" and its open-paren. And there 4 lines in SafeXmlParserTest.php with whitespace at the end of the line.

Thanks Brad,

In T85848#1078851, @Anomie wrote:

Code itself seems mostly ok, although I don't really know enough about xml_parse to say whether SafeXmlParser is functionally correct.

The bit in XMP.php where it has to cache the whole file before calling ->parse() is a little worrying if it's supposed to be completely interoperable though. And it's not clear how using XmlReader::open() avoids problems with partial documents in XmlReader::XML(), while it seems likely that using 'open' will use more memory for large XML documents due to the need to urlencode the input string.

It's weird. But basically, I'm trying to parse as much of the xml as I can, even if it's invalid.

> $xml = '<a>foo<b>bar<c>baz<d/>';

> $r = new XMLReader();

> $r->open( 'data://text/plain,' . urlencode( $xml ), null, LIBXML_NOERROR | LIBXML_NOWARNING  );

> while ( $r->read() ) { print "{$r->nodeType}: {$r->name}\n"; }
1: a
3: #text
1: b
3: #text
1: c
3: #text
1: d
PHP Warning:  XMLReader::read(): An Error Occurred while reading in /home/csteipp/code/core/maintenance/eval.php(78) : eval()'d code on line 1

Warning: XMLReader::read(): An Error Occurred while reading in /home/csteipp/code/core/maintenance/eval.php(78) : eval()'d code on line 1

> unset( $r );

> $r = new XMLReader();

> $r->XML( $xml, null, LIBXML_NOERROR | LIBXML_NOWARNING  );

> while ( $r->read() ) { print "{$r->nodeType}: {$r->name}\n"; }
PHP Warning:  XMLReader::read(): An Error Occurred while reading in /home/csteipp/code/core/maintenance/eval.php(78) : eval()'d code on line 1

Warning: XMLReader::read(): An Error Occurred while reading in /home/csteipp/code/core/maintenance/eval.php(78) : eval()'d code on line 1

>

Should there be a ScopedCallback or a catch-and-rethrow to ensure libxml_disable_entity_loader( $oldDisable ) gets called even if one of the callbacks throws an exception?

I see includes/media/BitmapMetadataHandler.php and includes/media/JpegMetadataExtractor.php are still calling function_exists( 'xml_parser_create_ns' ) in a few places before loading XMPReader, which should be fixed. Even better, add some sort of XMPReader::isSupported() method instead of having callers know what magic to test for.

If you want nitpicking, there shouldn't be a space between "list" and the open-paren, much like array(). Nor between "set_error_handler" and its open-paren. And there 4 lines in SafeXmlParserTest.php with whitespace at the end of the line.

Should be all fixed in

Brett indicated that FB is looking at what they can do to solve the issue, so this might all be unnecessary.

In T85848#1080879, @csteipp wrote:

It's weird. But basically, I'm trying to parse as much of the xml as I can, even if it's invalid.

That's some weird behavior from XmlReader.

What we really need is some stream wrapper that'll read directly from a variable, rather than having to re-encode as a data URI... If I write that, would you be interested? Or should I not bother?

Should be all fixed in

mimemagic-and-xmp2.patch22 KBDownload

You missed one check of xml_parser_create_ns in BitmapMetadataHandler::GIF.

Also, what happened to this one?

I see Xml::isWellFormed still uses xml_parse, and the only caller seems to be the function in Parser for checking user-supplied sigs.

Brett got back to me, and they're ok with us releasing patches to work around hhvm. They're aware it's an issue, but it's not super high priority for them right now, and they haven't been able to find a quick solution for the issue.

I really don't feel comfortable with the XMP patch, so I think the best way to handle that is like Wordpress-- detect a doctype declaration in the xml and just abort if we see one. XMP implements RDF/XML, which as far as I can tell does support a doctype and expandable entities, but I'm hoping these are rare in practice. I'll download and grep a big chunk of commons to check that.

In T85848#1110406, @csteipp wrote:

I really don't feel comfortable with the XMP patch, so I think the best way to handle that is like Wordpress-- detect a doctype declaration in the xml and just abort if we see one. XMP implements RDF/XML, which as far as I can tell does support a doctype and expandable entities, but I'm hoping these are rare in practice. I'll download and grep a big chunk of commons to check that.

I downloaded about 3k recent jpg/gif/png/pdf files from commons, and just over 1k included XML blocks. Of those, none had a doctype in the XMP block. So I feel pretty good about simply not parsing the XMP block if there is a doctype found in the xml.

So these two:

Replace

As for,

I see Xml::isWellFormed still uses xml_parse, and the only caller seems to be the function in Parser for checking user-supplied sigs.

As you said, the only caller seems to be internal to the Xml class, which is setting a root element and its own doctype around the xml. So no danger of expansion for anything coming in through Xml::isWellFormedXmlFragment. Maybe make isWellFormed protected, so calls have to go through isWellFormedXmlFragment?

In T85848#1115508, @csteipp wrote:

mimemagic2.patch933 BDownload

Seems ok.

xmp-test.patch6 KBDownload

In XMPReader, you create a field "parseInProgress" but never set it to true?

There's no way that something could somehow fragment things so the full doctype isn't passed to XMPReader::parse() in a way that it would be detected? For example, if I read this Adobe PDF about XMP correctly it sounds like the XML document for extended XMP in a jpeg is just chopped up substr()-wise into multiple ExtendedXMP segments with no minimum size. What if the doctype part is chopped into single byte segments?

As you said, the only caller seems to be internal to the Xml class, which is setting a root element and its own doctype around the xml. So no danger of expansion for anything coming in through Xml::isWellFormedXmlFragment. Maybe make isWellFormed protected, so calls have to go through isWellFormedXmlFragment?

Ah, that makes sense. I like the protected idea, but I'd like private even better, or maybe just inline it into isWellFormedXmlFragment and remove it as a separate function entirely.

There's no way that something could somehow fragment things so the full doctype isn't passed to XMPReader::parse() in a way that it would be detected? For example, if I read this Adobe PDF about XMP correctly it sounds like the XML document for extended XMP in a jpeg is just chopped up substr()-wise into multiple ExtendedXMP segments with no minimum size. What if the doctype part is chopped into single byte segments?

True, need to buffer and check. Added a test case for that situation too.

Ah, that makes sense. I like the protected idea, but I'd like private even better, or maybe just inline it into isWellFormedXmlFragment and remove it as a separate function entirely.

In T85848#1118447, @csteipp wrote:

True, need to buffer and check. Added a test case for that situation too.

xmp-test2.patch9 KBDownload

• The behavior of xml_parse(), and therefore XMPReader::parse(), when $allOfIt = false is that it returns true until it finds an error. You seem to have assumed it returns false until it knows the file is good. Create an otherwise-valid file that leaves off the final '>', for example, and compare the return value when you run it through whole versus how you do testCheckParseSafety().
  • Which also means the logic you're adding in XMPReader after the call to checkParseSafety() needs adjustment to return true if !$allOfIt and $this->parsable is not NO, and revise the error message for the non-NO case when $allOfIt === true.
• The comment "XMP blocks must be less than 64KB, so urlencoding isn't completely crazy." doesn't really apply anymore, since we could be buffering hundreds of them.

iswellformed.patch1 KBDownload

+1

• The behavior of xml_parse(), and therefore XMPReader::parse(), when $allOfIt = false is that it returns true until it finds an error. You seem to have assumed it returns false until it knows the file is good. Create an otherwise-valid file that leaves off the final '>', for example, and compare the return value when you run it through whole versus how you do testCheckParseSafety().
  • Which also means the logic you're adding in XMPReader after the call to checkParseSafety() needs adjustment to return true if !$allOfIt and $this->parsable is not NO, and revise the error message for the non-NO case when $allOfIt === true.
• The comment "XMP blocks must be less than 64KB, so urlencoding isn't completely crazy." doesn't really apply anymore, since we could be buffering hundreds of them.

No current code (core or WMF repos) checks the return of parse / parseExtended, but you're right, it shouldn't really indicate that it was unsuccessful. I also updated the test case to have it's own "good" .xmp file for testing.

In T85848#1122924, @csteipp wrote:

xmp-test3.patch11 KBDownload

+1, although "fregmented" looks like a typo.

I hear all the cool kids are fregmenting nowadays.

In T85848#1122987, @csteipp wrote:

xmp-test5.patch11 KBDownload

+1

To address T71210, we can update the XMP patch to always check the xml with XMLReader before sending it through xml_parse, and backport that. The patch has been running on hhvm servers for a while with no complaints from the commons community that I've been able to find, so I believe this shouldn't break things for most installs.

In T85848#1157830, @csteipp wrote:

T85848-xmptest6.patch11 KBDownload

+1 I like this even better than

In T85848#1158031, @Anomie wrote:

+1 I like this even better than

xmp-test5.patch11 KBDownload

, since it doesn't assume non-HHVM is automatically ok.

Deployed this afternoon as part of,
21:10 csteipp: redeploy security patches to wmf22
21:02 csteipp: redeploy security patches to wmf23

While testing the backported 1.24 tarball, there's something weird with ScopedCallback never resetting the libxml flag... oh, hey, in 1.24, ScopedCallback doesn't pass parameters to the callback, just silently drops them.

Change 201018 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow directly calling Xml::isWellFormed

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

Change 201019 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow entities in XMP

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

Change 201029 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow directly calling Xml::isWellFormed

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

Change 201030 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow entities in XMP

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

Change 201039 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow entities in XMP

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

Change 201018 merged by jenkins-bot:
SECURITY: Don't allow directly calling Xml::isWellFormed

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

Change 201019 merged by jenkins-bot:
SECURITY: Don't allow entities in XMP

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

Change 201029 merged by jenkins-bot:
SECURITY: Don't allow directly calling Xml::isWellFormed

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

Change 201030 merged by jenkins-bot:
SECURITY: Don't allow entities in XMP

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

Change 201039 merged by jenkins-bot:
SECURITY: Don't allow entities in XMP

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

Change 201224 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow directly calling Xml::isWellFormed

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

Change 201225 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow entities in XMP with HHVM

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

Change 201224 merged by jenkins-bot:
SECURITY: Don't allow directly calling Xml::isWellFormed

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

Change 201225 merged by jenkins-bot:
SECURITY: Don't allow entities in XMP with HHVM

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