From 806ce1f904eba492a654a3132037b3d5676de452 Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Fri, 30 Oct 2015 20:36:55 +1030 Subject: [PATCH 01/11] symlinked modules, generate setup script, ignore route issue Loads correct path if module is symlinked which is the case for modules which are composered in During dry-run generates setup script which can be used instead of directly executing it on production Option to ignore route issue, useful if you just want to generate setup script for allowed variables/blocks --- .gitignore | 1 + fixSUPEE6788.php | 112 +++++++++++++++++++++++++++++++++-------------- 2 files changed, 79 insertions(+), 34 deletions(-) create mode 100644 .gitignore diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..a09c56d --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +/.idea diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index ed11879..2d7dabb 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -88,42 +88,43 @@ public function run() $this->_loadWhitelistsFromFile(); $this->_findModules(); - - static::log('---- Searching config for bad routers -----------------------------'); - - $configAffectedModules = $this->_fixBadAdminhtmlRouter( $dryRun ); - - static::log('---- Moving controllers for bad routers to avoid conflicts --------'); - $this->_moveAdminControllers( $configAffectedModules, $dryRun ); - - static::log('---- Searching files for bad routes -------------------------------'); - $this->_fixBadAdminRoutes( $dryRun ); - + if (!isset($this->_args['ignoreAdminRoutes'])){ + static::log('---- Searching config for bad routers -----------------------------'); + + $configAffectedModules = $this->_fixBadAdminhtmlRouter( $dryRun ); + + static::log('---- Moving controllers for bad routers to avoid conflicts --------'); + $this->_moveAdminControllers( $configAffectedModules, $dryRun ); + + static::log('---- Searching files for bad routes -------------------------------'); + $this->_fixBadAdminRoutes( $dryRun ); + + sort( $this->_modifiedFiles ); + + static::log('---- Summary ------------------------------------------------------'); + static::log( sprintf( "Affected Modules:\n %s", implode( "\n ", $configAffectedModules ) ) ); + static::log( sprintf( "Affected Files:\n %s", implode( "\n ", $this->_modifiedFiles ) ) ); + static::log( sprintf( "Issues:\n %s", implode( "\n ", static::$_errors ) ) ); + static::log('See var/log/fixSUPEE6788.log for a record of all results.'); + + if( isset( $this->_args['recordAffected'] ) ) { + file_put_contents( + Mage::getBaseDir('var') . DS . 'log' . DS . 'fixSUPEE6788-modules.log', + implode( "\n", $configAffectedModules ) + ); + static::log('Wrote affected modules to var/log/fixSUPEE6788-modules.log'); + + file_put_contents( + Mage::getBaseDir('var') . DS . 'log' . DS . 'fixSUPEE6788-files.log', + implode( "\n", $this->_modifiedFiles ) + ); + static::log('Wrote affected files to var/log/fixSUPEE6788-files.log'); + } + } static::log('---- Searching for whitelist problems -----------------------------'); $whitelist = new TemplateVars(); $whitelist->execute( $dryRun ); - - sort( $this->_modifiedFiles ); - - static::log('---- Summary ------------------------------------------------------'); - static::log( sprintf( "Affected Modules:\n %s", implode( "\n ", $configAffectedModules ) ) ); - static::log( sprintf( "Affected Files:\n %s", implode( "\n ", $this->_modifiedFiles ) ) ); - static::log( sprintf( "Issues:\n %s", implode( "\n ", static::$_errors ) ) ); - static::log('See var/log/fixSUPEE6788.log for a record of all results.'); - - if( isset( $this->_args['recordAffected'] ) ) { - file_put_contents( - Mage::getBaseDir('var') . DS . 'log' . DS . 'fixSUPEE6788-modules.log', - implode( "\n", $configAffectedModules ) - ); - static::log('Wrote affected modules to var/log/fixSUPEE6788-modules.log'); - - file_put_contents( - Mage::getBaseDir('var') . DS . 'log' . DS . 'fixSUPEE6788-files.log', - implode( "\n", $this->_modifiedFiles ) - ); - static::log('Wrote affected files to var/log/fixSUPEE6788-files.log'); - } + } elseif( !is_null( $this->_args['fixWhitelists'] ) ) { static::log('-------------------------------------------------------------------'); @@ -154,6 +155,7 @@ public function usageHelp() fixWhitelists Add any missing whitelist entries, without any other changes. SUPEE-6788 must be applied first. recordAffected If given, affected modules/files will be written to var/log/fixSUPEE6788-modules.log and var/log/fixSUPEE6788-files.log for other uses. + ignoreAdminRoutes do not analyze or fix admin route issue APPSEC-1034 For all cases, shell/fixSUPEE6788-whitelist-modules.log and shell/fixSUPEE6788-whitelist-files.log will be loaded and excluded from analysis/changes if they exist. Format same as recordAffected output. @@ -535,6 +537,9 @@ protected function _findModules() // Skip any whitelisted modules. if( !in_array( $name, $this->_moduleWhitelist ) && !in_array( $dir, $this->_moduleWhitelist ) ) { + if ((!is_dir($dir) || is_link($dir)) && is_dir(realpath($dir))){ + $dir = realpath($dir); + } $this->_modules[ $name ] = $dir; } } @@ -693,6 +698,7 @@ public function execute( $dryRun=true ) $this->walkDir($scan, $localeDir, $list); if(count($list['block']) > 0) { + $setupScriptVariables = array(); Mage_Shell_PatchClass::log('Blocks that are not whitelisted:'); $inserts = array(); @@ -704,6 +710,7 @@ public function execute( $dryRun=true ) 'block_name' => $blockName, 'is_allowed' => 1, ); + $setupScriptVariables[] = $blockName; } if( $dryRun === false && !is_null( $this->_blocksTable ) && count( $inserts ) > 0 ) { @@ -711,13 +718,31 @@ public function execute( $dryRun=true ) Mage_Shell_PatchClass::log('Added missing entries to the whitelist'); } + //blocks detected during dry run + elseif ($setupScriptVariables){ + $content = '$blocksToAllow = ' . var_export($setupScriptVariables ,true) . ";\n\n"; + $content.= <<getCollection() + ->addFieldToFilter('block_name',\$blockName) + ->getFirstItem() + ->setBlockName(\$blockName) + ->setIsAllowed(true) + ->save(); +} +scriptContent; + Mage_Shell_PatchClass::log("Add following as setup script \n\n" . $content . "\n"); + + } } if(count($list['variable']) > 0) { Mage_Shell_PatchClass::log('Config variables that are not whitelisted:'); $inserts = array(); - + $setupScriptVariables = array(); foreach ($list['variable'] as $key => $varName) { Mage_Shell_PatchClass::log( sprintf( ' %s in %s', $varName, substr( $key, 0, -1 * strlen($varName) ) ) ); @@ -725,6 +750,7 @@ public function execute( $dryRun=true ) 'variable_name' => $varName, 'is_allowed' => 1, ); + $setupScriptVariables[] = $varName; } if( $dryRun === false && !is_null( $this->_varsTable ) && count( $inserts ) > 0 ) { @@ -732,6 +758,24 @@ public function execute( $dryRun=true ) Mage_Shell_PatchClass::log('Added missing entries to the whitelist'); } + elseif ($setupScriptVariables){ + $content = '$variablesToAllow = ' . var_export($setupScriptVariables ,true) . ";\n\n"; + Mage::getModel('admin/variable')->load(); + $content.= <<getCollection() + ->addFieldToFilter('variable_name',\$variableName) + ->getFirstItem() + ->setBlockName(\$variableName) + ->setIsAllowed(true) + ->save(); +} +scriptContent; + Mage_Shell_PatchClass::log("Add following as setup script \n\n" . $content . "\n"); + + } } } From 6ac4b34a2bf524f4dfbaca9f5f98b781fbd13998 Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Mon, 2 Nov 2015 17:00:21 +1030 Subject: [PATCH 02/11] separated the logic of fixAdminRoutes from addFieldToUrl issue --- fixSUPEE6788.php | 218 +++++++++++++++++++++++++++++------------------ 1 file changed, 136 insertions(+), 82 deletions(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index 2d7dabb..6466f34 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -88,7 +88,10 @@ public function run() $this->_loadWhitelistsFromFile(); $this->_findModules(); - if (!isset($this->_args['ignoreAdminRoutes'])){ + if (isset($this->_args['ignoreAdminRoutes'])) { + $this->_showUnescapedFields($dryRun); + } + else{ static::log('---- Searching config for bad routers -----------------------------'); $configAffectedModules = $this->_fixBadAdminhtmlRouter( $dryRun ); @@ -405,20 +408,14 @@ protected function _moveAdminControllers( $modulePaths, $dryRun=true ) return $this; } - - /** - * Attempt to find and fix any admin URLs (routes) affected by the router change. - * - * @param boolean $dryRun If true, find affected only; do not apply changes. - * @return $this - */ - protected function _fixBadAdminRoutes( $dryRun=true ) + + public function loopLines($dryRun, $lineCallBack) { $scanPaths = array( Mage::getBaseDir('code'), Mage::getBaseDir('design') . DS . 'adminhtml', ); - + /** * Trudge through the filesystem. */ @@ -432,95 +429,149 @@ protected function _fixBadAdminRoutes( $dryRun=true ) if( strrpos( $file, '.php' ) === false && strrpos( $file, '.xml' ) === false && strrpos( $file, '.phtml' ) === false ) { continue; } - + // Skip any files inside .svn directories if( strrpos( $file, '.svn') !== false) { continue; } - + // Skip any whitelisted files. if( in_array( $file, $this->_fileWhitelist ) ) { continue; } - + $fileContents = file_get_contents( $file ); $lines = explode( "\n", $fileContents ); - $changes = false; - - /** - * Scan the file line-by-line for each pattern. - */ - $oldUrlPath = ''; - $newUrlPath = ''; - $checkLine = -1; - foreach( $lines as $key => $line ) { - foreach( $this->_fileReplacePatterns as $pattern => $replacement ) { - if( strpos( $line, $pattern ) !== false ) { - $lines[ $key ] = str_replace( $pattern, $replacement, $line ); - } else if ( $checkLine !== $key && strpos( $pattern, 'getUrl' ) !== false && strpos( $line, 'getUrl' ) !== false ) { - // Handle multi-line getUrl syntax. cf. https://github.com/rhoerr/supee-6788-toolbox/pull/1 - $oldUrlPath = substr( $pattern, strcspn($pattern, '"\'') + 1 ); - $newUrlPath = substr( $replacement, strcspn($replacement, '"\'') + 1 ); - $checkLine = ( strlen($oldUrlPath) > 0 && strlen($newUrlPath ) > 0 ) ? $key + 1 : -1; - } - - if ( $key == $checkLine ) { - if( strpos( $line, $oldUrlPath ) !== false ) { - $lines[ $key ] = str_replace( $oldUrlPath, $newUrlPath, $line ); - } - - $oldUrlPath = ''; - $newUrlPath = ''; - $checkLine = -1; - } - } - - /** - * Check for APPSEC-1063 - Thanks @timvroom - */ - if( preg_match( '/addFieldToFilter\(\s*[\'"]?[\`\(]/i', $line ) ) { - static::log( sprintf( 'POSSIBLE SQL VULNERABILITY: %s:%s', $file, $key ), true ); - static::log( sprintf( ' CODE:%s', $line ) ); - } - - /** - * If this line has any changes, record it. - */ - if( $line != $lines[ $key ] ) { - if( $changes === false ) { - static::log( $file ); - $changes = true; - } - - static::log( sprintf( ' WAS:%s', $line ) ); - static::log( sprintf( ' NOW:%s', $lines[ $key ] ) ); - } + if (!is_array($lineCallBack) || is_callable($lineCallBack)) { + $lineCallBack = array($lineCallBack); } - - /** - * If the file has been modified, record it and save. - */ - if( $changes === true ) { - if( $dryRun === false ) { - $fileContents = implode( "\n", $lines ); - - if( file_put_contents( $file, $fileContents ) !== false ) { - $this->_modifiedFiles[] = $file; - // Silence! - } - else { - static::log( sprintf( 'Unable to write changes to %s', $file ), true ); - } + foreach ($lineCallBack as $callBack) { + if (is_callable($callBack)) { + $args = array($dryRun, $lines, $file); + call_user_func_array($callBack, $args); } - else { - $this->_modifiedFiles[] = $file; + } + } + } + } + + /** + * call back to display already escaped calls to addFieldToFilter + * @param $dryRun + * @param $lines + * @param $file + */ + public function displayEscapedFields($dryRun, $lines, $file) + { + foreach ($lines as $key => $line) { + /** + * Check for APPSEC-1063 - Thanks @timvroom + */ + if( preg_match( '/addFieldToFilter\(\s*[\'"]?[\`\(]/i', $line ) ) { + static::log( sprintf( 'POSSIBLE SQL VULNERABILITY: %s:%s', $file, $key ), true ); + static::log( sprintf( ' CODE:%s', $line ) ); + } + } + } + + /** + * Call back to fix Admin Routes + * @param $dryRun + * @param $lines + * @param $file + */ + public function fixAdminRouteCallBack($dryRun, $lines, $file) + { + $changes = false; + + /** + * Scan the file line-by-line for each pattern. + */ + $oldUrlPath = ''; + $newUrlPath = ''; + $checkLine = -1; + foreach ($lines as $key => $line) { + foreach ($this->_fileReplacePatterns as $pattern => $replacement) { + if (strpos($line, $pattern) !== false) { + $lines[$key] = str_replace($pattern, $replacement, $line); + } + else if ($checkLine !== $key && strpos($pattern, 'getUrl') !== false && strpos($line, 'getUrl') !== false) { + // Handle multi-line getUrl syntax. cf. https://github.com/rhoerr/supee-6788-toolbox/pull/1 + $oldUrlPath = substr($pattern, strcspn($pattern, '"\'') + 1); + $newUrlPath = substr($replacement, strcspn($replacement, '"\'') + 1); + $checkLine = (strlen($oldUrlPath) > 0 && strlen($newUrlPath) > 0) ? $key + 1 : -1; + } + + if ($key == $checkLine) { + if (strpos($line, $oldUrlPath) !== false) { + $lines[$key] = str_replace($oldUrlPath, $newUrlPath, $line); } + + $oldUrlPath = ''; + $newUrlPath = ''; + $checkLine = -1; + } + } + + /** + * If this line has any changes, record it. + */ + if ($line != $lines[$key]) { + if ($changes === false) { + static::log($file); + $changes = true; } + + static::log(sprintf(' WAS:%s', $line)); + static::log(sprintf(' NOW:%s', $lines[$key])); } } - + + /** + * If the file has been modified, record it and save. + */ + if ($changes === true) { + if ($dryRun === false) { + $fileContents = implode("\n", $lines); + + if (file_put_contents($file, $fileContents) !== false) { + $this->_modifiedFiles[] = $file; + // Silence! + } + else { + static::log(sprintf('Unable to write changes to %s', $file), true); + } + } + else { + $this->_modifiedFiles[] = $file; + } + } + } + /** + * Attempt to find and fix any admin URLs (routes) affected by the router change. + * + * @param boolean $dryRun If true, find affected only; do not apply changes. + * @return $this + */ + protected function _fixBadAdminRoutes( $dryRun=true ) + { + $adminRouteCallBack = (array($this,'fixAdminRouteCallBack')); + $escapedFieldsCallBack = (array($this,'displayEscapedFields')); + + $callBack = array($adminRouteCallBack, $escapedFieldsCallBack); + $this->loopLines($dryRun,$callBack); return $this; } + + /** + * Display already escaped calls to addFieldToFilter + * @param $dryRun + */ + protected function _showUnescapedFields($dryRun) + { + $escapedFieldsCallBack = (array($this,'displayEscapedFields')); + $this->loopLines($dryRun,$escapedFieldsCallBack); + } /** * Locate all modules in the system. @@ -537,7 +588,10 @@ protected function _findModules() // Skip any whitelisted modules. if( !in_array( $name, $this->_moduleWhitelist ) && !in_array( $dir, $this->_moduleWhitelist ) ) { - if ((!is_dir($dir) || is_link($dir)) && is_dir(realpath($dir))){ + if (isset($this->_modules[ $name ])){ + continue; + } + if (is_dir(realpath($dir)) && ((is_link($dir)) || ($dir != realpath($dir))) ){ $dir = realpath($dir); } $this->_modules[ $name ] = $dir; From d43d510ca16408c37948f8ac0e1413f318dc2cd6 Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Mon, 2 Nov 2015 18:13:29 +1030 Subject: [PATCH 03/11] added check for form keys and password token --- fixSUPEE6788.php | 68 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index 6466f34..ea553be 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -127,6 +127,7 @@ public function run() static::log('---- Searching for whitelist problems -----------------------------'); $whitelist = new TemplateVars(); $whitelist->execute( $dryRun ); + $this->_fixTemplateIssues(); } elseif( !is_null( $this->_args['fixWhitelists'] ) ) { @@ -409,12 +410,15 @@ protected function _moveAdminControllers( $modulePaths, $dryRun=true ) return $this; } - public function loopLines($dryRun, $lineCallBack) + public function loopLines($dryRun, $lineCallBack, $scanPaths = array()) { - $scanPaths = array( - Mage::getBaseDir('code'), - Mage::getBaseDir('design') . DS . 'adminhtml', - ); + if (!$scanPaths){ + $scanPaths = array( + Mage::getBaseDir('code'), + Mage::getBaseDir('design') . DS . 'adminhtml', + ); + } + /** * Trudge through the filesystem. @@ -447,7 +451,7 @@ public function loopLines($dryRun, $lineCallBack) } foreach ($lineCallBack as $callBack) { if (is_callable($callBack)) { - $args = array($dryRun, $lines, $file); + $args = array($dryRun, $lines, $fileContents, $file); call_user_func_array($callBack, $args); } } @@ -461,7 +465,7 @@ public function loopLines($dryRun, $lineCallBack) * @param $lines * @param $file */ - public function displayEscapedFields($dryRun, $lines, $file) + public function displayEscapedFieldsCallBack($dryRun, $lines, $fileContents, $file) { foreach ($lines as $key => $line) { /** @@ -474,13 +478,57 @@ public function displayEscapedFields($dryRun, $lines, $file) } } + /** + * display code where form keys are missing + * + * @param $dryRun + * @param $lines + * @param $file + */ + public function formKeysCallBack($dryRun, $lines, $fileContents, $file) + { + if (!strpos($file,'template/persistent/customer/form/register.phtml')){ + return ; + } + if (strpos($fileContents,'getSuccessUrl') && !(strpos($fileContents,'getFormKey'))){ + static::log( sprintf( 'POSSIBLE MISSING FORMKEYS: %s', $file ), true ); + } + } + + /** + * display code which is still using setResetPasswordLinkToken or getResetPasswordLinkToken + * + * @param $dryRun + * @param $lines + * @param $file + */ + public function passwordTokenCallBack($dryRun, $lines, $fileContents, $file) + { + if (strpos($fileContents,'setResetPasswordLinkToken') || (strpos($fileContents,'getResetPasswordLinkToken'))){ + static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken : %s', $file ), true ); + } + } + + protected function _fixTemplateIssues( $dryRun=true ) + { + static::log('---- Searching for template issues -----------------------------'); + $formsKeysCallBack = (array($this, 'formKeysCallBack')); + $passwordTokenCallBack = (array($this, 'passwordTokenCallBack')); + $callBack = array($formsKeysCallBack, $passwordTokenCallBack); + + $scanPaths = array(Mage::getBaseDir('design'), Mage::getBaseDir('code')); + $this->loopLines($dryRun, $callBack, $scanPaths); + + return $this; + } + /** * Call back to fix Admin Routes * @param $dryRun * @param $lines * @param $file */ - public function fixAdminRouteCallBack($dryRun, $lines, $file) + public function fixAdminRouteCallBack($dryRun, $lines, $fileContents, $file) { $changes = false; @@ -556,7 +604,7 @@ public function fixAdminRouteCallBack($dryRun, $lines, $file) protected function _fixBadAdminRoutes( $dryRun=true ) { $adminRouteCallBack = (array($this,'fixAdminRouteCallBack')); - $escapedFieldsCallBack = (array($this,'displayEscapedFields')); + $escapedFieldsCallBack = (array($this,'displayEscapedFieldsCallBack')); $callBack = array($adminRouteCallBack, $escapedFieldsCallBack); $this->loopLines($dryRun,$callBack); @@ -569,7 +617,7 @@ protected function _fixBadAdminRoutes( $dryRun=true ) */ protected function _showUnescapedFields($dryRun) { - $escapedFieldsCallBack = (array($this,'displayEscapedFields')); + $escapedFieldsCallBack = (array($this,'displayEscapedFieldsCallBack')); $this->loopLines($dryRun,$escapedFieldsCallBack); } From a08bbef74ee266eb2ead75c80b1aba7042aa0c8f Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Mon, 2 Nov 2015 20:01:21 +1030 Subject: [PATCH 04/11] Do not duplicate blocks/variables in setup script --- fixSUPEE6788.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index ea553be..a170c69 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -812,7 +812,7 @@ public function execute( $dryRun=true ) 'block_name' => $blockName, 'is_allowed' => 1, ); - $setupScriptVariables[] = $blockName; + $setupScriptVariables[$blockName] = $blockName; } if( $dryRun === false && !is_null( $this->_blocksTable ) && count( $inserts ) > 0 ) { @@ -822,7 +822,10 @@ public function execute( $dryRun=true ) } //blocks detected during dry run elseif ($setupScriptVariables){ - $content = '$blocksToAllow = ' . var_export($setupScriptVariables ,true) . ";\n\n"; + $variableString = var_export(array_values($setupScriptVariables) ,true); + //strip numeric keys 1=> 'value + $variableString = preg_replace("/[0-9]+ \=\>/i", '', $variableString); + $content = '$blocksToAllow = ' . $variableString . ";\n\n"; $content.= << $varName, 'is_allowed' => 1, ); - $setupScriptVariables[] = $varName; + $setupScriptVariables[$varName] = $varName; } if( $dryRun === false && !is_null( $this->_varsTable ) && count( $inserts ) > 0 ) { @@ -861,7 +864,10 @@ public function execute( $dryRun=true ) Mage_Shell_PatchClass::log('Added missing entries to the whitelist'); } elseif ($setupScriptVariables){ - $content = '$variablesToAllow = ' . var_export($setupScriptVariables ,true) . ";\n\n"; + $variableString = var_export(array_values($setupScriptVariables) ,true); + //strip numeric keys 1=> 'value + $variableString = preg_replace("/[0-9]+ \=\>/i", '', $variableString); + $content = '$variablesToAllow = ' . $variableString . ";\n\n"; Mage::getModel('admin/variable')->load(); $content.= << Date: Tue, 3 Nov 2015 11:43:20 +1030 Subject: [PATCH 05/11] show line when issue with reset password is detected --- fixSUPEE6788.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index a170c69..8422402 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -505,7 +505,12 @@ public function formKeysCallBack($dryRun, $lines, $fileContents, $file) public function passwordTokenCallBack($dryRun, $lines, $fileContents, $file) { if (strpos($fileContents,'setResetPasswordLinkToken') || (strpos($fileContents,'getResetPasswordLinkToken'))){ - static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken : %s', $file ), true ); + foreach ($lines as $key => $line) { + if (strpos($line,'setResetPasswordLinkToken') || (strpos($line,'getResetPasswordLinkToken'))){ + static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken: %s:%s', $file, $key ), true ); + static::log( sprintf( ' CODE:%s', $line ) ); + } + } } } From 4372b6a1f25850edb98c5bdb0307aeb074e89395 Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Wed, 4 Nov 2015 10:53:54 +1030 Subject: [PATCH 06/11] correct line number and expanded register.phtml search --- fixSUPEE6788.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index 8422402..e0c0a37 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -472,7 +472,7 @@ public function displayEscapedFieldsCallBack($dryRun, $lines, $fileContents, $fi * Check for APPSEC-1063 - Thanks @timvroom */ if( preg_match( '/addFieldToFilter\(\s*[\'"]?[\`\(]/i', $line ) ) { - static::log( sprintf( 'POSSIBLE SQL VULNERABILITY: %s:%s', $file, $key ), true ); + static::log( sprintf( 'POSSIBLE SQL VULNERABILITY: %s:%s', $file, $key+1 ), true ); static::log( sprintf( ' CODE:%s', $line ) ); } } @@ -487,7 +487,7 @@ public function displayEscapedFieldsCallBack($dryRun, $lines, $fileContents, $fi */ public function formKeysCallBack($dryRun, $lines, $fileContents, $file) { - if (!strpos($file,'template/persistent/customer/form/register.phtml')){ + if (!strpos($file, 'register.phtml')) { return ; } if (strpos($fileContents,'getSuccessUrl') && !(strpos($fileContents,'getFormKey'))){ @@ -507,7 +507,7 @@ public function passwordTokenCallBack($dryRun, $lines, $fileContents, $file) if (strpos($fileContents,'setResetPasswordLinkToken') || (strpos($fileContents,'getResetPasswordLinkToken'))){ foreach ($lines as $key => $line) { if (strpos($line,'setResetPasswordLinkToken') || (strpos($line,'getResetPasswordLinkToken'))){ - static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken: %s:%s', $file, $key ), true ); + static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken: %s:%s', $file, $key+1 ), true ); static::log( sprintf( ' CODE:%s', $line ) ); } } From 9a967cdf891005d94d136df1158af112fcf9362d Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Wed, 4 Nov 2015 13:28:55 +1030 Subject: [PATCH 07/11] remove base dir and suggests form key solution --- fixSUPEE6788.php | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index e0c0a37..df4aa04 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -46,6 +46,9 @@ class Mage_Shell_PatchClass extends Mage_Shell_Abstract ); protected $_fileWhitelist = array(); + + protected $_formKeysIssue = false; + protected $_resetPasswordTokenIssue = false; /** * Apply PHP settings to shell script @@ -472,7 +475,7 @@ public function displayEscapedFieldsCallBack($dryRun, $lines, $fileContents, $fi * Check for APPSEC-1063 - Thanks @timvroom */ if( preg_match( '/addFieldToFilter\(\s*[\'"]?[\`\(]/i', $line ) ) { - static::log( sprintf( 'POSSIBLE SQL VULNERABILITY: %s:%s', $file, $key+1 ), true ); + static::log( sprintf( 'POSSIBLE SQL VULNERABILITY: %s:%s', $this->removeBaseDir($file), $key+1 ), true ); static::log( sprintf( ' CODE:%s', $line ) ); } } @@ -491,7 +494,8 @@ public function formKeysCallBack($dryRun, $lines, $fileContents, $file) return ; } if (strpos($fileContents,'getSuccessUrl') && !(strpos($fileContents,'getFormKey'))){ - static::log( sprintf( 'POSSIBLE MISSING FORMKEYS: %s', $file ), true ); + static::log( sprintf( 'POSSIBLE MISSING FORMKEYS: %s', $this->removeBaseDir($file)), true ); + $this->_formKeysIssue = true; } } @@ -507,13 +511,19 @@ public function passwordTokenCallBack($dryRun, $lines, $fileContents, $file) if (strpos($fileContents,'setResetPasswordLinkToken') || (strpos($fileContents,'getResetPasswordLinkToken'))){ foreach ($lines as $key => $line) { if (strpos($line,'setResetPasswordLinkToken') || (strpos($line,'getResetPasswordLinkToken'))){ - static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken: %s:%s', $file, $key+1 ), true ); + static::log( sprintf( 'POSSIBLE use of resetPasswordLinkToken: %s:%s', $this->removeBaseDir($file), $key+1 ), true ); static::log( sprintf( ' CODE:%s', $line ) ); } } + $this->_resetPasswordTokenIssue = true; } } + public function removeBaseDir($file) + { + return str_replace(Mage::getBaseDir('base') . '/','',$file); + } + protected function _fixTemplateIssues( $dryRun=true ) { static::log('---- Searching for template issues -----------------------------'); @@ -523,7 +533,16 @@ protected function _fixTemplateIssues( $dryRun=true ) $scanPaths = array(Mage::getBaseDir('design'), Mage::getBaseDir('code')); $this->loopLines($dryRun, $callBack, $scanPaths); - + if ($this->_formKeysIssue){ + $message = "Fix Form Keys issue by adding \n" + . ' getFormKey() ?>" />'; + static::log($message); + } + if ($this->_resetPasswordTokenIssue){ + $message = "fix Reset password token issue by removing \n" + . " , array('_query' => array('id' => \$this->getCustomerId(), 'token' => \$this->getResetPasswordLinkToken()))"; + static::log($message); + } return $this; } From 05e6718fa061da0d7f51da5c2a5e99d217c0abba Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Mon, 9 Nov 2015 11:39:32 +1030 Subject: [PATCH 08/11] scan in vendor folder too when present --- fixSUPEE6788.php | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index df4aa04..f738460 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -532,6 +532,7 @@ protected function _fixTemplateIssues( $dryRun=true ) $callBack = array($formsKeysCallBack, $passwordTokenCallBack); $scanPaths = array(Mage::getBaseDir('design'), Mage::getBaseDir('code')); + $scanPaths = $this->addVendorFolderToArray($scanPaths); $this->loopLines($dryRun, $callBack, $scanPaths); if ($this->_formKeysIssue){ $message = "Fix Form Keys issue by adding \n" @@ -642,7 +643,25 @@ protected function _fixBadAdminRoutes( $dryRun=true ) protected function _showUnescapedFields($dryRun) { $escapedFieldsCallBack = (array($this,'displayEscapedFieldsCallBack')); - $this->loopLines($dryRun,$escapedFieldsCallBack); + $scanPaths = array(Mage::getBaseDir('design'), Mage::getBaseDir('code'),); + $scanPaths = $this->addVendorFolderToArray($scanPaths); + $this->loopLines($dryRun,$escapedFieldsCallBack,$scanPaths); + } + + protected function getVendorFolder() + { + $vendorFolder = dirname(dirname(Mage::getBaseDir('code'))) . '/vendor'; + $vendorFolder = is_dir($vendorFolder) ? $vendorFolder : false; + return $vendorFolder; + } + + protected function addVendorFolderToArray($arrayGiven) + { + $vendorFolder = $this->getVendorFolder(); + if ($vendorFolder) { + $arrayGiven[] = $vendorFolder; + } + return $arrayGiven; } /** From 4f9a5f0e6228f7666fd49130cd168d0ff718145d Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Mon, 9 Nov 2015 12:20:59 +1030 Subject: [PATCH 09/11] Look into .modman folder as well for incompatibilities --- fixSUPEE6788.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index f738460..3b27f6b 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -648,16 +648,20 @@ protected function _showUnescapedFields($dryRun) $this->loopLines($dryRun,$escapedFieldsCallBack,$scanPaths); } - protected function getVendorFolder() + protected function getVendorFolder($vendorFolder) { - $vendorFolder = dirname(dirname(Mage::getBaseDir('code'))) . '/vendor'; + $vendorFolder = dirname(dirname(Mage::getBaseDir('code'))) . '/' . $vendorFolder; $vendorFolder = is_dir($vendorFolder) ? $vendorFolder : false; return $vendorFolder; } protected function addVendorFolderToArray($arrayGiven) { - $vendorFolder = $this->getVendorFolder(); + $vendorFolder = $this->getVendorFolder('vendor'); + if ($vendorFolder) { + $arrayGiven[] = $vendorFolder; + } + $vendorFolder = $this->getVendorFolder('.modman'); if ($vendorFolder) { $arrayGiven[] = $vendorFolder; } From ed3947258825456a51bb631516a1e78f44ed848f Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Tue, 17 Nov 2015 17:36:49 +1030 Subject: [PATCH 10/11] documented ignoreAdminRoute and added manual layout xml check --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 92f77df..a912652 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ If you need help, let us know. Contact details at the bottom. * Backup your website. * Upload fixSUPEE6788.php to {magento}/shell/fixSUPEE6788.php * **To analyze:** Run from SSH: `php -f fixSUPEE6788.php -- analyze` +* **To analyze ignoring admin route:** Run from SSH: `php -f fixSUPEE6788.php -- analyze --ignoreAdminRoute` * **To apply changes:** Run from SSH: `php -f fixSUPEE6788.php -- fix` * **To fix missing whitelist entries only:** Run from SSH: `php -f fixSUPEE6788.php -- fixWhitelists` * Additional option: `recordAffected` - If given, two files will be written after running: `var/log/fixSUPEE6788-modules.log` containing all modules affected by the patch, and `var/log/fixSUPEE6788-files.log` containing all files the script would/did modify. Use this to grab an archive of modified files (`tar czf modified.tar.gz -T var/log/fixSUPEE6788-files.log`), or weed out any files/modules for the fix whitelist. @@ -45,6 +46,9 @@ There are four points of interest outlined. * Script will not handle multiple admin routes in a single module. * The script may not catch all possible route formats. The automated changes may result in broken admin pages that must be corrected manually. +## Manual Checks +* If layout file customer.xml is overwritten make sure overwrite has modified handle `` + ## Who we are This script is provided as a courtesy from ParadoxLabs. We created it to help with applying our own patches, and we're sharing it so you can benefit too. We are a Magento Silver Solution Partner, based out of Lancaster, Pennsylvania USA. From a519f13fce17a2571c9a73793cd7b767a0e607cc Mon Sep 17 00:00:00 2001 From: Zain ul abidin Date: Fri, 4 Dec 2015 17:13:02 +1030 Subject: [PATCH 11/11] correct path when its symlinked --- fixSUPEE6788.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixSUPEE6788.php b/fixSUPEE6788.php index 3b27f6b..c26582c 100644 --- a/fixSUPEE6788.php +++ b/fixSUPEE6788.php @@ -26,7 +26,7 @@ * Support: http://support.paradoxlabs.com */ -require_once 'abstract.php'; +require_once dirname($_SERVER['SCRIPT_NAME']) . DIRECTORY_SEPARATOR . 'abstract.php'; class Mage_Shell_PatchClass extends Mage_Shell_Abstract {