From 11bd685bdff20d4346d2d92cbcecf8d53fdd51f9 Mon Sep 17 00:00:00 2001 From: bermiferrer Date: Mon, 18 Feb 2008 13:24:12 +0000 Subject: [PATCH] Adding tests for securing private variable inclussion on templates. Fixed #112 by extending from AkObject wich now has a default __toString() method. Added support for nested members on Savants PhpCodeAnalizer. (repoted to the maintainer) git-svn-id: http://svn.akelos.org/trunk@480 a2fa5c27-f921-0410-a72c-bf682d381be0 --- lib/AkActionController.php | 3 +- lib/AkActionView.php | 2 +- lib/AkActionView/AkActionViewHelper.php | 2 +- lib/AkActionView/AkPhpCodeSanitizer.php | 61 +++++++++++++++++++---- lib/AkActionView/helpers/capture_helper.php | 2 +- lib/AkActionView/helpers/number_helper.php | 2 +- lib/AkActionView/helpers/tag_helper.php | 2 +- lib/AkActionView/helpers/text_helper.php | 2 +- lib/AkActionView/helpers/url_helper.php | 2 +- lib/AkObject.php | 5 ++ lib/AkRequest.php | 2 +- test/unit/lib/AkActionView/AkPhpCodeSanitizer.php | 51 +++++++++++++++++++ vendor/PHPCodeAnalyzer/PHPCodeAnalyzer.php | 17 ++++--- 13 files changed, 127 insertions(+), 26 deletions(-) create mode 100644 test/unit/lib/AkActionView/AkPhpCodeSanitizer.php diff --git a/lib/AkActionController.php b/lib/AkActionController.php index c1b9b2a..07f7e6b 100644 --- a/lib/AkActionController.php +++ b/lib/AkActionController.php @@ -1083,7 +1083,8 @@ class AkActionController extends AkObject function toString() { return $this->Request->getProtocol().$this->Request->getHostWithPort(). - $this->Request->getPath().$this->parameters['controller'].$this->parameters['action'].$this->parameters['inspect']; + $this->Request->getPath().@$this->parameters['controller']. + @$this->parameters['action'].@$this->parameters['inspect']; } /** diff --git a/lib/AkActionView.php b/lib/AkActionView.php index f3f2e7d..2a7c5af 100644 --- a/lib/AkActionView.php +++ b/lib/AkActionView.php @@ -89,7 +89,7 @@ * * See the AkActionView/Helpers/PrototypeHelper/JavaScriptGenerator documentation for more details. */ -class AkActionView +class AkActionView extends AkObject { var $first_render, $base_path, $assigns, $template_extension, $controller, $logger, $params, $request, $response, $session, $headers, $flash; diff --git a/lib/AkActionView/AkActionViewHelper.php b/lib/AkActionView/AkActionViewHelper.php index 1d43f51..9071920 100644 --- a/lib/AkActionView/AkActionViewHelper.php +++ b/lib/AkActionView/AkActionViewHelper.php @@ -16,7 +16,7 @@ * @license GNU Lesser General Public License */ -class AkActionViewHelper +class AkActionViewHelper extends AkObject { var $locales_namespace = 'helpers'; diff --git a/lib/AkActionView/AkPhpCodeSanitizer.php b/lib/AkActionView/AkPhpCodeSanitizer.php index 33bbe97..702da14 100644 --- a/lib/AkActionView/AkPhpCodeSanitizer.php +++ b/lib/AkActionView/AkPhpCodeSanitizer.php @@ -29,7 +29,7 @@ class AkPhpCodeSanitizer var $secure_active_record_method_calls = false; var $_errors = array(); var $_options = array(); - var $_protedted_types = array('constructs','variables','functions','classes','methods'); + var $_protedted_types = array('constructs','variables','member variables','functions','classes','methods'); function clearErrors() { @@ -49,7 +49,6 @@ class AkPhpCodeSanitizer $code =& $this->_options['code']; } $this->AnalyzeCode($code); - $this->secureVariables($code); $this->secureFunctions($code); $this->secureConstructs($code); @@ -71,14 +70,19 @@ class AkPhpCodeSanitizer { $code = empty($code) ? @$this->_options['code'] : $code; if(AK_DEBUG){ - echo - '

'.Ak::t('Template %template_file security error', array('%template_file'=>@$this->_options['file_path'])).':

'. - "
\n". - '

'.Ak::t('Showing template source from %file:',array('%file'=>$this->_options['file_path'])).'

'. - (isset($this->_options['file_path']) ? '
'.htmlentities(Ak::file_get_contents($this->_options['file_path'])).'
':''). - '

'.Ak::t('Showing compiled template source:').'

'.highlight_string($code,true); - - die(); + // We can't halt execution while testing and the error message is too large for trigger_error + if(AK_ENVIRONMENT == 'testing'){ + trigger_error(join("\n", $this->getErrors()), E_USER_WARNING); + }else{ + echo + '

'.Ak::t('Template %template_file security error', array('%template_file'=>@$this->_options['file_path'])).':

'. + "
\n". + '

'.Ak::t('Showing template source from %file:',array('%file'=>$this->_options['file_path'])).'

'. + (isset($this->_options['file_path']) ? '
'.htmlentities(Ak::file_get_contents($this->_options['file_path'])).'
':''). + '

'.Ak::t('Showing compiled template source:').'

'.highlight_string($code,true); + + die(); + } }else{ trigger_error(Ak::t('Template compilation error'),E_USER_ERROR); } @@ -105,6 +109,9 @@ class AkPhpCodeSanitizer array_map(array(&$this,'_addDollarSymbol_'), $_forbidden['variables']); $_used_vars = array_keys((array)$this->Analyzer->usedVariables); + + $this->lookForPrivateMemberVariables($this->Analyzer->usedMemberVariables); + $this->_invalid['variables'] = array_diff($_used_vars, array_diff($_used_vars,array_merge($_forbidden['variables'], array_filter($_used_vars, array(&$this, 'isPrivateVar'))))); } @@ -171,7 +178,39 @@ class AkPhpCodeSanitizer function isPrivateVar($var) { - return $var[1]==="_"; + return preg_match('/^["\'${\.]*_/', $var); + } + + function isPrivateDynamicVar($var) + { + if(preg_match('/^["\'{\.]*\$/', $var)){ + $var_name = trim($var, '{"\'.$'); + if(isset($GLOBALS[$var_name])){ + return $this->isPrivateVar($GLOBALS[$var_name]); + } + return true; + } + return false; + } + + function lookForPrivateMemberVariables($var, $nested = false) + { + if(is_string($var) && $this->isPrivateVar($var)){ + $this->_invalid['member variables'][$var] = $var; + return true; + }elseif (is_array($var)){ + foreach (array_keys($var) as $k){ + if($this->isPrivateVar($k) || ($nested && $this->isPrivateDynamicVar($k))){ + $this->_invalid['member variables'][$k] = $k; + return true; + }elseif($this->lookForPrivateMemberVariables($var[$k], true)){ + return true; + } + } + }elseif (is_object($var)){ + return $this->lookForPrivateMemberVariables((array)$var, true); + } + return false; } function &getCodeAnalyzerInstance() diff --git a/lib/AkActionView/helpers/capture_helper.php b/lib/AkActionView/helpers/capture_helper.php index 96fd6c1..bd872c1 100644 --- a/lib/AkActionView/helpers/capture_helper.php +++ b/lib/AkActionView/helpers/capture_helper.php @@ -57,7 +57,7 @@ * * Normal view text */ -class CaptureHelper +class CaptureHelper extends AkObject { var $_stack = array(); /** diff --git a/lib/AkActionView/helpers/number_helper.php b/lib/AkActionView/helpers/number_helper.php index 253f834..a267d4e 100644 --- a/lib/AkActionView/helpers/number_helper.php +++ b/lib/AkActionView/helpers/number_helper.php @@ -21,7 +21,7 @@ * Provides methods for converting a number into a formatted string that currently represents * one of the following forms: phone number, percentage, money, or precision level. */ -class NumberHelper +class NumberHelper extends AkObject { /** * Formats a +number+ into a US phone number string. The +options+ can be a array used to customize the diff --git a/lib/AkActionView/helpers/tag_helper.php b/lib/AkActionView/helpers/tag_helper.php index d3b1260..74e7738 100644 --- a/lib/AkActionView/helpers/tag_helper.php +++ b/lib/AkActionView/helpers/tag_helper.php @@ -21,7 +21,7 @@ * Use these methods to generate HTML tags programmatically when you can't use a Builder. * By default, they output XHTML compliant tags. */ -class TagHelper +class TagHelper extends AkObject { /** * Returns an empty HTML tag of type *name* which by default is XHTML diff --git a/lib/AkActionView/helpers/text_helper.php b/lib/AkActionView/helpers/text_helper.php index ce98a0c..8288178 100644 --- a/lib/AkActionView/helpers/text_helper.php +++ b/lib/AkActionView/helpers/text_helper.php @@ -50,7 +50,7 @@ define('AK_AUTO_LINK_REGEX','/ * Title: truncate($post->title, 20) ?> * {end} */ -class TextHelper +class TextHelper extends AkObject { function setController(&$controller) diff --git a/lib/AkActionView/helpers/url_helper.php b/lib/AkActionView/helpers/url_helper.php index 62c48c3..2ec583f 100644 --- a/lib/AkActionView/helpers/url_helper.php +++ b/lib/AkActionView/helpers/url_helper.php @@ -18,7 +18,7 @@ require_once(AK_LIB_DIR.DS.'AkActionView'.DS.'helpers'.DS.'javascript_helper.php'); -class UrlHelper +class UrlHelper extends AkObject { function setController(&$controller) { diff --git a/lib/AkObject.php b/lib/AkObject.php index eff0f99..c742b82 100644 --- a/lib/AkObject.php +++ b/lib/AkObject.php @@ -87,6 +87,11 @@ class AkObject { return get_class($this); } + + function __toString() + { + return $this->toString(); + } // }}} diff --git a/lib/AkRequest.php b/lib/AkRequest.php index 971448b..b57a96c 100644 --- a/lib/AkRequest.php +++ b/lib/AkRequest.php @@ -751,7 +751,7 @@ class AkRequest extends AkObject include_once($module_shared_model); } - if(!include_once($controller_path)){ + if(!is_file($controller_path) || !include_once($controller_path)){ if(AK_ENVIRONMENT == 'development'){ trigger_error(Ak::t('Could not find the file /app/controllers/%controller_file_name for '. 'the controller %controller_class_name', diff --git a/test/unit/lib/AkActionView/AkPhpCodeSanitizer.php b/test/unit/lib/AkActionView/AkPhpCodeSanitizer.php new file mode 100644 index 0000000..baec8ae --- /dev/null +++ b/test/unit/lib/AkActionView/AkPhpCodeSanitizer.php @@ -0,0 +1,51 @@ +assertInvalidCode(''); + $this->assertInvalidCode(''); + } + + function test_should_avoid_private_array_keys() + { + $this->assertInvalidCode(''); + $this->assertInvalidCode(''); + $this->assertInvalidCode(''); + $this->assertInvalidCode(''); + } + + function test_should_avoid_private_object_attributes() + { + $this->assertInvalidCode('_private; ?>'); + $this->assertInvalidCode('_private?>'); + $this->assertInvalidCode('public->_private]?>'); + $this->assertInvalidCode('{\'_private\'}?>'); + $this->assertInvalidCode('$variable_attr?>'); + } + + + /**/ + function assertValidCode($code) + { + $this->CodeSanitizer =& new AkPhpCodeSanitizer(); + $this->CodeSanitizer->setOptions(array('code'=>$code)); + $this->assertTrue($this->CodeSanitizer->isCodeSecure(), 'Secure code not accepted: '.$code); + } + + function assertInvalidCode($code) + { + $this->CodeSanitizer =& new AkPhpCodeSanitizer(); + $this->CodeSanitizer->setOptions(array('code'=>$code)); + $this->assertFalse($this->CodeSanitizer->isCodeSecure(), 'Unsecure code not detected: '.$code); + $this->assertErrorPattern('/You can\'t use/'); + } + +} + +?> diff --git a/vendor/PHPCodeAnalyzer/PHPCodeAnalyzer.php b/vendor/PHPCodeAnalyzer/PHPCodeAnalyzer.php index dbbcbb7..a0f518a 100644 --- a/vendor/PHPCodeAnalyzer/PHPCodeAnalyzer.php +++ b/vendor/PHPCodeAnalyzer/PHPCodeAnalyzer.php @@ -204,7 +204,7 @@ class PHPCodeAnalyzer else { list($id, $text) = $token; - if (isseT($handleMap[$id])) + if (isset($handleMap[$id])) { $call = $handleMap[$id]; $this->$call($id,$text); @@ -278,6 +278,10 @@ class PHPCodeAnalyzer $this->currentString = null; $this->currentStrings = null; break; + + case ']': + $this->useMemberVar(false); + break; } } @@ -466,15 +470,16 @@ class PHPCodeAnalyzer * we used a member variable record it * @access private */ - function useMemberVar() + function useMemberVar($reset = true) { - if (!isset($this->usedMemberVariables[$this->currentVar][$this->currentString])) - { + if (!isset($this->usedMemberVariables[$this->currentVar][$this->currentString])){ $this->usedMemberVariables[$this->currentVar][$this->currentString] = array(); } $this->usedMemberVariables[$this->currentVar][$this->currentString][] = $this->lineNumber; - $this->currentVar = false; - $this->currentString = null; + if($reset){ + $this->currentVar = false; + $this->currentString = null; + } } /** -- 2.11.4.GIT