524547 - (WH-3436428) XSS in SUMO forum bare query string, comments_reply_thread_Id parameter, and comments_threshold parameter

Status: VERIFIED FIXED

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

Description

[Dave Miller [:justdave]]

The following URLs are identified by the Sentinel scanner as causing XSS exposure:

https://support.mozilla.com/en-US/forum/3/471316?"whscheck="whscheck
https://support.mozilla.com/en-US/forum/3/478887?"whscheck="whscheck
https://support.mozilla.com/en-US/forum/3/478887?forumId=3&comments_threshold=0&comments_reply_threadId=478927&comments_offset=0&"whscheck="whscheck&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1
https://support.mozilla.com/en-US/forum/3/478887?forumId=3&comments_threshold=0&comments_reply_threadId="whscheck="whscheck&comments_offset=0&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1
https://support.mozilla.com/en-US/forum/3/478887?forumId=3&comments_threshold="whscheck="whscheck&comments_reply_threadId=478927&comments_offset=0&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1
https://support.mozilla.com/en-US/forum/3/478887?forumId=3&"whscheck="whscheck&comments_threshold=0&comments_reply_threadId=478927&comments_offset=0&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1

Comment 1

[James Socol [:jsocol, :james]]

The quotes are escaped in all cases (either URL encoded to %22 or entity encoded to "). The issue is that the value of the hidden fields "comments_threshold" and/or "comments_reply_threadId" is set to ""whscheck="whscheck". In either case, with the URLs above, attempting to post results in an error ("Forum is flat...").

Removing "comments_grandParentId" from the URLs above allows the post through, and the resulting page does not contain the string "whscheck".

Reed: what behavior would you like to see here?

Comment 2

[Dave Miller [:justdave]]

The quote get escaped by Firefox.  Not every browser quotes them.  You need to pretend they got through intact (if you hit the URL with wget or curl you'll be able to emulate that).

Comment 3

[James Socol [:jsocol, :james]]

Ok, so then we are looking at the URLs, not the form fields, correct? IE 8 also URL-encodes the quotes, hence my confusion.

Comment 4

[Dave Miller [:justdave]]

The output shown from sentinel shows the following output for the first/second URLs:

line 215: &nbsp;<a href="/en-US/forum/3/478887"whscheck="whscheck#threadId478887" title="Link to this post." class="permalink">#</a>  

line 326: &nbsp;<a href="/en-US/forum/3/478887"whscheck="whscheck#threadId478927" title="Link to this post." class="permalink">#</a>

line 418: &nbsp;<a href="/en-US/forum/3/478887"whscheck="whscheck#threadId478929" title="Link to this post." class="permalink">#</a>  

for the third one:

line 215: &nbsp;<a href="/en-US/forum/3/478887?forumId=3&comments_threshold=0&comments_reply_threadId=478927&comments_offset=0&"whscheck="whscheck&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1#threadId478887" title="Link to this post." class="permalink">#</a>

line 328: &nbsp;<a href="/en-US/forum/3/478887?forumId=3&comments_threshold=0&comments_reply_threadId=478927&comments_offset=0&"whscheck="whscheck&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1#threadId478927" title="Link to this post." class="permalink">#</a>

line 420: &nbsp;<a href="/en-US/forum/3/478887?forumId=3&comments_threshold=0&comments_reply_threadId=478927&comments_offset=0&"whscheck="whscheck&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=478887&comments_parentId=478887&thread_style=commentStyle_plain&post_reply=1#threadId478929" title="Link to this post." class="permalink">#</a>

The rest are all pretty much the same, the exposed parameters are in the same 3 places each time.

Comment 5

[Reed Loden [:reed]]

(In reply to comment #1)
> Reed: what behavior would you like to see here?

I'd like to see the URLs all escaped, as mentioned above (basically, use the escape:'url' filter). Should be a pretty easy fix. Feel free to tag me for review if nobody else is available.

Comment 6

[James Socol [:jsocol, :james]]

(In reply to comment #5)
> I'd like to see the URLs all escaped, as mentioned above (basically, use the
> escape:'url' filter). Should be a pretty easy fix. Feel free to tag me for
> review if nobody else is available.

I was unclear before since the two browsers I checked happened to both escape the URLs. Those URLs don't actually require any of the query string, so I'll just remove all the irrelevant data.

Comment 7

[Reed Loden [:reed]]

(In reply to comment #6)
> Those URLs don't actually require any of the query string, so I'll
> just remove all the irrelevant data.

That's even better! :)

Comment 8

[James Socol [:jsocol, :james]]

Attachment: v1

Instead of taking the request URI, this patch builds new URIs, only including the data it needs (and escaping anything from the URI).

Comment 9

[Reed Loden [:reed]]

Comment on attachment 408634 [details] [diff] [review]
v1

>-			&nbsp;<a href="{$smarty.server.REQUEST_URI}#threadId{$comment.threadId}" title="{tr}Link to this post.{/tr}" class="permalink">#</a>
>+			&nbsp;<a href="{if $prefs.feature_forum_friendly_urls eq 'y'}{$tikiroot}en-US/forum/{$forum_info.forumId}/{$thread_info.threadId}{if $smarty.request.comments_offset > 1}?comments_offset={$smarty.request.comments_offset|escape:"url"}{/if}{else}{$tikiroot}tiki-view_forum_thread.php?forumId={$forum_info.forumId}&amp;comments_parentId={$thread_info.threadId}{if $smarty.request.comments_offset > 1}&amp;comments_offset={$smarty.request.comments_offset|escape:"url"}{/if}{/if}#threadId{$comment.threadId}" title="{tr}Link to this post.{/tr}" class="permalink">#</a>

Let's break down the logic for my benefit:

{if $prefs.feature_forum_friendly_urls eq 'y'}
  {$tikiroot}en-US/forum/{$forum_info.forumId}/{$thread_info.threadId}
  {if $smarty.request.comments_offset > 1}
    ?comments_offset={$smarty.request.comments_offset|escape:"url"}
  {/if}
{else}
  {$tikiroot}tiki-view_forum_thread.php?forumId={$forum_info.forumId}&amp;comments_parentId={$thread_info.threadId}
  {if $smarty.request.comments_offset > 1}
    &amp;comments_offset={$smarty.request.comments_offset|escape:"url"}
  {/if}
{/if}
#threadId{$comment.threadId}

Why are you hardcoding en-US? That seems wrong... Shouldn't the actual locale be used here?

I'm trusting that you've ensured {$forum_info.forumId}, {$thread_info.threadId}, and {$comment.threadId} can't be used for XSS.

Nit: Use |escape:'url'| (single quotes rather than double quotes). This helps with syntax colorization tools and figuring out where quotes start and end (especially since the entire URL is encapsulated in double quotes already).

You also need to fix templates/styles/mozfr/comment-body.tpl.

It would probably be wise to grep for $smarty.server.REQUEST_URI and see if any other templates need to be fixed because of this same problem. Could you do that, please?

Sorry about the delay.

Comment 10

[James Socol [:jsocol, :james]]

(In reply to comment #9)
> Why are you hardcoding en-US? That seems wrong... Shouldn't the actual locale
> be used here?

This is an order of magnitude simpler in the next patch.

> I'm trusting that you've ensured {$forum_info.forumId},
> {$thread_info.threadId}, and {$comment.threadId} can't be used for XSS.

This data comes out of INT columns in the database and is not overwritten by request data, but I threw |escape:'url'| on the remaining calls anyway.

> Nit: Use |escape:'url'| (single quotes rather than double quotes). This helps
> with syntax colorization tools and figuring out where quotes start and end
> (especially since the entire URL is encapsulated in double quotes already).

My "IDE" (vim) has no problem highlighting this with double quotes, but OK.

> You also need to fix templates/styles/mozfr/comment-body.tpl.

This is a symlink to templates/styles/mozkb/comment-body.tpl

> It would probably be wise to grep for $smarty.server.REQUEST_URI and see if any
> other templates need to be fixed because of this same problem. Could you do
> that, please?

# grep -rl 'smarty.server.REQUEST_URI' webroot/ | grep -v '.svn'
webroot/templates/wiki-plugins/wikiplugin_subscribegroups.tpl
webroot/templates/tiki-show_page.tpl
webroot/templates/modules/mod-freetag.tpl
webroot/templates/modules/mod-change_category.tpl
webroot/templates/tracker_item_field_value.tpl
webroot/templates/tiki-top_bar.tpl
webroot/tiki-tell_a_friend.php

We don't use the wikiplugin and I'm unsure of how it works.
Escaped in tiki-show_page.tpl.
mod-freetag.tpl and mod-change_category.tpl don't seem to be used at all.
We don't use trackers and so I don't know how they work.
Escaped in top_bar.tpl.
Escaped (and commented out) in tell_a_friend.php.

I've filed bugs at TikiWiki to fix the issue in sections irrelevant to SUMO.

Comment 11

[James Socol [:jsocol, :james]]

Attachment: v2

Comment 12

[Reed Loden [:reed]]

(In reply to comment #10)
> (In reply to comment #9)
> > Why are you hardcoding en-US? That seems wrong... Shouldn't the actual locale
> > be used here?
> 
> This is an order of magnitude simpler in the next patch.

Indeed, the new patch looks much better. Since you tagged morgamic for review, I'll leave it alone.

> > I'm trusting that you've ensured {$forum_info.forumId},
> > {$thread_info.threadId}, and {$comment.threadId} can't be used for XSS.
> 
> This data comes out of INT columns in the database and is not overwritten by
> request data, but I threw |escape:'url'| on the remaining calls anyway.

Well, if you don't think it's needed (and have confirmed it, as you have), then I don't see any reason to escape it.

> My "IDE" (vim) has no problem highlighting this with double quotes, but OK.

Mine sadly does. :(

> > You also need to fix templates/styles/mozfr/comment-body.tpl.
> 
> This is a symlink to templates/styles/mozkb/comment-body.tpl

Ah, my bad. My `find` output for "-name comment-body.tpl" just showed it, and I didn't look to see what type of file it was.

> We don't use the wikiplugin and I'm unsure of how it works.

Any reason why it shouldn't just be deleted?

> mod-freetag.tpl and mod-change_category.tpl don't seem to be used at all.

Delete?

> We don't use trackers and so I don't know how they work.

delete?

> Escaped in tiki-show_page.tpl.
> Escaped in top_bar.tpl.
> Escaped (and commented out) in tell_a_friend.php.

I don't see these fixes in the new patch. Are you fixing them elsewhere?

> I've filed bugs at TikiWiki to fix the issue in sections irrelevant to SUMO.

Awesome!

Comment 13

[James Socol [:jsocol, :james]]

(In reply to comment #12)
> > My "IDE" (vim) has no problem highlighting this with double quotes, but OK.
> 
> Mine sadly does. :(

vim ftw.

> > We don't use the wikiplugin and I'm unsure of how it works.
> 
> Any reason why it shouldn't just be deleted?
> 
> > mod-freetag.tpl and mod-change_category.tpl don't seem to be used at all.
> 
> Delete?
> 
> > We don't use trackers and so I don't know how they work.
> 
> delete?

Without better coverage from our automated test suites, I'm loath to delete files when I don't know their function.

> > Escaped in tiki-show_page.tpl.
> > Escaped in top_bar.tpl.
> > Escaped (and commented out) in tell_a_friend.php.
> 
> I don't see these fixes in the new patch. Are you fixing them elsewhere?

I should say, they are _already_ escaped in these places.

Comment 14

[Reed Loden [:reed]]

(In reply to comment #13)
> Without better coverage from our automated test suites, I'm loath to delete
> files when I don't know their function.

Maybe file a bug to look at deleting them (and other unused templates) in the future? Your call...

> > > Escaped in tiki-show_page.tpl.
> > > Escaped in top_bar.tpl.
> > > Escaped (and commented out) in tell_a_friend.php.
> > 
> > I don't see these fixes in the new patch. Are you fixing them elsewhere?
> 
> I should say, they are _already_ escaped in these places.

Ah, ok. Thanks again for checking that out. I hate playing whack-a-mole, so always good to check all the templates for common problems that get discovered.

Comment 15

[Michael Morgan [:morgamic]]

Comment on attachment 408921 [details] [diff] [review]
v2

Are you sure urlencode on $_GET is going to escape this enough that you can trust it?  Is it being globally parsed elsewhere?

Comment 16

[James Socol [:jsocol, :james]]

(In reply to comment #15)
> Are you sure urlencode on $_GET is going to escape this enough that you can
> trust it?  Is it being globally parsed elsewhere?

Yes, it's only used in a Location: header and injecting headers doesn't work. From the PHP manual, since PHP 5.1.2:

> This function now prevents more than one header to be sent at once as a protection against header injection attacks.

If there's a possible attack vector here, I don't know it, but I'd like to.

Comment 17

[Michael Morgan [:morgamic]]

Comment on attachment 408921 [details] [diff] [review]
v2

Ok - thought urlencode stuff went to HTML at first, sorry.  I think this looks good, but don't forget to let TW guys know about other instances of REQUEST_URI and those probably never-used template files.

Comment 18

[James Socol [:jsocol, :james]]

I e-mailed [e-mail] and they're looking into it.

Comment 19

[James Socol [:jsocol, :james]]

r54581 (trunk)

Comment 20

[Vishal Kamdar [:vish_moz]]

Verified, FIXED.
The URL's are now escaped. For e.g. the following url:
https://support-stage.mozilla.org/en-US/forum/3/388579?forumId=3&comments_threshold=%22whscheck=%22whscheck&comments_reply_threadId=388579&comments_offset=0&thread_sort_mode=commentDate_asc&comments_per_page=20&comments_grandParentId=388579&comments_parentId=388579&thread_style=commentStyle_plain&post_reply=1
gives you this in the source code:
<input type="hidden" name="comments_threshold" value="&quot;whscheck=&quot;whscheck" />

Comment 21

[James Socol [:jsocol, :james]]

r54896 (prod)

Comment 22

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

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

Comment 23

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

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