Fix CWE errors #58
Fix CWE errors #58
Conversation
Hello. Yes, I just created it. :) |
|
Excellent! Thx. |
| $ret_val = ob_get_contents(); | ||
| ob_end_clean(); | ||
| return $ret_val; | ||
| } | ||
|
|
||
| function sanitize($value) { | ||
| return htmlspecialchars(strip_tags($value), ENT_COMPAT, 'utf-8'); |
There was a problem hiding this comment.
Running both strip_tags and htmlspecialchars doesn't make much sense. In general, trying to write an all-purpose "sanitize" method is a bad idea: escaping should be related to the context where you're outputting or using something.
| echo '<input type="hidden" name="proxyhost" value="' . $client->sanitize($proxyhost) .'">'; | ||
| echo '<input type="hidden" name="proxyport" value="' . $client->sanitize($proxyport) .'">'; | ||
| echo '<input type="hidden" name="proxyusername" value="' . $client->sanitize($proxyusername) .'">'; | ||
| echo '<input type="hidden" name="proxypassword" value="' . $client->sanitize($proxypassword) .'">'; |
There was a problem hiding this comment.
This should probably just use htmlspecialchars directly, because we shouldn't be changing the value, just escaping it for the browser.
| @@ -908,11 +908,15 @@ function getmicrotime() | |||
| function varDump($data) | |||
| { | |||
| ob_start(); | |||
| var_dump($data); | |||
| var_dump($this->sanitize($data)); | |||
There was a problem hiding this comment.
$data here is not guaranteed to be a string. We also don't have an output context here, so escaping is inappropriate - it might never be displayed in an XML or HTML context.
| @@ -2714,13 +2718,13 @@ function setCredentials($username, $password, $authtype = 'basic', $digestReques | |||
| $A1 = $username . ':' . (isset($digestRequest['realm']) ? $digestRequest['realm'] : '') . ':' . $password; | |||
|
|
|||
| // H(A1) = MD5(A1) | |||
| $HA1 = md5($A1); | |||
| $HA1 = password_hash($A1, PASSWORD_DEFAULT); | |||
There was a problem hiding this comment.
Digest Authentication is specified to use MD5 hashes, we can't simply substitute a different algorithm.
| @@ -8433,7 +8410,7 @@ function __construct($cache_dir='.', $cache_lifetime=0) { | |||
| * @access private | |||
| */ | |||
| function createFilename($wsdl) { | |||
| return $this->cache_dir.'/wsdlcache-' . md5($wsdl); | |||
| return $this->cache_dir.'/wsdlcache-' . password_hash($wsdl, PASSWORD_DEFAULT); | |||
There was a problem hiding this comment.
This is not a security hash, so password_hash is inappropriate - password hashing is designed to be slow, but in this case we want something fast. md5 is still perfectly appropriate for this kind of use.
| @@ -8497,15 +8474,15 @@ function get($wsdl) { | |||
| * @access private | |||
| */ | |||
| function obtainMutex($filename, $mode) { | |||
| if (isset($this->fplock[md5($filename)])) { | |||
| if (isset($this->fplock[password_hash($filename, PASSWORD_DEFAULT)])) { | |||
There was a problem hiding this comment.
As above, this is not a secure hash, md5 is fine.
| } | ||
| } elseif ($this->wsdl) { | ||
| $this->debug("In service, return Web description"); | ||
| print $this->wsdl->webDescription(); | ||
| print $this->sanitize($this->wsdl->webDescription()); |
There was a problem hiding this comment.
This is a huge breaking change: HTML documentation can be embedded in WSDL files, and this completely breaks its display.
|
Because this PR introduced some unexpected behaviour and revert it and bump v0.9.10 (https://github.com/pwnlabs/nusoap/releases/tag/v0.9.10). We need to dig deeper. |
Fixes some vulnerabilities pointed out by Veracode.