Closed Bug 460129 Opened 16 years ago Closed 16 years ago

XSS on search page

Categories

(addons.mozilla.org Graveyard :: Public Pages, defect)

defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: clouserw, Assigned: rdoherty)

Details

(Keywords: wsec-xss)

Attachments

(3 files, 3 obsolete files)

This just came through: https://addons.mozilla.org/en-US/firefox/search?q=%C0%22%20onmouseover=alert(/xss/.source)%20\&cat=all It's actually publicly disclosed at this point but I'm marking this confidential right now because I think the coverage is fairly narrow right now anyway.
Wil, want to add the URL where this vulnerability was announced? Also, if the related SQL error opens the door for SQL injection, we should push this fix off-cycle.
Assignee: nobody → rdoherty
Status: NEW → ASSIGNED
(In reply to comment #1) > Wil, want to add the URL where this vulnerability was announced? > http://sla.ckers.org/forum/read.php?3,44,24950#msg-24950
It's an encoding exploit. Running the search terms through iconv("UTF-8", "UTF-8", $_terms) fixes it. Not quite sure yet if it's a sql injection bug also....
Thanks Ryan, the query seems to run fine for me - not sure why you guys were getting errors there. > Also, if the related SQL error opens the door for SQL injection, we should push > this fix off-cycle. Regardless if it's SQL or XSS I think it should be pushed asap. They're both dangerous; XSS probably more so because it's easier to exploit.
Attached patch patch for html exploit (obsolete) — Splinter Review
Here's the patch for the html exploit
Attachment #343300 - Flags: review?(clouserw)
Attachment #343300 - Flags: review?(clouserw) → review-
Attachment #343300 - Attachment is obsolete: true
Attachment #343303 - Flags: review?(clouserw)
Comment on attachment 343303 [details] [diff] [review] patch in _sanitizeArray instead with //IGNORE please put {} around the if. Otherwise it works great (at least for this page). Have you run any tests since this is such a central change?
Attachment #343303 - Flags: review?(clouserw) → review-
Attachment #343303 - Attachment is obsolete: true
Attachment #343312 - Flags: review?(clouserw)
Attachment #343312 - Flags: review?(clouserw) → review+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Thanks Ryan. I think pushing this tomorrow with 4.0.2 is fine. Feel free to disagree. :)
Keywords: push-needed
I'm really worried about leaving a known xss exploit open for any amount of time. I think we should push asap. Do we have any policies in place for security exploits?
I don't think we have official policies. I'd support pushing just this patch early. We could even do a local patch and have svn overwrite it when we up on thursday. morgamic?
I'm still awaiting preview to cycle, so that I can verify on https://preview.addons.mozilla.org/en-US/firefox/search?q=%C0%22%20onmouseover=alert%28/xss/.source%29%20\&cat=all, but obviously this has already been tested by you guys...
(In reply to comment #14) > I'm still awaiting preview to cycle, so that I can verify on > https://preview.addons.mozilla.org/en-US/firefox/search?q=%C0%22%20onmouseover=alert%28/xss/.source%29%20\&cat=all, > but obviously this has already been tested by you guys... It's cycled.
My main concern would be not so much fixing this as negatively affecting other areas (which I haven't found yet, but who knows)
In reply to comment 15; yeah, it literally did just after I posted that comment (figures!) Preview now shows " onmouseover=alert(/xss/.source) \ in the search textfield. I'll run my search test suite again, but that's not really negative testing; any ideas on what to look for, fallout-wise?
(In reply to comment #16) > My main concern would be not so much fixing this as negatively affecting other > areas (which I haven't found yet, but who knows) So if QA can do some sanity checks would that be enough? I agree with your point that we don't know with 100% certainty we haven't introduced other bugs.
(In reply to comment #17) > I'll run my search test suite again, but that's not really negative testing; > any ideas on what to look for, fallout-wise? My patch affects any user-entered data. So we should be looking for corrupted or missing information in comments, info, etc.
(In reply to comment #18) > (In reply to comment #16) > > My main concern would be not so much fixing this as negatively affecting other > > areas (which I haven't found yet, but who knows) > > So if QA can do some sanity checks would that be enough? I agree with your > point that we don't know with 100% certainty we haven't introduced other bugs. Please still add a test case for this exploit :)
(In reply to comment #20) > Please still add a test case for this exploit :) Working on it :)
I'd be fine with pushing the security fix tonight on just this file unless it conflicts/includes other changes.
(In reply to comment #22) > I'd be fine with pushing the security fix tonight on just this file unless it > conflicts/includes other changes. Does that mean you're talking to IT or you want me to?
I want you to since you're more familiar with the fix/patch.
Attached patch combo patchSplinter Review
I'm adding another patch that combines my "dont sanitize icondata" fix with rdoherty's patch so we can push this tonight.
bug 460173 filed to push this live
Group: client-services-security
Keywords: push-needed
Verified FIXED on preview; Wil, Ryan, and I all tested various character inputs, and additionally I ran my Selenium search suite and we didn't regress search.
Status: RESOLVED → VERIFIED
Thanks everyone for working on this.
(In reply to comment #28) > Thanks everyone for working on this. Yes, how seriously we are taking security problems makes me all warm and fuzzy on the inside -- or so :) Good job, guys!
Awesome :) Thanks Wil for pushing it out. Couple of things I think we should take from this: * Need to test all our sites for this type of attack * Perhaps we should create some sort of XSS test suite? (http://ha.ckers.org/xss.html) * Also need a security policy that dictates how fast we should respond to this. Does the Firefox team have a security policy for exploits we could look at for guidance?
(In reply to comment #30) > * Also need a security policy that dictates how fast we should respond to this. Our policy regarding web security issues is to fix them as quickly and responsibly as possible. There are some efforts underway regarding security testing, would be happy to discuss.
Attached patch Test case for exploit (obsolete) — Splinter Review
Here's a test case for the exploit.
Attachment #343408 - Flags: review?(fwenzel)
Attachment #343408 - Flags: review?(clouserw)
Comment on attachment 343408 [details] [diff] [review] Test case for exploit Ah, I thought you maybe just wanted to add a testcase for "does our sanitization function still work when we feed it invalid UTF-8?" but this one works as well.
Attachment #343408 - Flags: review?(fwenzel) → review+
(In reply to comment #30) > * Need to test all our sites for this type of attack > * Perhaps we should create some sort of XSS test suite? > (http://ha.ckers.org/xss.html) We are in the process of rolling out WhiteHat Sentinel on three of our high-visibility sites (AMO, SUMO, Bugzilla) which should give us this type of systematic testing for web app vulnerabilities. The Sentinel scans have already found a few issues for which I will be opening bugs over the next few days.
(In reply to comment #34) > We are in the process of rolling out WhiteHat Sentinel on three of our > high-visibility sites (AMO, SUMO, Bugzilla) which should give us this type of > systematic testing for web app vulnerabilities. The Sentinel scans have > already found a few issues for which I will be opening bugs over the next few > days. Awesome :)
Moved test case to SearchResultTest. I don't think we need to test if the search works with invalid data. If someone is entering invalid data there's no guarantee it will work. And it would be difficult to guess all the different types of invalid data to test for.
Attachment #343408 - Attachment is obsolete: true
Attachment #343427 - Flags: review?(fwenzel)
Attachment #343427 - Flags: review?(clouserw)
Attachment #343408 - Flags: review?(clouserw)
Comment on attachment 343427 [details] [diff] [review] Moved testcase to SearchResultTest class Exactly my point, no need to test search specifically when we can test site-wide data sanitization. But your test case is good for this bug.
Attachment #343427 - Flags: review?(fwenzel) → review+
Attachment #343427 - Flags: review?(clouserw) → review+
Comment on attachment 343427 [details] [diff] [review] Moved testcase to SearchResultTest class thanks
Adding keywords to bugs for metrics, no action required. Sorry about bugmail spam.
Keywords: wsec-xss
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: