505652 - Over-escaping forum-thread titles in the "Support Website Forums" page

Status: VERIFIED FIXED

(support.mozilla.org :: General, defect)

Description

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

(Not putting this into Forums, because I don't think this code is forum code, as that works properly in step 3.)

STR:

1. Load http://support.mozilla.com/en-US/kb/Support+Website+Forums
2. Look under "Most Popular Topics", at "I have the dreaded "Icanseek" a..."
3. Compare that to http://support.mozilla.com/tiki-view_forum_thread.php?locale=en-US&forumId=1&comments_parentId=386372

Expected Results:

"I have the dreaded "Icanseek" a..."

Actual Results:

See above

Comment 1

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

Attachment: Screenshot

Bug is in the 5th forum title down from the left.

Comment 2

[Paul Craciunoiu [:paulc]]

"Support Website Forums" makes me sad.

Comment 3

[Paul Craciunoiu [:paulc]]

So I see this in the screenshot, but I created an article with the same title on my local copy, and it shows up alright. Stephen, could you maybe try to reproduce this on stage? That forum topic doesn't show on the page anymore on prod.

Comment 4

[[:Cww]]

happens on stage too...

Comment 5

[Laura Thomson :laura]

Reopen if we can reproduce.

Comment 6

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

OK, just reproduced.

https://support-stage.mozilla.org/en-US/kb/Support+Website+Forums

Comment 7

[[:Cww]]

Paul and I did investigation and so here's the conclusion:

We're escaping in the FORM that posts to the support forum (But not the forum that edits posts or posts to the contributor forum) so titles in the DATABASE are escaped (ugh).  We're then (likely) passing the raw title on tiki-view_forum and tiki-view_forum_thread without further escaping BUT the module that handles the forum front page IS doing a second escape.

The ideal situation is we DO NOT escape when entering new forum threads so the data in the table is the exact string entered and then we DO escape everywhere we display that text.  This would require making sure that we check everywhere we might be showing forum titles to make sure escaping is done.

Comment 8

[Paul Craciunoiu [:paulc]]

Attachment: patch, v1

So as Cheng, Stephen and I discussed, there was some over-escaping going on -- in forum_advanced_post.php

It turns out that the escaping was done in php, which is risky and opens up vulnerabilities. Escaping should be done one display. This patch fixes that:
* comments out the php escaping in forum_advanced_post.php
* adds escaping on editing and viewing the forum topic titles in tiki-view_forum.php and tiki-view_forum_thread.php

I've looked carefully over these files so I hope I haven't missed anything. This will need some good QA to make sure we don't open up other vulnerabilities. However, strings were NOT previously properly escaped, as you can see here:
https://support-stage.mozilla.org/tiki-view_forum_thread.php?comments_parentId=374591&forumId=1
Notice <script> gets turned into <sc<x>ript> but <a> doesn't get escaped. This breaks, for example, here:
https://support-stage.mozilla.org/tiki-view_forum.php?locale=en-US&forumId=1 (see that the topic posted by Test contains 2 links in the title), potentially allowing spammers to replace the title link with their own -- see https://support-stage.mozilla.org/tiki-view_forum_thread.php?locale=en-US&comments_parentId=374592&forumId=1 whose title is broken professionally :)

Comment 9

[Reed Loden [:reed]]

Make sure you do not regress bug 500146.

Comment 10

[Paul Craciunoiu [:paulc]]

Thanks Reed. It doesn't seem to regress bug 500146 on URL:
http://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
(of course I tested locally)

At any rate, we should make sure to test this on stage too (hint hint QA).

Comment 11

[Paul Craciunoiu [:paulc]]

r48845 / r48846

Comment 12

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

Attachment: Post-fix screenshot, forums

Comment 13

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

Attachment: Post-fix screenshot, email notification

Comment 14

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

Verified FIXED, w00t! :-)