Skip to content

Commit

Permalink
Bugfix for false positives
Browse files Browse the repository at this point in the history
  • Loading branch information
@phoenix128
phoenix128 committed Sep 9, 2017
1 parent 33cb73b commit 13f7ac7
Show file tree
Hide file tree
Showing 11 changed files with 387 additions and 146 deletions.
6 changes: 5 additions & 1 deletion Api/ProcessorInterface.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@

interface ProcessorInterface
{
const RES_NO_MATCH = 'no-match';
const RES_REPLACE = 'replace';
const RES_SPAWN = 'spawn';

/**
* Dig field and return true if matched
* @param string $fieldName
* @param string &$fieldValue
* @return boolean
* @return string
*/
public function processValue($fieldName, &$fieldValue);
}
5 changes: 5 additions & 0 deletions Model/Detector/Language.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ public function encodeQuery($fieldName, $fieldValue, &$threats)
'id' => static::RESCODE_SCRIPT_INJECTION,
'reason' => __('Code execution attempt'),
'regex' => [
'\\`.+?\\`' => DetectorInterface::SCORE_CRITICAL_MATCH,
'exec\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'system\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'passthru\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'popen\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'eval\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'(?:preg|ereg|eregi)_(?:replace|match|split|filter)'
. '(?:[\\w\\_]+)*\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
Expand Down
6 changes: 3 additions & 3 deletions Model/Detector/SqlInjection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ protected function encodeQuery($query, array &$threats)
$tokens = preg_split('/(\b)/', $query, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY);
foreach ($tokens as $token) {
$token = mb_strtolower(trim($token));
if (!$token || in_array($token, ['.'])) {
if (!strlen($token) || in_array($token, ['.'])) {
continue;
}

Expand Down Expand Up @@ -466,7 +466,7 @@ protected function evaluateEncodedQuery($encodedQuery, array &$threats)
'id' => static::RESCODE_SQLI_INJECTION,
'reason' => __('SQL operations injection'),
'regex' => [
'f' => DetectorInterface::SCORE_SUSPICIOUS_MATCH, // MySQL functions without opening parenthesis
// 'f' => DetectorInterface::SCORE_LOW_PROBABILITY_MATCH, // MySQL functions without opening parenthesis
'f\\(' => DetectorInterface::SCORE_CRITICAL_MATCH, // MySQL functions with opening parenthesis

's(?:o|k){0,8}y' => DetectorInterface::SCORE_CRITICAL_MATCH, // insert into tablename
Expand Down Expand Up @@ -500,7 +500,7 @@ protected function evaluateEncodedQuery($encodedQuery, array &$threats)
'id' => static::RESCODE_SQLI_INJECTION,
'reason' => __('Arguments injection'),
'regex' => [
'x\,' => DetectorInterface::SCORE_SUSPICIOUS_MATCH,
'x\,' => DetectorInterface::SCORE_LOW_PROBABILITY_MATCH,
]
]
];
Expand Down
6 changes: 4 additions & 2 deletions Model/Detector/Xss.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,11 @@ protected function evaluateQuery($value, array &$threats)
'id' => static::RESCODE_SCRIPT_INJECTION,
'reason' => __('JS injection'),
'regex' => [
'location\\.href' => DetectorInterface::SCORE_CRITICAL_MATCH,
'location\\s*\\.\\s*href' => DetectorInterface::SCORE_CRITICAL_MATCH,
'\\.to(\\w{3,5})string\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'(?:this|window|top|parent|frames|self|content)\\.(?:location|document)' => DetectorInterface::SCORE_CRITICAL_MATCH,
'alert\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'(?:this|window|top|parent|frames|self|content)\\s*\\.\\s*(?:location|document)' => DetectorInterface::SCORE_CRITICAL_MATCH,
'document\\s*\\.\\s*\\w+' => DetectorInterface::SCORE_CRITICAL_MATCH,
'getelementby(?:names|id|classname|tag|tagname)\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
'queryselector(?:all)?\\s*\\(' => DetectorInterface::SCORE_CRITICAL_MATCH,
]
Expand Down
41 changes: 24 additions & 17 deletions Model/Ips.php
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

namespace MSP\Shield\Model;

use Magento\Framework\Registry;
Expand All @@ -15,17 +16,14 @@ class Ips implements IpsInterface
* @var DetectorInterface[]
*/
private $detectors;

/**
* @var FilterInterface[]
*/
private $filters;

/**
* @var ProcessorInterface[]
*/
private $processors;

/**
* @var ScanResultInterfaceFactory
*/
Expand All @@ -36,7 +34,8 @@ public function __construct(
$processors = [],
$filters = [],
$detectors = []
) {
)
{
$this->detectors = $detectors;
$this->filters = $filters;
$this->processors = $processors;
Expand All @@ -53,17 +52,30 @@ protected function runProcessors($fieldName, $fieldValue, array &$values = [])
{
if ($fieldValue) {
if (is_string($fieldValue)) {
$preFieldValue = $fieldValue;

foreach ($this->processors as $processor) {
if ($processor->processValue($fieldName, $fieldValue)) {
$this->runProcessors($fieldName, $fieldValue, $values);
$preFieldValue = $fieldValue;
$values[] = $preFieldValue;
$res = $processor->processValue($fieldName, $fieldValue);

// Remove old value, so the new one can replace it
if ($res === ProcessorInterface::RES_REPLACE) {
while (($n = array_search($preFieldValue, $values)) !== false) {
unset($values[$n]);
}
}

if (is_array($fieldValue)) {
break;
}
}

if (!is_array($fieldValue)) {
$values[] = $preFieldValue;
if ($res === ProcessorInterface::RES_SPAWN) {
$values[] = $fieldValue;
}

if ($res !== ProcessorInterface::RES_NO_MATCH) {
$this->runProcessors($fieldName, $fieldValue, $values);
break;
}
}
}

Expand Down Expand Up @@ -102,7 +114,6 @@ protected function runDetectors($fieldName, $fieldValue, &$threats)
];
$scanThreat->setAdditional($additional);
}

$threats = array_merge($threats, $scanThreats);
}
}
Expand All @@ -123,12 +134,10 @@ protected function shouldScan($fieldName, $fieldValue)
if ($res == FilterInterface::MUST_SCAN) {
return true;
}

if ($res == FilterInterface::NO_SCAN) {
return false;
}
}

return true;
}

Expand All @@ -143,7 +152,6 @@ public function scanRequest(array $request)
foreach ($request as $area => $params) {
foreach ($params as $k => $v) {
$fieldKey = $area . '.' . $k;

$possibleValues = [];
$this->runProcessors($fieldKey, $v, $possibleValues);
$possibleValues = array_unique($possibleValues);
Expand All @@ -159,7 +167,6 @@ public function scanRequest(array $request)
$scanResult = $this->scanResultInterfaceFactory->create([
'threats' => $threats,
]);

return $scanResult;
}
}
}
9 changes: 5 additions & 4 deletions Model/Processor/Basic.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,17 @@
class Basic implements ProcessorInterface
{
/**
* Return scanning results
* Dig field and return true if matched
* @param string $fieldName
* @param string &$fieldValue
* @return boolean
* @return string
*/
public function processValue($fieldName, &$fieldValue)
{
$originalValue = $fieldValue;
$fieldValue = preg_replace("/[\r\n\s]+/", ' ', trim($originalValue));
$res = preg_replace("/[\r\n\s]+/", ' ', trim($fieldValue));
$fieldValue = $res;

return ($originalValue !== $fieldValue);
return ($originalValue !== $fieldValue) ? ProcessorInterface::RES_REPLACE : ProcessorInterface::RES_NO_MATCH;
}
}
22 changes: 11 additions & 11 deletions Model/Processor/Charset.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,17 @@ class Charset implements ProcessorInterface
*/
public function processValue($fieldName, &$fieldValue)
{
$utf8 = utf8_decode($fieldValue);
if ($utf8 !== $fieldValue) {
$fieldValue = $utf8;
return true;
}

$utf7 = mb_convert_encoding($fieldValue, 'UTF-8', 'UTF-7');
if ($utf7 !== $fieldValue) {
$fieldValue = $utf7;
return true;
}
// $utf8 = utf8_decode($fieldValue);
// if ($utf8 !== $fieldValue) {
// $fieldValue = $utf8;
// return true;
// }
//
// $utf7 = mb_convert_encoding($fieldValue, 'UTF-8', 'UTF-7');
// if ($utf7 !== $fieldValue) {
// $fieldValue = $utf7;
// return true;
// }

return false;
}
Expand Down
27 changes: 17 additions & 10 deletions Model/Processor/Unpack.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ class Unpack implements ProcessorInterface
* @var DecoderInterface
*/
private $decoder;

/**
* @var array
*/
Expand All @@ -38,7 +37,8 @@ class Unpack implements ProcessorInterface
public function __construct(
DecoderInterface $decoder,
array $skip = []
) {
)
{
$this->decoder = $decoder;
$this->skip = $skip;
}
Expand All @@ -47,12 +47,19 @@ public function __construct(
* Return scanning results
* @param string $fieldName
* @param string &$fieldValue
* @return boolean
* @return string
*/
public function processValue($fieldName, &$fieldValue)
{
if (in_array($fieldName, $this->skip)) {
return false;
return ProcessorInterface::RES_NO_MATCH;
}

// Check if it is an html encoded string
$res = html_entity_decode($fieldValue, ENT_QUOTES | ENT_HTML5, 'UTF-8');
if ($res !== $fieldValue) {
$fieldValue = $res;
return ProcessorInterface::RES_REPLACE;
}

// Check if it is a base64 string
Expand All @@ -61,7 +68,7 @@ public function processValue($fieldName, &$fieldValue)
) {
if ($res = base64_decode($fieldValue)) {
$fieldValue = $res;
return true;
return ProcessorInterface::RES_SPAWN;
}
}

Expand All @@ -74,7 +81,7 @@ public function processValue($fieldName, &$fieldValue)
) {
try {
$fieldValue = $this->decoder->decode($fieldValue);
return true;
return ProcessorInterface::RES_REPLACE;
} catch (\Exception $e) {
}
}
Expand All @@ -83,20 +90,20 @@ public function processValue($fieldName, &$fieldValue)
$urlDecoded = urldecode($fieldValue);
if ($urlDecoded !== $fieldValue) {
$fieldValue = $urlDecoded;
return true;
return ProcessorInterface::RES_SPAWN;
}

// Check PHP serialized variable
if (preg_match('/^\w:\d+:/', $fieldValue)) {
try {
if ($res = unserialize($fieldValue)) {
$fieldValue = $res;
return true;
return ProcessorInterface::RES_REPLACE;
}
} catch (\Exception $e) {
}
}

return false;
return ProcessorInterface::RES_NO_MATCH;
}
}
}
Loading

0 comments on commit 13f7ac7

Please sign in to comment.