Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patch for webservices XXE/XEE vulnerability #2177

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Version 1.1.14 work in progress
- Bug #2086: Fixed .hgignore rule for assets folder (GeXu3, Koduc)
- Bug #2112: Fixed broken yiic shell CRUD command (mbischof)
- Bug #2146: CEmailValidator fix when fsockopen() can output uncatched error 'Connection refused (61)' (armab)
- Bug #2174: CWebService is vulnerable to XXE/XEE attacks (redguy)
- Enh: Better CFileLogRoute performance (Qiang, samdark)
- Enh: Refactored CHttpRequest::getDelete and CHttpRequest::getPut not to use _restParams directly (samdark)
- Enh #1847: Added COutputCache::varyByLanguage to generate separate cache for different languages (Obramko)
Expand Down
55 changes: 53 additions & 2 deletions framework/web/services/CWebService.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ class CWebService extends CComponent
* @since 1.1.12
*/
public $generatorConfig='CWsdlGenerator';
/**
* @var boolean whether to check requests for XXE/XEE attacks
* @since 1.1.14
*/
public $filterRequests=true;

private $_method;

Expand Down Expand Up @@ -153,7 +158,49 @@ public function generateWsdl()
$cache->set($key,$wsdl,$this->wsdlCacheDuration);
return $wsdl;
}

/**
* Prepares request XML and in case of string serialized XML parses and seeks for DOCTYPE
* tags which may contain danger XML entities definition.
* Code adapted from Zend_Soap_Server by Zend
* @param mixed $request request data
* @return string request XML as string
*/
protected function filterRequest($request)
{
//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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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? :-)

elseif($request instanceof DOMNode)
$xml = $request->ownerDocument->saveXML();
elseif($request instanceof SimpleXMLElement)
$xml = $request->asXML();
elseif(is_object($request) || is_string($request))
{
//when request is passed as string or unknown object class - we check it for DOCTYPE tags
if(is_object($request))
$xml = $request->__toString();
else
$xml = $request;

//disabling entity loader is crucial to prevent XXE/XEE checks or we will fall into same trap
$oldValue=libxml_disable_entity_loader(true);
$dom = new DOMDocument();
if(strlen($xml)==0 || !$dom->loadXML($xml))
throw new CException('Invalid XML: Parse error');

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return $xml;
}

/**
* Handles the web service request.
*/
Expand All @@ -178,16 +225,20 @@ public function run()
else
$server->setClass('CSoapObjectWrapper',$provider);

$request=Yii:app()->request->rawBody;
if($this->filterRequests)
$request=$this->filterRequest($request);

if($provider instanceof IWebServiceProvider)
{
if($provider->beforeWebMethod($this))
{
$server->handle();
$server->handle($request);
$provider->afterWebMethod($this);
}
}
else
$server->handle();
$server->handle($request);
}
catch(Exception $e)
{
Expand Down