556544 - (WH-3730327) XSS in tiki-browse_categories.php via parentId p win various params

Status: VERIFIED FIXED

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

Description

[Chris Lyon [:clyon]]

Sentinel has detected an XSS in the parameter "parentId" in parentId tiki-browse_categories.php. 


sentinel get:

https:// ​mobile.support.mozilla.com/​tiki-browse_categories.php?​ parentId=<whscheck>&​watch_event=category_changed&​ watch_object=11&​deep=off&​watch_action=add 

https:// ​mobile.support.mozilla.com/​tiki-browse_categories.php?​ locale=en-US&​find=&​deep=off&​type=&​ parentId=<whscheck>&​offset=10&​sort_mode=name_asc

Comment 1

[Chris Lyon [:clyon]]

(In reply to comment #0)
> Sentinel has detected an XSS in the parameter "parentId" in parentId
> tiki-browse_categories.php. 
> 
> 
> sentinel get:
> 
> https:// ​mobile.support.mozilla.com/​tiki-browse_categories.php?​
> parentId=<whscheck>&​watch_event=category_changed&​
> watch_object=11&​deep=off&​watch_action=add 
> 
> https:// ​mobile.support.mozilla.com/​tiki-browse_categories.php?​
> locale=en-US&​find=&​deep=off&​type=&​
> parentId=<whscheck>&​offset=10&​sort_mode=name_asc

Do you guys have what you need in order to get this fixed to? This has been sitting since 4/1 without any action.

Comment 2

[James Socol [:jsocol, :james]]

I do, and am working on a fix now. 1.5.4 should go out next Thursday.

Comment 3

[James Socol [:jsocol, :james]]

Attachment: patch, urlencodes output

Adds |escape:'url' several dozen places. Some where escaped incorrectly, others were unescaped.

Attaching two bits of SQL, again for mobile and desktop, that also contain fixes.

Comment 4

[James Socol [:jsocol, :james]]

Attachment: SQL to fix dynamic content on desktop

SQL to update dynamic content to properly escaped content.

Comment 5

[James Socol [:jsocol, :james]]

Attachment: SQL to fix dynamic content on mobile

More SQL.

Comment 6

[Paul Craciunoiu [:paulc]]

Comment on attachment 438877 [details] [diff] [review]
patch, urlencodes output

>Index: webroot/templates/tiki-browse_categories.tpl
>===================================================================
>--- webroot/templates/tiki-browse_categories.tpl	(revision 65751)
>+++ webroot/templates/tiki-browse_categories.tpl	(working copy)
>-<a class="categpath" href="tiki-browse_categories.php?parentId={$path[x].categId}&amp;deep={$deep}&amp;type={$type|escape}">{$path[x].name|tr_if}</a>
>+<a class="categpath" href="tiki-browse_categories.php?parentId={$path[x].categId|escape:'url'}&amp;deep={$deep|escape:'url'}&amp;type={$type|escape}">{$path[x].name|tr_if}</a>
Missed $type right here. Also, |*.categId| is escaped here...

>-<a class="categpath" href="tiki-browse_categories.php?parentId={$paths[x][y].categId}&amp;deep={$deep}&amp;type={$type|escape}">{$paths[x][y].name|tr_if}</a>
>+<a class="categpath" href="tiki-browse_categories.php?parentId={$paths[x][y].categId}&amp;deep={$deep|escape:'url'}&amp;type={$type|escape}">{$paths[x][y].name|tr_if}</a>
... but not here.

>Index: webroot/templates/styles/mozkb/tiki-browse_categories.tpl
>===================================================================
>--- webroot/templates/styles/mozkb/tiki-browse_categories.tpl	(revision 65751)
>+++ webroot/templates/styles/mozkb/tiki-browse_categories.tpl	(working copy)
>-<a class="neatlink" href="tiki-browse_categories.php?find={$find|escape}&amp;deep={$deep|escape}&amp;type={$type}&amp;parentId={$parentId|escape}&amp;offset={$prev_offset|escape}&amp;sort_mode={$sort_mode|escape}">&lt;&lt;</a>&nbsp;
>+<a class="neatlink" href="tiki-browse_categories.php?find={$find|escape:'url'}&amp;deep={$deep|escape}&amp;type={$type|escape:'url'}&amp;parentId={$parentId|escape:'url'}&amp;offset={$prev_offset|escape:'url'}&amp;sort_mode={$sort_mode|escape:'url'}">&lt;&lt;</a>&nbsp;
Need escape:'url' on $deep here, this happens at least on lines 198, 223, 229.

> <div class="oneresultfreetags">
>-<a href="{$objects[ix].href}" class="searchresultlink">{$objects[ix].name}</a>
>+<a href="{$objects[ix].href}" class="searchresultlink">{$objects[ix].name|escape}</a>
Looks the href should be escape:'url' here as well?

Comment 7

[Paul Craciunoiu [:paulc]]

Comment on attachment 438878 [details] [diff] [review]
SQL to fix dynamic content on desktop

SQL looks good, tested on both.

Comment 8

[James Socol [:jsocol, :james]]

Attachment: fixes missed variables

The {$objects[ix].href} are paths. URL-encoding will break them.

This fixes all the other spots I missed (I hope).

Comment 9

[Paul Craciunoiu [:paulc]]

Comment on attachment 438906 [details] [diff] [review]
fixes missed variables

Looks good. Didn't see anything this time. ZOMG!

Comment 10

[James Socol [:jsocol, :james]]

r65786 on the Fennec branch.

Filed bug 559239 to run the SQL patches.

Comment 11

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

Thanks. Marked for retest in sentinel.

Comment 12

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

Loading https://mobile-support-stage.mozilla.com/%E2%80%8Btiki-browse_categories.php?%E2%80%8BparentId=%3Cwhscheck%3E&%E2%80%8Bwatch_event=category_changed&%E2%80%8Bwatch_object=11&%E2%80%8Bdeep=off&%E2%80%8Bwatch_action=add gives me a 404; is that intended?

Ditto for https://mobile-support-stage.mozilla.com/%E2%80%8Btiki-browse_categories.php?%E2%80%8Blocale=en-US&%E2%80%8Bfind=&%E2%80%8Bdeep=off&%E2%80%8Btype=&%E2%80%8BparentId=%3Cwhscheck%3E&%E2%80%8Boffset=10&%E2%80%8Bsort_mode=name_asc.

Comment 13

[James Socol [:jsocol, :james]]

(In reply to comment #11)
> Thanks. Marked for retest in sentinel.

This will only update to stage automatically. Sentinel only tests the production sites. As this is an undisclosed vulnerability, can we way to push the fix to production until our planned release date (the 22nd)?

Comment 14

[James Socol [:jsocol, :james]]

Stephen: remove the "%E2%80%8B" strings from the URLs.

Comment 15

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

We'd really prefer the fix to be pushed live as soon as possible. Its great that we found these 3 issues before anyone outside did. Let's close off the gap and get this pushed live.

Also, I'd like to reopen this bug and only mark it resolved when the XSS is confirmed fixed in the production version. That way it doesn't slip off the radar.

Comment 16

[James Socol [:jsocol, :james]]

We close bugs when they're fixed on staging, and we don't push changes to production until QA marks a bug as VERIFIED--which they can't do if it's open. Once QA verifies all three bugs, I'll file an IT request to push the mobile changes immediately.

Comment 17

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

Sounds good.  Sorry for propagating my mistake across all the XSS bugs.  We'll keep our eye out and retest the issue after we see the issue has been pushed to prod.

Thanks!

Comment 18

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

Ok, I think this looks good on https://mobile-support-stage.mozilla.com/tiki-browse_categories.php?parentId=%3Cwhscheck%3E&watch_event=category_changed&watch_object=11&deep=off&watch_action=add.

Verified FIXED.

Comment 19

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

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