From 23682de65e787704b1ea1ad199d13f9738555e57 Mon Sep 17 00:00:00 2001 From: csteipp Date: Tue, 10 Feb 2015 17:32:48 -0800 Subject: [PATCH] SECURITY: Check limits before parsing XML To prevent various DoS attacks in our XML parsing, check that PHP has sane time and memory limits set before parsing when XML entities are expanded. Users can set $wgAllowUnsafeLimits = true if they want to use these features without setting any limits. Bug: T85848 Change-Id: Ib95edc9c4ad2247f2b58016e8128bcbf9109af6d --- includes/DefaultSettings.php | 7 +++++++ includes/GlobalFunctions.php | 15 +++++++++++++++ includes/Setup.php | 6 ++++++ includes/libs/XmlReaderTypeCheck.php | 10 ++++++++++ includes/libs/XmlTypeCheck.php | 8 ++++++++ includes/media/SVGMetadataExtractor.php | 8 ++++++++ includes/media/XMP.php | 8 ++++++++ 7 files changed, 62 insertions(+) diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index f9dc1ed..c38ee7d 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -7404,6 +7404,13 @@ $wgPageLanguageUseDB = false; $wgUseLinkNamespaceDBFields = true; /** + * Allow processing user input that could potentially consume significant resources, without + * ensuring that sane limits are set by PHP. + * @var bool + */ +$wgAllowUnsafeLimits = false; + +/** * For really cool vim folding this needs to be at the end: * vim: foldmarker=@{,@} foldmethod=marker * @} diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 2025e17..65241a2 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -4198,3 +4198,18 @@ function wfIsConfiguredProxy( $ip ) { wfDeprecated( __METHOD__, '1.24' ); return IP::isConfiguredProxy( $ip ); } + +/** + * Check resource limits, unless wgAllowUnsafeLimits is set. + * @return bool + */ +function wfCheckPhpLimits() { + global $wgCommandLineMode, $wgAllowUnsafeLimits; + if ( !$wgCommandLineMode + && !$wgAllowUnsafeLimits + && ( ini_get( 'memory_limit' ) === '-1' || ini_get( 'max_execution_time' ) === '0' ) + ) { + return false; + } + return true; +} diff --git a/includes/Setup.php b/includes/Setup.php index ed0aabe..a82b0a9 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -675,6 +675,12 @@ foreach ( $wgExtensionFunctions as $func ) { wfDebug( "Fully initialised\n" ); $wgFullyInitialised = true; +if ( !wfCheckPhpLimits() ) { + wfWarn( 'Running without memory_limit or max_execution_time defined might allow ' . + 'some DoS attacks to succeed.' ); +} + + Profiler::instance()->scopedProfileOut( $ps_extensions ); Profiler::instance()->scopedProfileOut( $ps_setup ); diff --git a/includes/libs/XmlReaderTypeCheck.php b/includes/libs/XmlReaderTypeCheck.php index e859bac..37cea9e 100644 --- a/includes/libs/XmlReaderTypeCheck.php +++ b/includes/libs/XmlReaderTypeCheck.php @@ -77,10 +77,20 @@ class XmlReaderTypeCheck { * filename (default, true) or if it is a string (false) * @param array $options list of additional parsing options: * processing_instruction_handler: Callback for xml_set_processing_instruction_handler + * @throws MWException */ function __construct( $input, $filterCallback = null, $isFile = true, $options = array() ) { $this->filterCallback = $filterCallback ?: function() { return false; } ; $this->parserOptions = array_merge( $this->parserOptions, $options ); + + // Since we intentionally expand entities, make sure sane resource limits + // are setup - T85848 + if ( !wfCheckPhpLimits() ) { + throw new MWException( 'Please set PHP\'s memory_limit and max_execution_time ' . + 'to safely use this function, or set wgAllowUnsafeLimits to disable '. + 'this check.' ); + } + $this->validateFromInput( $input, $isFile ); } diff --git a/includes/libs/XmlTypeCheck.php b/includes/libs/XmlTypeCheck.php index 0d6c3a6..6d2fecf 100644 --- a/includes/libs/XmlTypeCheck.php +++ b/includes/libs/XmlTypeCheck.php @@ -74,11 +74,19 @@ class XmlTypeCheck { * filename (default, true) or if it is a string (false) * @param array $options list of additional parsing options: * processing_instruction_handler: Callback for xml_set_processing_instruction_handler + * @throws MWException */ function __construct( $input, $filterCallback = null, $isFile = true, $options = array() ) { $this->filterCallback = $filterCallback; $this->parserOptions = array_merge( $this->parserOptions, $options ); + // If we expands entities, make sure sane resource limits are set - T85848 + if ( !wfCheckPhpLimits() ) { + throw new MWException( 'Please set PHP\'s memory_limit and max_execution_time ' . + 'to safely use this function, or set wgAllowUnsafeLimits to disable '. + 'this check.' ); + } + if ( $isFile ) { $this->validateFromFile( $input ); } else { diff --git a/includes/media/SVGMetadataExtractor.php b/includes/media/SVGMetadataExtractor.php index 2037c33..21152d5 100644 --- a/includes/media/SVGMetadataExtractor.php +++ b/includes/media/SVGMetadataExtractor.php @@ -74,6 +74,14 @@ class SVGReader { throw new MWException( "Error getting filesize of SVG." ); } + // Since we intentionally expand entities, make sure sane resource limits + // are setup - T85848 + if ( !wfCheckPhpLimits() ) { + throw new MWException( 'Please set PHP\'s memory_limit and max_execution_time ' . + 'to safely use this function, or set wgAllowUnsafeLimits to disable '. + 'this check.' ); + } + if ( $size > $wgSVGMetadataCutoff ) { $this->debug( "SVG is $size bytes, which is bigger than $wgSVGMetadataCutoff. Truncating." ); $contents = file_get_contents( $source, false, null, -1, $wgSVGMetadataCutoff ); diff --git a/includes/media/XMP.php b/includes/media/XMP.php index 0d341aa..1714731 100644 --- a/includes/media/XMP.php +++ b/includes/media/XMP.php @@ -112,6 +112,7 @@ class XMPReader { * Constructor. * * Primary job is to initialize the XMLParser + * @throws MWException */ function __construct() { @@ -120,6 +121,13 @@ class XMPReader { throw new MWException( 'XMP support requires XML Parser' ); } + // If xml_parse expands entities, make sure sane resource limits are set - T85848 + if ( !wfCheckPhpLimits() ) { + throw new MWException( 'Please set PHP\'s memory_limit and max_execution_time ' . + 'to safely use this function, or set wgAllowUnsafeLimits to disable '. + 'this check.' ); + } + $this->items = XMPInfo::getItems(); $this->resetXMLParser(); -- 1.8.4.5