559172 - (WH-3768445) Reflected XSS on tiki-list_file_gallery.php due to lack of Output Encoding

Status: VERIFIED FIXED

(support.mozilla.org :: Mobile, task)

Description

[Michael Coates [:mcoates] (acct no longer active)]

Issue

A reflected cross site scripting vulnerability is present within
https://mobile.support.mozilla.com/tiki-list_file_gallery.php. Data accepted from the
user via URL parameters are returned within the HTML response without
appropriate HTML encoding. Note: Tiki-wiki using blacklist filtering to make
XSS attacks difficult; however, this filtering should not be relied upon to
prevent all XSS variations. The root cause identified in this bug needs to be
fixed.

Recommended Remediation

The primary recommendation is to perform HTML entity encoding on any user
supplied data that is returned within the HTML response. In this case, the data
obtained from the "file_find" URL parameter.

In addition, perform input validation against the file_find field to
limit the types of characters which are accepted by the application. Utilize a
positive approach where acceptable character types are defined. If data does
not match this "positive" filter then the entire data string is rejected.


Proof of Concept
1. Login with a valid account
2. Follow the url below

https://mobile.support.mozilla.com/tiki-list_file_gallery.php?galleryId=1&file_find=%00%22whscheck=%22whscheck&search=Find&file_sort_mode=created_desc

3. View the source of the page and observe that the user supplied data has
broken out of the href attribute. 

e.g. <a class="tableheading" href="/tiki-list_file_gallery.php?galleryId=1&amp;file_find="whscheck="whscheck&amp;file_sort_mode=fileId_desc">ID</a>

An attacker can refine the input to inject a XSS attack.

Comment 1

[James Socol [:jsocol, :james]]

I believe a patch for this exists in the desktop version. Will find it and apply it to the mobile branch.

Comment 2

[Paul Craciunoiu [:paulc]]

Attachment: escape $find variable in template

I've reproduced this bug locally on trunk, and looking at svn history, I don't see any changes made to list-file_gallery and included templates or the main php file. This patch just escapes the $find variable in the template.

Comment 3

[Paul Craciunoiu [:paulc]]

Attachment: v2, explicitly escape:'url' when inside href=""

Same as previous patch except uses escape:'url' inside links, which... is everywhere it wasn't escaped.

Comment 4

[James Socol [:jsocol, :james]]

Comment on attachment 438870 [details] [diff] [review]
v2, explicitly escape:'url' when inside href=""

>Index: webroot/templates/list_file_gallery.tpl
>===================================================================
>--- webroot/templates/list_file_gallery.tpl	(revision 65770)
>+++ webroot/templates/list_file_gallery.tpl	(working copy)
>@@ -37,60 +37,60 @@
> <td  class="heading">&nbsp;</td>
> {/if}
> {if $gal_info.show_id eq 'y'}
>-	<td class="heading"><a class="tableheading" href="{$smarty.server.PHP_SELF}?galleryId={$gal_info.galleryId}{if isset($file_info)}&amp;fileId={$file_info.fileId}{/if}{if $offset ne 0}&amp;{$ext}offset={$offset}{/if}{if $find}&amp;{$ext}find={$find}{/if}{if isset($page)}&amp;page={$page}{/if}&amp;{$ext}sort_mode={if $sort_mode eq 'fileId_desc'}fileId_asc{else}fileId_desc{/if}{if $filegals_manager eq 'y'}&filegals_manager{/if}">{tr}ID{/tr}</a></td>
>+	<td class="heading"><a class="tableheading" href="{$smarty.server.PHP_SELF}?galleryId={$gal_info.galleryId}{if isset($file_info)}&amp;fileId={$file_info.fileId}{/if}{if $offset ne 0}&amp;{$ext}offset={$offset}{/if}{if $find}&amp;{$ext}find={$find|escape:'url'}{/if}{if isset($page)}&amp;page={$page|escape}{/if}&amp;{$ext}sort_mode={if $sort_mode eq 'fileId_desc'}fileId_asc{else}fileId_desc{/if}{if $filegals_manager eq 'y'}&filegals_manager{/if}">{tr}ID{/tr}</a></td>

Please also escape the other parameters here: $gal_info.galleryId, $file_info.fileId, $offset, $ext, and fix $page|escape. ($smarty.server.PHP_SELF should be fine, as it's set by PHP.) There may well be other un- or improperly- escaped variables in URLs. You should grab them all while you're in here.

Comment 5

[Paul Craciunoiu [:paulc]]

Attachment: more and more escapes

As per your wise request, I escaped almost everything and also fixed some |escape typos, mostly for $page.

Comment 6

[James Socol [:jsocol, :james]]

Comment on attachment 438907 [details] [diff] [review]
more and more escapes

Your enthusiasm for escaping is good. Unfortunately, in your fervor you've encoded paths that will now break.

Comment 7

[Paul Craciunoiu [:paulc]]

Attachment: same as previous, but doesn't escape $download_path and $url_path

No more path escaping. Also checked that we don't use these features, just ftr.

Comment 8

[James Socol [:jsocol, :james]]

Comment on attachment 438910 [details] [diff] [review]
same as previous, but doesn't escape $download_path and $url_path

It's still a little aggressive but since we're not using this feature, safe > functional.

Comment 9

[Paul Craciunoiu [:paulc]]

r65795 on fennec branch.

Comment 10

[Michael Coates [:mcoates] (acct no longer active)]

Thanks! Marked for retest in Sentinel

Comment 11

[James Socol [:jsocol, :james]]

(In reply to comment #10)
> Thanks! Marked for retest in Sentinel

This change has only been pushed to our staging server. See my comments in bug 556544.

Comment 12

[Michael Coates [:mcoates] (acct no longer active)]

Reopening bug. Will close when fix is confirmed in production.

Comment 13

[James Socol [:jsocol, :james]]

We don't push to production until it's VERIFIED.

Comment 14

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

Loading https://mobile-support-stage.mozilla.com/tiki-list_file_gallery.php?galleryId=1&file_find=%22whscheck%3D%22whscheck&search=Find&file_sort_mode=created_desc, I now see:

<td class="heading"><a class="tableheading" href="/tiki-list_file_gallery.php?galleryId=1&amp;file_find=%22whscheck%3D%22whscheck&amp;file_sort_mode=fileId_desc">ID</a>

Verified FIXED (as I understand it); Michael, mind double-checking on staging?  Thanks!

Comment 15

[Michael Coates [:mcoates] (acct no longer active)]

That looks correct to me.  I don't have access to stage (just requested it), but what I see in comment 14 looks like the correct behavior.

Comment 16

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

Thx; verified.

Comment 17

[James Socol [:jsocol, :james]]

(In reply to comment #15)
> That looks correct to me.  I don't have access to stage (just requested it),
> but what I see in comment 14 looks like the correct behavior.

You don't need access to our staging servers. The password prompt is just to keep search crawlers out: the username/password is "support"/"stage".

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.