TJavaScript::quoteString() vulnerable to Unicode strings & TJavaScript::jsonEncode() doesn't check for errors

[ctrlaltca]

From google...@pcforum.hu on January 28, 2012 20:04:41

What steps will reproduce the problem? 1. Create a new page

2. Add the line "$this->Page->ClientScript->registerEndScript('abcd', 'alert(\''.TJavaScript::quoteString(hex2bin('920629d49ad739c034124f208d')).'\');');" to the onPreRender method
3. Run/load the page What is the expected output? What do you see instead? You'd expect a JavaScript message box to show. However, nothing happens. If you open up your browsers console you see that there is a parse/script error in the page (IE&FF complain about an unterminated string contant, Chrome about an invalid token.)

This happens because TJavaScript::quoteString() fails to properly encode some Unicode sequences. The example above contains a malformed sequence, but there might be other sequences which cause similar errors (as TJavaScript::quoteString encodes only some special characters, and does not use Unicode hex encoding at all). Also malformed Unicode sequences could be very well submitted by a web user to the Prado application, which then in turn could store that in a database or a file, and make itself vulnerable to XSS attacks when trying to use this malformed data in a quoted string, because of relying on TJavaScript::quoteString() to properly encode every possible sequence (which it actually does NOT).

As a solution I've devised to use TJavaScript::jsonEncode() to encode the string, which will 1. check the passed string for sanity and 2. is probably the fastest way to transcode PHP strings to escaped JavaScript strings in a Unicode-aware way, thus preventing the above XSS attacks.

However, upon trying to work around the original problem by calling TJavaScript::jsonEncode() I've realized that the latter calls json_encode() without checking afterwards whether there were any encoding errors. So if you pass a string with for ex. an invalid Unicode sequence (or an array or object containing one or more strings with invalid Unicode sequences) to it, even though it will refuse to encode the string - thus preventing the above or any other XSS attacks -, but will simply return 'null', without raising any error or exception. This, even though not a vulnerability per se, is not good, as obviously such encoding errors should be signaled to the application code - because if gone undetected, this malfunction might cause data-loss. So, I've added some error checks to TJavaScript::jsonEncode() which will now check whether the encoding was successful, and if not, will raise an appropriate exception for the application code to handle (or for the request to fail with an error).

These two measures together should prevent any possible XSS vulnerabilites using malformed string in conjuction with TJavaScript::quoteString() and also ::jsonEncode() conversion errors - for whatever reason - going undetected.

I've attached the patches TJavaScript.php for review.

Btw. TJavaScript::jsonDecode() has no error checks implemented either, but I didn't bother putting some in there, which however should be trivial to add. I've also noticed that it does no Unicode->Application code page conversion - which would be logical to do, if jsonEncode() does the opposite of that automatically. What version of the product are you using? On what operating system? Prado/trunk

Attachment: tjavascript-malformed-unicode-xss-fix.zip

Original issue: http://code.google.com/p/prado3/issues/detail?id=382

[ctrlaltca]

From ctrlal...@gmail.com on January 29, 2012 04:40:33

Quick notice: hex2bin doesn't still exists in php 5.3, so i've not been able to test your example; i tried using unpack("H*", ..) and hexdec(decbin(..)), but they didn't work (an alert was correctly displayed).

[ctrlaltca]

From google...@pcforum.hu on January 29, 2012 10:02:29

Are you using utf-8 encoding and content-type header for your pages?

[ctrlaltca]

From google...@pcforum.hu on January 29, 2012 10:19:01

Testcase attached. Reproduces using original trunk sources flawlessly. I'm using PHP 5.3.6/VC9, but that shouldn't matter.

Attachment: tjavascript-quotestring-fails-to-quote-invalid-unicode-strings-testpage.zip tjavascript-quotestring-fails-to-quote-invalid-unicode-strings.jpg tjavascript-quotestring-fails-to-quote-invalid-unicode-strings-chrome.jpg

[ctrlaltca]

From ctrlal...@gmail.com on January 29, 2012 12:23:52

I tested a bit the code involved and the first idea i had was to completely replace prado's TJavascript::encode() with jsonencode. This creates some easy-to-resolve issues (avoid encoding "javascript:" strings, workaround INF(inite), fix controls like TDatePicker), but there's also a bigger issue that needs to be solved. Json[en|de]code is only able to work on UTF8, while the actual TJavascript methods takes different approaches:

• encode() and quoteString() seems to have been charset-agnostic at first, but then quoteUTF8() has been added, caisung them to become UTF8-only;
• jsonEncode(), as you noticed, tries to convert from the Application->Globalization->Charset to UTF8 before encoding;
• jsonDecode() doesn's validate its input, and will fail if it's not UTF8;
• the same jsonEncode() and jsonDecode() behaviours appears in the TJSON php implementation.

Assuming that TJavaScript::encode() was written first as a low-computational-cost alternative to a full php json implementation (php's json extension was not available at the time of prado 3.0.0), and that nowadays json_encode is surely quickier than any php-based alternative, the idea of just trashing TJavascript::encode() can be meaningful.

This will make prado use UTF-8 only for javascript and ajax. I'd like to note that our globalization stuff is already UTF8-only. As an example, setting in application.xml

will make TDatePicker month and week days appear as mojibake.

So my idea would be to:

1. use json_encode() instead of encode();
2. fix the involved code;
3. deprecate TJSON to avoid people from using it; 4a. explicitly state in the documentation that everything related to Javascript uses UTF8, or 4b. add proper _toUTF8 and _fromUTF8 routines to both php and javascript to be able to convert from Application->Globalization->Charset to UTF8 and viceversa.

Sorry for derailing your ticket.

[ctrlaltca]

From ctrlal...@gmail.com on January 29, 2012 12:31:17

Sorry, i answered without seeing your comment #2 and #3. Yes i'm using UTF8, and here's a screenshot of the result:

Attachment: ss.jpg

[ctrlaltca]

From ctrlal...@gmail.com on February 05, 2012 09:35:03

Side note: TPage actually tries to decode the postData in processCallbackRequest:

    // Decode Callback postData from UTF-8 to current Charset
    if (($g=$this->getApplication()->getGlobalization(false))!==null &&
        strtoupper($enc=$g->getCharset())!='UTF-8')
            foreach ($this->_postData as $k=>$v)
                $this->_postData[$k]=iconv('UTF-8',$enc.'//IGNORE',$v);

[ctrlaltca]

From ctrlal...@gmail.com on February 09, 2012 07:34:48

I managed to reproduce the problem using your example code under osx.

[ctrlaltca]

From ctrlal...@gmail.com on February 09, 2012 08:46:23

Your patch has been committed in r3103 . Some parts have been changed:

• re-added mising JSMin() method;
• unified error control code for jsonEncode and jsonDecode, and added conditional checks for the use of json_last_error() that requires php5.3;
• changed quoteString() behaviour to actually add quotes around the string, avoiding the need to strip them off after json_encode()

Status: Fixed

[ctrlaltca]

From google...@pcforum.hu on February 10, 2012 14:34:19

Patch needs corrections in TJavaScript::checkJsonError():

1. "switch (json_last_error())" should be "switch ($err = json_last_error())" (otherwise there will be no $err for inclusion in the exception text)
2. also the text in the Exception next to the switch statement should be changed from "JSON encode error" to "JSON encode/decode error" as it is called both after encode and decode, and telling the developer that there was a "JSON encode error" whereas the error might have occoured (and is most likely to occour anyway) during a JSON-decode might be misleading, and make the developer to look for problems at the wrong places. even better solution would be to pass a string describing whether it was an encode/decode operation to the check function, and include that one in the exception text as well, so it would tell exactly whether the error was a decoding or encoding error.

[ctrlaltca]

From ctrlal...@gmail.com on February 11, 2012 00:40:48

Good point on 1, i just missed it. About point 2, i just removed the "encode" word from the error message, since the stack strace already tells the developer the exact place where the exception gets triggered:

0 prado/framework/Web/Javascripts/TJavaScript.php(216): TJavaScript::checkJsonError()

1 prado/framework/Web/Javascripts/TJavaScript.php(86): TJavaScript::jsonEncode('??)???9?4?O ?', 13)

2 testcase/protected/Pages/Home.php(10): TJavaScript::quoteString('??)???9?4?O ?')