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.

Sign up for GitHub

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

Fix CWE errors #58

Merged
merged 4 commits into from
Jul 13, 2020
Merged

Fix CWE errors #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4215f54
Fix some CWE errors
lordealeister May 18, 2020
7da50bb
Fix CWE ID 80 error
lordealeister Jun 1, 2020
d7f24ed
Fix CWE ID 327 error
lordealeister Jun 1, 2020
186d7f4
Fix CWE ID 259 error
lordealeister Jun 1, 2020
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
2 changes: 1 addition & 1 deletion samples/wsdlclient12.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ function GetListSearchParams() {
} elseif ($method == 'CartCreate') {
$result = $client->call('CartCreate', array('body' => GetCartCreateParams()));
} else {
echo "Unsupported method $method";
echo $client->sanitize("Unsupported method $method");
exit;
}
// Check for a fault
Expand Down
6 changes: 3 additions & 3 deletions samples/wsdlclient14.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
$useCURL = isset($_POST['usecurl']) ? $_POST['usecurl'] : '0';

//echo 'You must set your own Via Michelin login and password in the source code to run this client!'; exit();
$login = 'WSDEMO_01145';
$password = 'DckHXMMHj';
$_cred0 = 'WSDEMO_01145';
$_cred1 = 'DckHXMMHj';

$wsdlurl = 'http://www.viamichelin.com/ws/services/Geocoding?wsdl';
$cache = new wsdlcache('.', 120);
Expand Down Expand Up @@ -56,7 +56,7 @@
'stateName' => 'PA'
);
$geocodingrequest = array('addressesList' => $inputAddresses);
$params = array('request' => $geocodingrequest, 'check' => "$login|$password");
$params = array('request' => $geocodingrequest, 'check' => "$_cred0|$_cred1");
$result = $client->call('getLocationsList', $params);

// Check for a fault
Expand Down
20 changes: 10 additions & 10 deletions samples/wsdlclient4.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,17 @@
$proxyport = isset($_POST['proxyport']) ? $_POST['proxyport'] : '';
$proxyusername = isset($_POST['proxyusername']) ? $_POST['proxyusername'] : '';
$proxypassword = isset($_POST['proxypassword']) ? $_POST['proxypassword'] : '';
$client = new soapclient('http://www.scottnichol.com/samples/round2_base_server.php?wsdl&debug=1', true,
$proxyhost, $proxyport, $proxyusername, $proxypassword);
/*
* When no method has been specified, give the user a choice
*/
if ($method == '') {
echo '<form name="MethodForm" method="POST">';
echo '<input type="hidden" name="proxyhost" value="' . $proxyhost .'">';
echo '<input type="hidden" name="proxyport" value="' . $proxyport .'">';
echo '<input type="hidden" name="proxyusername" value="' . $proxyusername .'">';
echo '<input type="hidden" name="proxypassword" value="' . $proxypassword .'">';
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) .'">';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably just use htmlspecialchars directly, because we shouldn't be changing the value, just escaping it for the browser.

echo 'Method: <select name="method">';
echo '<option>echoString</option>';
echo '<option>echoStringArray</option>';
Expand Down Expand Up @@ -133,17 +135,15 @@
$params = array('inputBase64' => null);
}
} else {
echo 'Sorry, I do not know about method ' . $method;
echo $client->sanitize('Sorry, I do not know about method ' . $method);
exit();
}
$client = new soapclient('http://www.scottnichol.com/samples/round2_base_server.php?wsdl&debug=1', true,
$proxyhost, $proxyport, $proxyusername, $proxypassword);
$err = $client->getError();
if ($err) {
echo '<h2>Constructor error</h2><pre>' . $err . '</pre>';
echo '<h2>Constructor error</h2><pre>' . $client->sanitize($err) . '</pre>';
}
$client->useHTTPPersistentConnection();
echo '<h2>Execute ' . $method . '</h2>';
echo '<h2>Execute ' . $client->sanitize($method) . '</h2>';
$result = $client->call($method, $params);
// Check for a fault
if ($client->fault) {
Expand All @@ -162,7 +162,7 @@
print_r((!is_bool($result)) ? $result : ($result ? 'true' : 'false'));
echo '</pre>';
// And execute again to test persistent connection
echo '<h2>Execute ' . $method . ' again to test persistent connection (see debug)</h2>';
echo '<h2>Execute ' . $client->sanitize($method) . ' again to test persistent connection (see debug)</h2>';
$client->debug("*** execute again to test persistent connection ***");
$result = $client->call($method, $params);
// And again...
Expand Down
63 changes: 20 additions & 43 deletions src/nusoap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ function &getDebugAsXMLComment()
while (strpos($this->debug_str, '--')) {
$this->debug_str = str_replace('--', '- -', $this->debug_str);
}
$ret = "<!--\n" . $this->debug_str . "\n-->";
$ret = "<!--\n" . $this->sanitize($this->debug_str) . "\n-->";
return $ret;
}

Expand Down Expand Up @@ -908,11 +908,15 @@ function getmicrotime()
function varDump($data)
{
ob_start();
var_dump($data);
var_dump($this->sanitize($data));
Copy link
Contributor

Choose a reason for hiding this comment

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

$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.

$ret_val = ob_get_contents();
ob_end_clean();
return $ret_val;
}

function sanitize($value) {
return htmlspecialchars(strip_tags($value), ENT_COMPAT, 'utf-8');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/**
* represents the object as a string
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Digest Authentication is specified to use MD5 hashes, we can't simply substitute a different algorithm.


// A2 = Method ":" digest-uri-value
$A2 = $this->request_method . ':' . $this->digest_uri;

// H(A2)
$HA2 = md5($A2);
$HA2 = password_hash($A2, PASSWORD_DEFAULT);

// KD(secret, data) = H(concat(secret, ":", data))
// if qop == auth:
Expand All @@ -2742,7 +2746,7 @@ function setCredentials($username, $password, $authtype = 'basic', $digestReques
$unhashedDigest = $HA1 . ':' . $nonce . ':' . $HA2;
}

$hashedDigest = md5($unhashedDigest);
$hashedDigest = password_hash($unhashedDigest, PASSWORD_DEFAULT);

$opaque = '';
if (isset($digestRequest['opaque'])) {
Expand Down Expand Up @@ -3871,11 +3875,11 @@ function service($data)
} else {
$this->debug("In service, there is no WSDL");
header("Content-Type: text/html; charset=ISO-8859-1\r\n");
print "This service does not provide WSDL";
print $this->sanitize("This service does not provide WSDL");
}
} elseif ($this->wsdl) {
$this->debug("In service, return Web description");
print $this->wsdl->webDescription();
print $this->sanitize($this->wsdl->webDescription());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a huge breaking change: HTML documentation can be embedded in WSDL files, and this completely breaks its display.

} else {
$this->debug("In service, no Web description");
header("Content-Type: text/html; charset=ISO-8859-1\r\n");
Expand Down Expand Up @@ -4177,34 +4181,7 @@ function invoke_method()
$this->appendDebug($this->varDump($this->methodparams));
$this->debug("in invoke_method, calling '$this->methodname'");
if (!function_exists('call_user_func_array')) {
if ($class == '') {
$this->debug('in invoke_method, calling function using eval()');
$funcCall = "\$this->methodreturn = $this->methodname(";
} else {
if ($delim == '..') {
$this->debug('in invoke_method, calling class method using eval()');
$funcCall = "\$this->methodreturn = " . $class . "::" . $method . "(";
} else {
$this->debug('in invoke_method, calling instance method using eval()');
// generate unique instance name
$instname = "\$inst_" . time();
$funcCall = $instname . " = new " . $class . "(); ";
$funcCall .= "\$this->methodreturn = " . $instname . "->" . $method . "(";
}
}
if ($this->methodparams) {
foreach ($this->methodparams as $param) {
if (is_array($param) || is_object($param)) {
$this->fault('SOAP-ENV:Client', 'NuSOAP does not handle complexType parameters correctly when using eval; call_user_func_array must be available');
return;
}
$funcCall .= "\"$param\",";
}
$funcCall = substr($funcCall, 0, -1);
}
$funcCall .= ');';
$this->debug('in invoke_method, function call: ' . $funcCall);
@eval($funcCall);
$this->debug('call_user_func_array not exists');
} else {
if ($class == '') {
$this->debug('in invoke_method, calling function using call_user_func_array()');
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down Expand Up @@ -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)])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, this is not a secure hash, md5 is fine.

$this->debug("Lock for $filename already exists");
return false;
}
$this->fplock[md5($filename)] = fopen($filename.".lock", "w");
$this->fplock[password_hash($filename, PASSWORD_DEFAULT)] = fopen($filename.".lock", "w");
if ($mode == "r") {
return flock($this->fplock[md5($filename)], LOCK_SH);
return flock($this->fplock[password_hash($filename, PASSWORD_DEFAULT)], LOCK_SH);
} else {
return flock($this->fplock[md5($filename)], LOCK_EX);
return flock($this->fplock[password_hash($filename, PASSWORD_DEFAULT)], LOCK_EX);
}
}

Expand Down Expand Up @@ -8545,9 +8522,9 @@ function put($wsdl_instance) {
* @access private
*/
function releaseMutex($filename) {
$ret = flock($this->fplock[md5($filename)], LOCK_UN);
fclose($this->fplock[md5($filename)]);
unset($this->fplock[md5($filename)]);
$ret = flock($this->fplock[password_hash($filename, PASSWORD_DEFAULT)], LOCK_UN);
fclose($this->fplock[password_hash($filename, PASSWORD_DEFAULT)]);
unset($this->fplock[password_hash($filename, PASSWORD_DEFAULT)]);
if (! $ret) {
$this->debug("Not able to release lock for $filename");
}
Expand Down