Bug 38922 - innerHTML decompilation issues in textarea
Summary: innerHTML decompilation issues in textarea
Status: RESOLVED FIXED
Alias: None
Product: Security
Classification: Unclassified
Component: Security (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL: http://code.google.com/p/chromium/iss...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-11 12:15 PDT by eduardo
Modified: 2012-05-10 08:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.97 KB, patch)
2010-05-12 10:06 PDT, Abhishek Arya
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eduardo 2010-05-11 12:15:23 PDT
Chrome and Safari (ergo, webkit) have a problem when they decompile 
innerHTML. This can be abused to create XSS attacks.

When you read:

node.innerHTML of a textarea, the chars inside are not encoded properly.

PoC:

Put this:
<textarea>&lt;/textarea&gt;&lt;script&gt;alert(1)&lt;/script&gt;</textarea>

Here:
http://0x.lv/innerHTMLinspect.html

Greetings!!
Comment 1 David Kilzer (:ddkilzer) 2010-05-11 12:43:58 PDT
<rdar://problem/7969861>
Comment 2 Darin Adler 2010-05-11 16:59:18 PDT
The problem seems to be in the appendStartMarkup function in markup.cpp in the Node::TEXT_NODE case, the appendUCharRange call. A test case for this should cover the <script>, <style>, <textarea>, and <xmp> elements and should be tested in other browsers to make sure their escaping behavior is the same.
Comment 3 Abhishek Arya 2010-05-11 17:05:57 PDT
I was planning to fix this, and it looks only on textarea and not on other elements. I compared the behavior with IE and we match on everything except textarea. For textarea, we need to html entity encode the output.

<body>
<script id=a1>//&lt;/textarea&gt;&lt;script&gt;alert(1)&lt;/script&gt;<def>
</script>
<style id=a2>&lt;/textarea&gt;&lt;script&gt;alert(1)&lt;/script&gt;<def></style>
<textarea id=a3>&lt;/textarea&gt;&lt;script&gt;alert(1)&lt;/script&gt;<def></textarea>
<xmp id=a4>&lt;/textarea&gt;&lt;script&gt;alert(1)&lt;/script&gt;<def></xmp>
<script>
alert(document.getElementById('a1').innerHTML);
alert(document.getElementById('a2').innerHTML);
alert(document.getElementById('a3').innerHTML);
alert(document.getElementById('a4').innerHTML);
</script>
</body>

I can fix this. in the layout test, i will include the other elements as well.
Comment 4 Abhishek Arya 2010-05-11 17:10:50 PDT
Both IE and Firefox produce the output as 
&lt;/textarea&gt;&lt;script&gt;alert(1)&lt;/script&gt;&lt;def&gt; 

which means that it html entity encodes everything.
Comment 5 Abhishek Arya 2010-05-12 10:06:29 PDT
Created attachment 55864 [details]
Patch
Comment 6 Abhishek Arya 2010-05-12 10:10:08 PDT
Darin, can you please review the patch and commit if it looks ok.

Some notes:
1. checked that appendUCharRange is not used anywhere else in file other than textnodes.
2. checked that createMarkup call is used only for innerhtml and outerhtml calls.
3. outerHTML is not supported by Firefox, so last 4 don't match.
4. First 4 match in Firefox.
5. Everything except case 3 matches in IE.
(a) reason for case 3 not match is IE does not support &apos; and fails during decoding it in textarea.
(b) last 4 in IE are same except the fact that Tag name become capitalized e.g. <SCRIPT>. not a big deal for security :)
6. First 2 cases show fail in IE whereas their value is same since === used in shouldbe function does not work well in IE.
FAIL innerHTML("script") should be /*&quot;&apos;&amp;&lt;&gt;&#34;&#39;&#38;&#60;&#62;"'&<>*/. Was            /*&quot;&apos;&amp;&lt;&gt;&#34;&#39;&#38;&#60;&#62;"'&<>*/.
FAIL innerHTML("style") should be /*&quot;&apos;&amp;&lt;&gt;&#34;&#39;&#38;&#60;&#62;"'&<>*/. Was /*&quot;&apos;&amp;&lt;&gt;&#34;&#39;&#38;&#60;&#62;"'&<>*/.
7. all browsers don't encode " and ' in textarea, but that is ok.
8. tested safari mac, all layout tests pass. checked that my layout tests passes on qt, gtk,chromium,safari win (although it should not matter).
Comment 7 Darin Adler 2010-05-12 10:35:07 PDT
Sure, I’ll land it.
Comment 8 Darin Adler 2010-05-12 10:38:31 PDT
I am going to rename these tests and put them in a different directory. There is no reason to include the words "XSS" in the test, and these do not relate to encoding. Also, we do not want the HTML file to be executable.
Comment 9 Abhishek Arya 2010-05-12 10:41:29 PDT
Thanks a lot Darin.
Comment 10 Darin Adler 2010-05-12 11:13:15 PDT
Committed r59241: <http://trac.webkit.org/changeset/59241>
Comment 11 Darin Adler 2010-05-12 11:16:16 PDT
Fixed ChangeLog entry in <http://trac.webkit.org/changeset/59242>.