500146 - XSS vuln in 'plugins' parameter on tiki-view_forum.php

Status: VERIFIED FIXED

(support.mozilla.org :: Knowledge Base Software, task)

Description

[Reed Loden [:reed]]

Found by hyperscan... testcase could use some work.

https://support.mozilla.com/tiki-view_forum.php?locale=en-US&comments_offset=0&comments_threadId=0&comments_threshold=0&thread_sort_mode=lastPost_desc&forumId=1&forum_advanced_post=y&comment_topictype=n&comments_data=advanced%20post%20in%20use&plugins=%22%20style%3d%22background%3aurl%28javascript%3aalert%28%27XSS%20Test%20Successful%27%29%3b%29%22%20OA%3d%22&comments_title=123456&fx_version=123456&userfile1=123456&anonymous_name=123456&anonymous_email=123456&comments_postComment=Post&comments_previewComment=Preview&comments_postCancel=Cancel

Comment 1

[Reed Loden [:reed]]

In forum_advanced_post.php --

$_REQUEST["long_description"] = rtrim(strip_tags($_REQUEST["long_description"]));
$_REQUEST["fx_version"] = strip_tags($_REQUEST["fx_version"]);
$_REQUEST["comments_title"] = strip_tags($_REQUEST["comments_title"]);
$_REQUEST["extensions"] = rtrim(strip_tags($_REQUEST["extensions"]));
$_REQUEST["os"] = strip_tags($_REQUEST["os"]);
$_REQUEST["plugins"] = strip_tags($_REQUEST["plugins"]);

$_REQUEST["comments_data"] = $_REQUEST["long_description"];

$_REQUEST["comments_data"] .= '';
if ($_REQUEST["extensions"]) {
        $_REQUEST["comments_data"] .= "\n\n" . "!! " . tra('Extensions installed: ');
        $_REQUEST["comments_data"] .= "\n\n";
        $_REQUEST["comments_data"] .= $_REQUEST["extensions"];
}
if ($_REQUEST["plugins"]) {
        $_REQUEST["comments_data"] .= "\n\n" . "!! " . tra('Plugins installed: ');
        $_REQUEST["comments_data"] .= "\n\n";
        $_REQUEST["comments_data"] .= $_REQUEST["plugins"];
}


:/

Comment 2

[Reed Loden [:reed]]

Attachment: patch - v1 (untested)

/me cries

Comment 3

[Reed Loden [:reed]]

If this is reviewed and landed today, can it make tomorrow's release?

Comment 4

[Laura Thomson :laura]

Comment on attachment 384837 [details] [diff] [review]
patch - v1 (untested)

Yes, it can make the release.  Commit ASAP.

Comment 5

[Reed Loden [:reed]]

r28394/r28395

ecooper, your review is always welcome for security fixes, too. always helps to have multiple pairs of eyes.

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Please let QA know the best way to verify this, thanks!

Comment 7

[Reed Loden [:reed]]

<h3 class="showhide_heading" id="_Plugins_installed_"> Plugins installed:</h3>

<p>" style="background:url(javascript:alert('XSS Test Successful');)" OA="
</p>

  <hr />

  </div>
  
  </div>
  
  
<div id="forumpost" style="display:block;">
      <form method="post" enctype="multipart/form-data" action="/tiki-view_forum.php?locale=en-US" id="editpageform">
    <input type="hidden" name="comments_offset" value="0" />
    <input type="hidden" name="comments_threadId" value="0" />
    <input type="hidden" name="comments_threshold" value="0" />
    <input type="hidden" name="thread_sort_mode" value="lastPost_desc" />
    <input type="hidden" name="forumId" value="1" />

		
<input type="hidden" name="forum_advanced_post" value="y" />
<input type="hidden" name="comment_topictype" value="n" />
<input type="hidden" name="comments_data" value="advanced post in use" />
<input type="hidden" name="plugins" value="" style="background:url(javascript:alert('XSS Test Successful');)" OA="" id="plugins" />



Both of the above things should be encoded instead of raw.

Comment 8

[Reed Loden [:reed]]

Sigh, I'm over-encoding.

 
  <div class="commenttext">
  

&lt;h3 class=&quot;showhide_heading&quot; id=&quot;_Plugins_installed_&quot;&gt; Plugins installed:&lt;/h3&gt;

&lt;p&gt;&quot; style=&quot;background:url(javascript:alert(&#039;XSS Test Successful&#039;);)&quot; OA=&quot;

&lt;/p&gt;

  <hr />
  </div>

Comment 9

[Reed Loden [:reed]]

Attachment: patch - v2 (untested)

How about this instead?

Comment 10

[Reed Loden [:reed]]

r28453/r28454

Seems ok... ready for QA.

Comment 11

[E. Cooper [:ecooper]]

Comment on attachment 385055 [details] [diff] [review]
patch - v2 (untested)

I know I'm wayyyy to late to be coming in with this, but using htmlspecialchars() on all the inputs in webroot/forum_advanced_post.php will cause the values to be double encoded in the <input> and <textarea> tags when they are rendered by smarty in forum_advanced_post.tpl. 

This will only be a problem if the user clicks 'Preivew' or tries to submit without all required fields completed. It won't affect the usual 'type then post' use cases.

Comment 12

[Reed Loden [:reed]]

(In reply to comment #11)
> (From update of attachment 385055 [details] [diff] [review])
> I know I'm wayyyy to late to be coming in with this, but using
> htmlspecialchars() on all the inputs in webroot/forum_advanced_post.php will
> cause the values to be double encoded in the <input> and <textarea> tags when
> they are rendered by smarty in forum_advanced_post.tpl. 

Indeed. This only affects a few of the variables, though. I'll track down which ones exactly.

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

We need this fixed so we can verify and tag for 1.2; I'm going to lunch, but either Krupa or myself will verify soon after it's fixed.

Comment 14

[E. Cooper [:ecooper]]

Attachment: v3

This removes the '|escape' from smarty variables already escaped via htmlspecialchars() in webroot/forum_advanced_post.php. 

And, as an added bonus, I fixed another XSS vulnerability involving the anon email address being pulled directly from the request.

Comment 15

[E. Cooper [:ecooper]]

r28557/r28558

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED as best I can tell on https://support-stage.mozilla.org/tiki-view_forum.php?locale=en-US&comments_offset=0&comments_threadId=0&comments_threshold=0&thread_sort_mode=lastPost_desc&forumId=1&forum_advanced_post=y&comment_topictype=n&comments_data=advanced%20post%20in%20use&plugins=%22%20style%3d%22background%3aurl%28javascript%3aalert%28%27XSS%20Test%20Successful%27%29%3b%29%22%20OA%3d%22&comments_title=123456&fx_version=123456&userfile1=123456&anonymous_name=123456&anonymous_email=123456&comments_postComment=Post&comments_previewComment=Preview&comments_postCancel=Cancel:

<input type="hidden" name="plugins" value="&quot; style=&quot;background:ur&lt;x&gt;l(ja&lt;x&gt;vascript:al&lt;x&gt;ert('XSS Test Successful');)&quot; OA=&quot;" id="plugins" />

Comment 17

[Reed Loden [:reed]]

Comment on attachment 385206 [details] [diff] [review]
v3

meh, I don't like doing half-and-half, but whatever, this is all horrible code that should be rewritten from scratch.

Comment 18

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.

Comment 19

[Will Kahn-Greene [:willkg] ET needinfo? me]

These bugs are all resolved, so I'm removing the security flag from them.