Bug 524547 (WH-3436428)

XSS in SUMO forum bare query string, comments_reply_thread_Id parameter, and comments_threshold parameter

VERIFIED FIXED in 1.4.2

Status

support.mozilla.org
Knowledge Base Software
--
critical
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: justdave, Assigned: jsocol)

Tracking

({wsec-xss})

unspecified
1.4.2
wsec-xss

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: sumo_only [WH-3436428 & WH-3436415 & WH-3367487] [infrasec:xss])

Attachments

(1 attachment, 1 obsolete attachment)

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
Whiteboard: WH-3436428
Alias: WH-3436428
Whiteboard: WH-3436428
Component: Forum → Knowledge Base Software
QA Contact: forum → kb-software
(Assignee)

Comment 1

9 years ago
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?
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).
Whiteboard: WH-3436428 & WH-3436415
(Assignee)

Comment 3

9 years ago
Ok, so then we are looking at the URLs, not the form fields, correct? IE 8 also URL-encodes the quotes, hence my confusion.
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.
(Assignee)

Updated

9 years ago
Assignee: nobody → james
Target Milestone: --- → 1.4.2
(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.
(Assignee)

Comment 6

9 years ago
(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.
(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! :)
(Assignee)

Comment 8

9 years ago
Created attachment 408634 [details] [diff] [review]
v1

Instead of taking the request URI, this patch builds new URIs, only including the data it needs (and escaping anything from the URI).
Attachment #408634 - Flags: review?(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.
Attachment #408634 - Flags: review?(reed) → review-
(Assignee)

Comment 10

9 years ago
(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.
(Assignee)

Comment 11

9 years ago
Created attachment 408921 [details] [diff] [review]
v2
Attachment #408634 - Attachment is obsolete: true
Attachment #408921 - Flags: review?(morgamic)
(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!
(Assignee)

Comment 13

9 years ago
(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.
(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 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?
Attachment #408921 - Flags: review?(morgamic)
Attachment #408921 - Flags: review?(morgamic)
(Assignee)

Comment 16

9 years ago
(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.
Whiteboard: WH-3436428 & WH-3436415 → WH-3436428 & WH-3436415 & WH-3367487
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.
Attachment #408921 - Flags: review?(morgamic) → review+
(Assignee)

Comment 18

9 years ago
I e-mailed security@tikiwiki.org and they're looking into it.
(Assignee)

Comment 19

9 years ago
r54581 (trunk)
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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" />
Status: RESOLVED → VERIFIED
(Assignee)

Comment 21

9 years ago
r54896 (prod)
(Assignee)

Updated

9 years ago
Whiteboard: WH-3436428 & WH-3436415 & WH-3367487 → sumo_only
Whiteboard: sumo_only → sumo_only [WH-3436428 & WH-3436415 & WH-3367487]
Whiteboard: sumo_only [WH-3436428 & WH-3436415 & WH-3367487] → sumo_only [WH-3436428 & WH-3436415 & WH-3367487] [infrasec:xss]
Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.
Keywords: wsec-xss
These bugs are all resolved, so I'm removing the security flag from them.
Group: websites-security
You need to log in before you can comment on or make changes to this bug.