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

[GoogleCodeExporter]

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

Original issue reported on code.google.com by google...@pcforum.hu on 28 Jan 2012 at 7:04

Attachments:

• tjavascript-malformed-unicode-xss-fix.zip

[GoogleCodeExporter]

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).

Original comment by ctrlal...@gmail.com on 29 Jan 2012 at 12:40

[GoogleCodeExporter]

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

Original comment by google...@pcforum.hu on 29 Jan 2012 at 6:02

[GoogleCodeExporter]

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

Original comment by google...@pcforum.hu on 29 Jan 2012 at 6:19

Attachments:

• 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

[GoogleCodeExporter]

I tested a bit the code involved and the first idea i had was to completely 
replace prado's TJavascript::encode() with json_encode.
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
  <module id="globalization" class="TGlobalization" Culture="zh" Charset="GB-2312" />
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.

Original comment by ctrlal...@gmail.com on 29 Jan 2012 at 8:23

[GoogleCodeExporter]

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

Original comment by ctrlal...@gmail.com on 29 Jan 2012 at 8:31

Attachments:

• ss.jpg

[GoogleCodeExporter]

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);

Original comment by ctrlal...@gmail.com on 5 Feb 2012 at 5:35

[GoogleCodeExporter]

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

Original comment by ctrlal...@gmail.com on 9 Feb 2012 at 3:34

[GoogleCodeExporter]

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()

Original comment by ctrlal...@gmail.com on 9 Feb 2012 at 4:46

• Changed state: Fixed

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 10 Feb 2012 at 10:34

[GoogleCodeExporter]

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 
?')

Original comment by ctrlal...@gmail.com on 11 Feb 2012 at 8:40