patch for #382

5 files changed, 60 insertions, 43 deletions

diff --git a/HISTORY b/HISTORY
index bc7158d2..e6c1af35 100644
--- a/HISTORY
+++ b/HISTORY
@@ -64,6 +64,7 @@ BUG: Issue #377 - THtmlArea Template Pluggin Options Parse Error (ctrlaltca)
 BUG: Issue #379 - JSON float encoding depends on current locale (ctrlaltca)
 BUG: Issue #380 - TCustomValidator's ControlToValidate should be optional (ctrlaltca)
 BUG: Issue #381 - The property CausesValidation in the TActiveDatePicker not work (ctrlaltca)
+BUG: Issue #382	- TJavaScript::quoteString() vulnerable to Unicode strings & TJavaScript::jsonEncode() doesn't check for errors (gabor)
 BUG: Issue #383 - Some THttpRequest methods raise NOTICE level errors on missing headers (gabor)
 
 Version 3.1.10 Jul 17, 2011
diff --git a/UPGRADE b/UPGRADE
index e19e6239..c8e33929 100644
--- a/UPGRADE
+++ b/UPGRADE
@@ -42,6 +42,11 @@ Upgrading from v3.1.x
 - All the THttpRequest's methods used to gather server informations have been paired to return null if no
   information is available. Previously some of them returned an empty string (getQueryString and
   getHttpProtocolVersion), some other returned null, others caused a php NOTICE.
+- Some TJavaScript methods have been modified to clear their use and provide better xss protection:
+  the undocumented quoteUTF8() was removed, since it didn't provide any real protection;
+  quoteString() now safely adds quotes around a string: previously it only added escape characters;
+  the json* family of methods actually checks for errors and generate exceptions on fail (requires
+  at least php 5.3.3).
 
 Upgrading from v3.1.10
 ----------------------
diff --git a/framework/Web/Javascripts/TJavaScript.php b/framework/Web/Javascripts/TJavaScript.php
index c492c197..68908f9e 100644
--- a/framework/Web/Javascripts/TJavaScript.php
+++ b/framework/Web/Javascripts/TJavaScript.php
@@ -76,45 +76,14 @@ class TJavaScript
 

 	/**

 	 * Quotes a javascript string.

-	 * After processing, the string can be safely enclosed within a pair of

-	 * quotation marks and serve as a javascript string.

+	 * After processing, the string is safely enclosed within a pair of

+	 * quotation marks and can serve as a javascript string.

 	 * @param string string to be quoted

-	 * @param boolean whether this string is used as a URL

 	 * @return string the quoted string

 	 */

-	public static function quoteString($js,$forUrl=false)

+	public static function quoteString($js)

 	{

-		return self::quoteUTF8(($forUrl) ? strtr($js,array('%'=>'%25',"\t"=>'\t',"\n"=>'\n',"\r"=>'\r','"'=>'\"','\''=>'\\\'','\\'=>'\\\\')) : strtr($js,array("\t"=>'\t',"\n"=>'\n',"\r"=>'\r','"'=>'\"','\''=>'\\\'','\\'=>'\\\\')));

-	}

-

-	public static function quoteUTF8($str)

-	{

-		$entities = '';

-		$length = strlen($str);

-		$lookingFor = 1;

-		$unicode = array();

-		$values = array();

-

-		for($i=0;$i<$length;$i++) {

-			$thisValue = ord($str[$i]);

-			if($thisValue < 128)

-				$unicode[] = $thisValue;

-			else {

-				if(count($values)==0)

-					$lookingFor = ($thisValue<224) ? 2 : 3;

-				$values[] = $thisValue;

-				if(count($values)==$lookingFor) {

-					$unicode[] = ($lookingFor == 3) ? (($values[0]%16)*4096)+(($values[1]%64)*64)+($values[2]%64) : (($values[0]%32)*64)+($values[1]%64);

-					$values = array();

-					$lookingFor = 1;

-				}

-			}

-		}

-

-		foreach($unicode as $value)

-			$entities .= ($value < 128) ? chr($value) : '\u'.str_pad(dechex($value), 4, '0', STR_PAD_LEFT);

-

-		return $entities;

+		return self::jsonEncode($js,JSON_HEX_QUOT | JSON_HEX_APOS | JSON_HEX_TAG);

 	}

 

 	/**

@@ -176,7 +145,7 @@ class TJavaScript
 			if(self::isFunction($value))

 				return preg_replace('/^\s*javascript:/', '', $value);

 			else

-				return "'".self::quoteString($value)."'";

+				return self::quoteString($value);

 		}

 		else if(is_bool($value))

 			return $value?'true':'false';

@@ -235,7 +204,7 @@ class TJavaScript
 	 * @param mixed variable to be encoded

 	 * @return string encoded string

 	 */

-	public static function jsonEncode($value)

+	public static function jsonEncode($value, $options = 0)

 	{

 		if (function_exists('json_encode'))

 		{

@@ -243,7 +212,9 @@ class TJavaScript
 				($g=Prado::getApplication()->getGlobalization(false))!==null &&

 				strtoupper($enc=$g->getCharset())!='UTF-8')

 				$value=iconv($enc, 'UTF-8', $value);

-			return json_encode($value);

+			$s = json_encode($value,$options);

+			self::checkJsonError();

+			return $s;

 		}

 

 		if(self::$_json === null)

@@ -260,12 +231,52 @@ class TJavaScript
 	public static function jsonDecode($value)

 	{

 		if (function_exists('json_decode'))

-			return json_decode($value);

-

+		{

+			$s= json_decode($value);

+			self::checkJsonError();

+			return $s;

+		}

 		if(self::$_json === null)

 			self::$_json = Prado::createComponent('System.Web.Javascripts.TJSON');

 		return self::$_json->decode($value);

 	}

+	

+	private static function checkJsonError()

+	{

+		// requires php 5.3.0

+		if (function_exists('json_last_error'))

+		{

+			// requires php 5.3.3

+			if(!defined('JSON_ERROR_UTF8'))

+				define('JSON_ERROR_UTF8', null);

+

+			switch (json_last_error())

+			{

+				case JSON_ERROR_NONE:

+					return;

+					break;

+				case JSON_ERROR_DEPTH:

+					$msg = 'Maximum stack depth exceeded';

+    				break;

+				case JSON_ERROR_STATE_MISMATCH:

+					$msg = 'Underflow or the modes mismatch';

+    				break;

+				case JSON_ERROR_CTRL_CHAR:

+					$msg = 'Unexpected control character found';

+					break;

+				case JSON_ERROR_SYNTAX:

+					$msg = 'Syntax error, malformed JSON';

+    				break;

+				case JSON_ERROR_UTF8:

+					$msg = 'Malformed UTF-8 characters, possibly incorrectly encoded';

+					break;

+				default:

+					$msg = 'Unknown error';

+    				break;

+			}

+			throw new Exception("JSON encode error ($err): $msg");

+		}

+	}

 

 	/**

 	 * Minimize the size of a javascript script.

diff --git a/framework/Web/UI/TClientScriptManager.php b/framework/Web/UI/TClientScriptManager.php
index 045169f1..3ce458a1 100644
--- a/framework/Web/UI/TClientScriptManager.php
+++ b/framework/Web/UI/TClientScriptManager.php
@@ -388,7 +388,7 @@ class TClientScriptManager extends TApplicationComponent
 		if($target instanceof TControl)
 			$target=$target->getClientID();
 		$id = TJavaScript::quoteString($target);
-		$this->_endScripts['prado:focus'] = 'new Effect.ScrollTo("'.$id.'"); Prado.Element.focus("'.$id.'");';
+		$this->_endScripts['prado:focus'] = 'new Effect.ScrollTo('.$id.'); Prado.Element.focus('.$id.');';
 
 		$params=func_get_args();
 		$this->_page->registerCachingAction('Page.ClientScript','registerFocusControl',$params);
diff --git a/framework/Web/UI/WebControls/TReCaptchaValidator.php b/framework/Web/UI/WebControls/TReCaptchaValidator.php
index 2898a913..7199e401 100644
--- a/framework/Web/UI/WebControls/TReCaptchaValidator.php
+++ b/framework/Web/UI/WebControls/TReCaptchaValidator.php
@@ -100,8 +100,8 @@ class TReCaptchaValidator extends TBaseValidator
 		  {

 		    $cs->registerEndScript(

 				$this->getClientID().'::validate',

-				'$(\''.TJavaScript::quoteString($this->getClientID().'_1').'\').value = \''.TJavaScript::quoteString($value).'\';'.

-				'Prado.Validation.validateControl(\''.TJavaScript::quoteString($control->ClientID).'\');'

+				'$('.TJavaScript::quoteString($this->getClientID().'_1').').value = '.TJavaScript::quoteString($value).';'.

+				'Prado.Validation.validateControl('.TJavaScript::quoteString($control->ClientID).');'

 		    );

 

 		    if ($control->getVisible(true))