From 8b9810e8819cb90628cfaee5594c900994e2e044 Mon Sep 17 00:00:00 2001 From: csteipp Date: Fri, 27 Feb 2015 13:38:37 -0800 Subject: [PATCH] SECURITY: Don't use xml_parse Convert MimeMagic and XMPReader to use XMLReader for parsing xml instead of xml_parse. Adds a SafeXmlParser which approximates SAX parsing using XMLReader, which should be safe for any input. Bug: T85848 Change-Id: Idb7770b74c7d2babc3df2c479c325ec899298e09 --- autoload.php | 1 + includes/MimeMagic.php | 2 +- includes/libs/SafeXmlParser.php | 322 ++++++++++++++++++++++ includes/media/XMP.php | 50 ++-- tests/phpunit/includes/libs/SafeXmlParserTest.php | 212 ++++++++++++++ 5 files changed, 568 insertions(+), 19 deletions(-) create mode 100644 includes/libs/SafeXmlParser.php create mode 100644 tests/phpunit/includes/libs/SafeXmlParserTest.php diff --git a/autoload.php b/autoload.php index 5facd2c..7d01982 100644 --- a/autoload.php +++ b/autoload.php @@ -1017,6 +1017,7 @@ $wgAutoloadLocalClasses = array( 'SQLiteField' => __DIR__ . '/includes/db/DatabaseSqlite.php', 'SVGMetadataExtractor' => __DIR__ . '/includes/media/SVGMetadataExtractor.php', 'SVGReader' => __DIR__ . '/includes/media/SVGMetadataExtractor.php', + 'SafeXmlParser' => __DIR__ . '/includes/libs/SafeXmlParser.php', 'Sanitizer' => __DIR__ . '/includes/Sanitizer.php', 'SavepointPostgres' => __DIR__ . '/includes/db/DatabasePostgres.php', 'ScopedCallback' => __DIR__ . '/includes/libs/ScopedCallback.php', diff --git a/includes/MimeMagic.php b/includes/MimeMagic.php index ebe98a3..6ca950e 100644 --- a/includes/MimeMagic.php +++ b/includes/MimeMagic.php @@ -724,7 +724,7 @@ class MimeMagic { /** * look for XML formats (XHTML and SVG) */ - $xml = new XmlTypeCheck( $file ); + $xml = new XmlReaderTypeCheck( $file ); if ( $xml->wellFormed ) { $xmlMimeTypes = $this->mConfig->get( 'XMLMimeTypes' ); if ( isset( $xmlMimeTypes[$xml->getRootElement()] ) ) { diff --git a/includes/libs/SafeXmlParser.php b/includes/libs/SafeXmlParser.php new file mode 100644 index 0000000..a7d0db4 --- /dev/null +++ b/includes/libs/SafeXmlParser.php @@ -0,0 +1,322 @@ +reader = new XMLReader(); + $this->addNS = $addNS; + $this->nsSeparator = $nsSeparator; + } + + public function __destroy() { + $this->reader->close(); + } + + /** + * make names uppercase + * @param bool + */ + public function setCaseFolding( $caseFolding ) { + $this->caseFolding = $caseFolding; + } + + /** + * It seems there isn't a good definition of what this is actually supposed to + * do, it seems to be broken for some versions of expat, and "XML_OPTION_SKIP_WHITE skips + * almost nothing." - https://bugs.php.net/bug.php?id=33240 + * Leaving the switch here in case we need it, but it doesn't do anything in this code. + * @param bool + */ + public function setSkipWhitespace( $skipWhitespace ) { + $this->skipWhitespace = $skipWhitespace; + } + + /** + * Did we encounter any errors during processing + * @return bool + */ + public function errors() { + return ( $this->xmlError !== false ); + } + + /** + * Did we encounter any errors during processing + * @return bool + */ + public function getError() { + list ( $errno, $errstr ) = $this->xmlError; + return $errstr; + } + + /** + * Equivalent to xml_set_element_handler( $parser, $openHandler, $closeHandler ) + */ + public function setElementHandler( $openHandler, $closeHandler ) { + $this->openHandler = $openHandler; + $this->closeHandler = $closeHandler; + + if ( ( $openHandler && !is_callable( $openHandler ) ) + || ( $closeHandler && !is_callable( $closeHandler ) ) + ) { + throw new Exception( "Bad handler set in " . __METHOD__ ); + } + } + + /** + * Equivalent to xml_set_processing_instruction_handler( $parser, $piHandler ) + */ + public function setProcessingInstructionHandler( $piHandler ) { + $this->piHandler = $piHandler; + if ( $piHandler && !is_callable( $piHandler ) ) { + throw new Exception( "Bad handler set in " . __METHOD__ ); + } + } + + /** + * Equivalent to xml_set_character_data_handler( $parser, $dataHandler ) + */ + public function setDataHandler( $dataHandler ) { + $this->dataHandler = $dataHandler; + if ( $dataHandler && !is_callable( $dataHandler ) ) { + throw new Exception( "Bad handler set in " . __METHOD__ ); + } + } + + /** + * Parse a block of XML. Equivalent to xml_parse( $parser, $xml ). + * @param string $xml + */ + public function parse( $xml ) { + + // XMLReader::XML fails on partial documents, so use open + $this->reader->open( + 'data://text/plain,' . urlencode( $xml ), + null, + LIBXML_NOERROR | LIBXML_NOWARNING | LIBXML_NONET + ); + $oldDisable = libxml_disable_entity_loader( true ); + $this->reader->setParserProperty( XMLReader::SUBST_ENTITIES, true ); + + while ( $this->readNext() ) { + + switch ( $this->reader->nodeType ) { + case XMLReader::ELEMENT: + $name = $this->expandNS( + $this->reader->name, + $this->reader->localName, + $this->reader->namespaceURI + ); + $empty = $this->reader->isEmptyElement; + $attrs = $this->getAttributesArray( $this->reader ); + $this->elementOpen( $name, $attrs ); + if ( $empty ) { + $this->elementClose( $name ); + } + break; + + case XMLReader::END_ELEMENT: + $name = $this->expandNS( + $this->reader->name, + $this->reader->localName, + $this->reader->namespaceURI + ); + $this->elementClose( $name ); + break; + + case XMLReader::WHITESPACE: + case XMLReader::SIGNIFICANT_WHITESPACE: + case XMLReader::CDATA: + case XMLReader::TEXT: + $this->elementData( $this->reader->value ); + break; + + case XMLReader::PI: + // Processing instructions can happen after the header too + $this->processingInstruction( + $this->reader->name, + $this->reader->value + ); + break; + + default: + // One of DOC, DOC_TYPE, ENTITY, END_ENTITY, + // NOTATION, or XML_DECLARATION, COMMENT, ENTITY_REF + // xml_parse didn't send these to the filter, so we won't. + } + } + + libxml_disable_entity_loader( $oldDisable ); + } + + /** + * Advance the XMLReader to the next element. We set the error_handler on each call, since + * parse calls code we don't control, which could update the error handler too. + */ + private function readNext() { + set_error_handler ( array( $this, 'XmlErrorHandler' ) ); + $ret = $this->reader->read(); + restore_error_handler(); + return $ret; + } + + /** + * Callback for error handler. You shouldn't call this in practice. + */ + public function XmlErrorHandler( $errno, $errstr ) { + $this->xmlError = array( $errno, $errstr ); + } + + /** + * @param $name element or attribute name, maybe with a full or short prefix + * @param $localname the local name, according to XMLReader + * @param $namespaceURI the namespaceURI + * @return string the name prefixed with namespaceURI + */ + private function expandNS( $name, $localname, $namespaceURI ) { + $ret = false; + if ( !$this->addNS ) { + $ret = $localname; + } elseif ( $namespaceURI ) { + $parts = explode( ':', $name ); + $localname = array_pop( $parts ); + $ret = $namespaceURI . $this->nsSeparator . $localname; + } else { + $ret = $name; + } + + if ( $this->caseFolding ) { + // names can be multibyte (http://www.w3.org/TR/REC-xml/#NT-NameStartChar) + $ret = mb_strtoupper( $ret ); + } + + return $ret; + } + + /** + * Get all of the attributes for an XMLReader's current node + * @param $r XMLReader + * @return array of attributes + */ + private function getAttributesArray( XMLReader $r ) { + $attrs = array(); + while ( $r->moveToNextAttribute() ) { + if ( $r->namespaceURI === 'http://www.w3.org/2000/xmlns/' ) { + // XMLReader treats xmlns attributes as normal + // attributes, while xml_parse doesn't + continue; + } + $name = $this->expandNS( $r->name, $r->localName, $r->namespaceURI ); + $attrs[$name] = $r->value; + } + return $attrs; + } + + /** + * @param $name + * @param $attribs + */ + private function elementOpen( $name, $attribs ) { + if ( is_callable( $this->openHandler ) ) { + // The function named by start_element_handler must accept three parameters: + // start_element_handler ( resource $parser , string $name , array $attribs ) + call_user_func( + $this->openHandler, + null, + $name, + $attribs + ); + } + } + + /** + * @param $name + * @param $attribs + */ + private function elementClose( $name ) { + if ( is_callable( $this->closeHandler ) ) { + // The function named by end_element_handler must accept two parameters: + // end_element_handler ( resource $parser , string $name ) + call_user_func( + $this->closeHandler, + null, + $name + ); + } + } + + /** + * @param $name + * @param $attribs + */ + private function elementData( $data ) { + if ( is_callable( $this->dataHandler ) ) { + // The function named by handler must accept two parameters: + // handler ( resource $parser , string $data ) + call_user_func( + $this->dataHandler, + null, + $data + ); + } + } + + /** + * @param $name + * @param $attribs + */ + private function processingInstruction( $target, $data ) { + if ( is_callable( $this->piHandler ) ) { + // The function named by handler must accept three parameters: + // handler ( resource $parser , string $target , string $data ) + call_user_func( + $this->piHandler, + null, + $target, + $data + ); + } + } +} diff --git a/includes/media/XMP.php b/includes/media/XMP.php index 0d341aa..fab7702 100644 --- a/includes/media/XMP.php +++ b/includes/media/XMP.php @@ -80,6 +80,9 @@ class XMPReader { /** @var int */ private $extendedXMPOffset = 0; + /** @var string **/ + private $partialXML = ''; + /** * These are various mode constants. * they are used to figure out what to do @@ -114,10 +117,9 @@ class XMPReader { * Primary job is to initialize the XMLParser */ function __construct() { - - if ( !function_exists( 'xml_parser_create_ns' ) ) { + if ( !class_exists( 'XMLReader' ) ) { // this should already be checked by this point - throw new MWException( 'XMP support requires XML Parser' ); + throw new MWException( 'XMP support requires XMLReader' ); } $this->items = XMPInfo::getItems(); @@ -133,18 +135,18 @@ class XMPReader { if ( $this->xmlParser ) { //is this needed? - xml_parser_free( $this->xmlParser ); + unset( $this->xmlParser ); } - $this->xmlParser = xml_parser_create_ns( 'UTF-8', ' ' ); - xml_parser_set_option( $this->xmlParser, XML_OPTION_CASE_FOLDING, 0 ); - xml_parser_set_option( $this->xmlParser, XML_OPTION_SKIP_WHITE, 1 ); + $this->xmlParser = new SafeXmlParser( true, ' ' ); + $this->xmlParser->setCaseFolding( 0 ); + $this->xmlParser->setSkipWhitespace( 1 ); - xml_set_element_handler( $this->xmlParser, + $this->xmlParser->setElementHandler( array( $this, 'startElement' ), array( $this, 'endElement' ) ); - xml_set_character_data_handler( $this->xmlParser, array( $this, 'char' ) ); + $this->xmlParser->setDataHandler( array( $this, 'char' ) ); } /** Destroy the xml parser @@ -153,7 +155,7 @@ class XMPReader { */ function __destruct() { // not sure if this is needed. - xml_parser_free( $this->xmlParser ); + unset( $this->xmlParser ); } /** Get the result array. Do some post-processing before returning @@ -163,6 +165,11 @@ class XMPReader { * FormatMetadata::getFormattedData(). */ public function getResults() { + + if ( $this->partialXML ) { + $this->parse( '', true ); + } + // xmp-special is for metadata that affects how stuff // is extracted. For example xmpNote:HasExtendedXMP. @@ -305,14 +312,23 @@ class XMPReader { wfRestoreWarnings(); } - $ok = xml_parse( $this->xmlParser, $content, $allOfIt ); + // Cache to make incomplete xml work with XMLReader. If getResults() is + // called before parse is called with $allOfIt = true, it will finish + // parsing any remaining xml. + if ( !$allOfIt ) { + $this->partialXML .= $content; + return true; + } + $content = $this->partialXML . $content; + $this->partialXML = ''; + + $this->xmlParser->parse( $content ); + $ok = !$this->xmlParser->errors(); + if ( !$ok ) { - $error = xml_error_string( xml_get_error_code( $this->xmlParser ) ); - $where = 'line: ' . xml_get_current_line_number( $this->xmlParser ) - . ' column: ' . xml_get_current_column_number( $this->xmlParser ) - . ' byte offset: ' . xml_get_current_byte_index( $this->xmlParser ); + $error = $this->xmlParser->getError(); - wfDebugLog( 'XMP', "XMPReader::parse : Error reading XMP content: $error ($where)" ); + wfDebugLog( 'XMP', "XMPReader::parse : Error reading XMP content: $error" ); $this->results = array(); // blank if error. return false; } @@ -409,7 +425,6 @@ class XMPReader { * @throws MWException On invalid data */ function char( $parser, $data ) { - $data = trim( $data ); if ( trim( $data ) === "" ) { return; @@ -1047,7 +1062,6 @@ class XMPReader { * @throws MWException */ function startElement( $parser, $elm, $attribs ) { - if ( $elm === self::NS_RDF . ' RDF' || $elm === 'adobe:ns:meta/ xmpmeta' || $elm === 'adobe:ns:meta/ xapmeta' diff --git a/tests/phpunit/includes/libs/SafeXmlParserTest.php b/tests/phpunit/includes/libs/SafeXmlParserTest.php new file mode 100644 index 0000000..d8521e3 --- /dev/null +++ b/tests/phpunit/includes/libs/SafeXmlParserTest.php @@ -0,0 +1,212 @@ +'; + $parser = new SafeXmlParser(); + $called = false; + $parser->setProcessingInstructionHandler( + function () use ( &$called ) { + $called = true; + } + ); + $parser->parse( $xml ); + $this->assertTrue( $called ); + } + + /** + * @covers SafeXmlParser::elementOpen + * @covers SafeXmlParser::elementClose + */ + public function testElementHandler() { + $xml = ''; + $parser = new SafeXmlParser(); + $opened = 0; + $closed = 0; + $parser->setElementHandler( + function ( $p, $e, $a ) use ( &$opened ) { + $opened += 1; + }, + function ( $p, $e ) use ( &$closed ) { + $closed += 1; + } + ); + $parser->parse( $xml ); + $this->assertEquals( 2, $opened, "number of opens" ); + $this->assertEquals( 2, $closed, "number of closes" ); + } + + /** + * @covers SafeXmlParser::parse + */ + public function testCallOrder() { + $xml = ''; + $parser = new SafeXmlParser( true ); + $opened = array(); + $parser->setElementHandler( + function ( $p, $e, $a ) use ( &$opened ) { + $opened[] = $e; + }, + false + ); + $parser->parse( $xml ); + $this->assertEquals( array( 'foo:root', 'bar:child' ), $opened ); + } + + /** + * @covers SafeXmlParser::setCaseFolding + */ + public function testCaseFolder() { + $xml = ''; + $parser = new SafeXmlParser( true, ' ' ); + $parser->setCaseFolding( true ); + $opened = array(); + $parser->setElementHandler( + function ( $p, $e, $a ) use ( &$opened ) { + $opened[] = $e; + }, + false + ); + $parser->parse( $xml ); + $this->assertEquals( array( 'FOO ROOT', 'BAR CHILD' ), $opened ); + } + + /** + * @covers SafeXmlParser::XmlErrorHandler + */ + public function testErrorHandler() { + $xml = ''; + $parser = new SafeXmlParser( true, ' ' ); + $opened = array(); + $parser->parse( $xml ); + $this->assertTrue( $parser->errors() ); + } + + /** + * @dataProvider provideComparisonXml + */ + public function testCompatOrder( $xml, $caseFolding, $skipWhitespace ) { + + $parser = new SafeXmlParser( true, ' ' ); + $parser->setCaseFolding( $caseFolding ); + $parser->setSkipWhitespace( $skipWhitespace ); + $opened = array(); + $data = ''; + $parser->setElementHandler( + function ( $p, $e, $a ) use ( &$opened ) { + $attrs = implode( '+', $a ); + $opened[] = "$e - $attrs"; + }, + false + ); + $parser->setDataHandler( + function ( $p, $d ) use ( &$data ) { + $data .= $d; + } + ); + $parser->parse( $xml ); + $valid = !$parser->errors(); + + $xmlparser = xml_parser_create_ns( 'UTF-8', ' ' ); + $xmlopened = array(); + $xmldata = ''; + xml_parser_set_option( $xmlparser, XML_OPTION_CASE_FOLDING, $caseFolding ); + xml_parser_set_option( $xmlparser, XML_OPTION_SKIP_WHITE, $skipWhitespace ); + xml_set_element_handler( $xmlparser, + function ( $p, $e, $a ) use ( &$xmlopened ) { + $attrs = implode( '+', $a ); + $xmlopened[] = "$e - $attrs"; + }, + false + ); + xml_set_character_data_handler( $xmlparser, + function ( $p, $d ) use ( &$xmldata ) { + $xmldata .= $d; + } + ); + $parsevalid = xml_parse( $xmlparser, $xml, true ); + $parsevalid = ( $parsevalid === 1 ); + + $this->assertEquals( $xmlopened, $opened, "opened: $xml" ); + $this->assertEquals( $xmldata, $data, "data: $xml" ); + $this->assertEquals( $parsevalid, $valid ); + } + + public static function provideComparisonXml() { + return array( + array( + ' ', + 0, + 0, + ), + array( + ' ', + 1, + 0, + ), + array( + ' + + + +]> +', + 1, + 0, + ), + array( + ' ', + 0, + 1, + ), + array( + 'foobarbaz', + 0, + 1, + ), + array( + ' + + + +True 0 1 False False + +', + 0, + 1, + ), + array( + ' + + a + b + c +d +e + f + g + h' & & ' + <em>Example</em> <strong>HTML B</strong> +', + 0, + true, + ), + ); + } + +} -- 1.8.4.5