patch for webservices XXE/XEE vulnerability #2177

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants

solves #2174

Wouldn't it be better to remember the old value?

$oldValue = libxml_disable_entity_loader( true );

And here to restore it?

libxml_disable_entity_loader( $oldValue );

Don't think it should modify the public interface of the web service class. If you really want to test it separatly, there are ways to do it with a bit extra work on the test side.

For example:

  • test it indirectly through the calling run() method
  • for the test, use a wrapper class that inherits from CWebService and provides a public method that delegates to filterRequest() (given you changed filterRequest() to protected)
  • use reflection to modify accessibility of the method during unit test run (requires php >= 5.3.2)

Not entirely sure about this. Should work, as SOAP always uses POST requests afaik and according to the php manual, you typically can re-read php://input [1]. However, it might be better to use Yii::app()->request->rawBody, just for the sake of consistency when accessing input data...


[1] http://us.php.net/manual/en/wrappers.php.php#refsect2-wrappers.php-unknown-unknown-descriptiop

Consider a closing else clause:

else {
  throw new CException( 'Unsupported parameter type.' );
}
Owner

redguy666 commented on d66cc6a Mar 8, 2013

ok. I agree with those notes.

bwoester commented Mar 8, 2013

Guess that's it from me, can't find any more. 😉

damn.. one more thing - I have to add line to CHANGELOG

Owner

samdark commented Mar 8, 2013

@yiisoft/core-developers any comments on this one?

bwoester commented Mar 9, 2013

Seems like php's soap server already disables entity loading when parsing the requests (see [1], called from [2]). Maybe someone can look up since when this behavior is implemented? If it was already available in PHP 5.1, we don't need to do any extra work.

Are there any other locations in the framework that are meant to parse xml provided by a user? Those might be affected...


[1] http://git.php.net/?p=php-src.git;a=blob;f=ext/soap/php_xml.c;h=939385557df326205e62d7d4ae343da1fa6ab02f;hb=HEAD#l153
[2] http://git.php.net/?p=php-src.git;a=blob;f=ext/soap/soap.c;h=7df84e5b2a6c960d3e044f83d55b4e02411468cf;hb=HEAD#l1605

as you can see here: http://git.php.net/?p=php-src.git;a=history;f=ext/soap/php_xml.c;h=939385557df326205e62d7d4ae343da1fa6ab02f;hb=HEAD
"Disabled external entities loading" commit was introduced at 2013-02-07. There is a reason Zend Framework introduced same soap request filtering procedure last year (http://framework.zend.com/code/log.php?repname=Zend+Framework&path=%2Ftrunk%2Flibrary%2FZend%2FSoap%2FServer.php&rev=25031&peg=25176), and I think PHP 5.1 is a little bit older...

+ foreach($dom->childNodes as $child)
+ {
+ if ($child->nodeType === XML_DOCUMENT_TYPE_NODE)
+ throw new CException('Invalid XML: Detected use of illegal DOCTYPE');
@resurtm

resurtm Mar 28, 2013

Contributor

I think the following code would be better:

throw new CException(Yii::t('yii','Invalid XML: Detected use of illegal DOCTYPE.')); // note trailing dot

Yii's common approach is to localize exception messages too.

+ libxml_disable_entity_loader($oldValue);
+ }
+ else
+ throw new CException('Unsupported parameter type.');
@resurtm

resurtm Mar 28, 2013

Contributor
throw new CException(Yii::t('yii','Unsupported parameter type.'));
Contributor

resurtm commented Mar 28, 2013

At first glance looks good to me too. Will try to test it with the real application soon.

@ghost ghost assigned resurtm Mar 28, 2013

+ {
+ //when request is already parsed - there is nothing to do, we only need to save it as string
+ if($request instanceof DOMDocument)
+ $xml = $request->saveXML();
@resurtm

resurtm Mar 28, 2013

Contributor

$xml = $request->saveXML();$xml=$request->saveXML();. Please fix all the other CS and consistency issues in your patch. We're trying to stick to agreements as strictly as possible, isn't it? :-)

Sure - will prepare additional commit with changes, however I am still investigating this issue, as it seems that PHP soap extension does not allow <DOCTYPE> and throws "DTD are not supported by SOAP". So maybe SOAP is in fact not vulnerable. Also - I checked other modules of Yii and there are no other XML parsing codes so maybe this patch is not necessary. And if so - why did Zend coders added such patch?

Owner

samdark commented May 27, 2013

@redguy666 any news on this one? We're preparing for 1.1.14 RC and it would be great to include your pull request as well.

Closing this issue as SOAP implementation is not vulnerable and there are no other XML parsing related threats in Yii.

@redguy666 redguy666 closed this Jun 4, 2013

Contributor

resurtm commented Jun 6, 2013

@redguy666, hm, have you tested it on a real web server? How did you come to this conclusion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment