Closed
Bug 460129
Opened 16 years ago
Closed 16 years ago
XSS on search page
Categories
(addons.mozilla.org Graveyard :: Public Pages, defect)
addons.mozilla.org Graveyard
Public Pages
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: clouserw, Assigned: rdoherty)
Details
(Keywords: wsec-xss)
Attachments
(3 files, 3 obsolete files)
628 bytes,
patch
|
clouserw
:
review+
|
Details | Diff | Splinter Review |
975 bytes,
patch
|
Details | Diff | Splinter Review | |
1.04 KB,
patch
|
wenzel
:
review+
clouserw
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•16 years ago
|
||
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
Reporter | ||
Comment 2•16 years ago
|
||
(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
Assignee | ||
Comment 3•16 years ago
|
||
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....
Assignee | ||
Comment 4•16 years ago
|
||
Reporter | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
Here's the patch for the html exploit
Attachment #343300 -
Flags: review?(clouserw)
Reporter | ||
Updated•16 years ago
|
Attachment #343300 -
Flags: review?(clouserw) → review-
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #343300 -
Attachment is obsolete: true
Attachment #343303 -
Flags: review?(clouserw)
Reporter | ||
Comment 8•16 years ago
|
||
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-
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #343303 -
Attachment is obsolete: true
Attachment #343312 -
Flags: review?(clouserw)
Reporter | ||
Updated•16 years ago
|
Attachment #343312 -
Flags: review?(clouserw) → review+
Assignee | ||
Comment 10•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 11•16 years ago
|
||
Thanks Ryan. I think pushing this tomorrow with 4.0.2 is fine. Feel free to disagree. :)
Keywords: push-needed
Assignee | ||
Comment 12•16 years ago
|
||
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?
Reporter | ||
Comment 13•16 years ago
|
||
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...
Assignee | ||
Comment 15•16 years ago
|
||
(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.
Reporter | ||
Comment 16•16 years ago
|
||
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?
Assignee | ||
Comment 18•16 years ago
|
||
(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.
Assignee | ||
Comment 19•16 years ago
|
||
(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.
Comment 20•16 years ago
|
||
(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 :)
Assignee | ||
Comment 21•16 years ago
|
||
(In reply to comment #20)
> Please still add a test case for this exploit :)
Working on it :)
Comment 22•16 years ago
|
||
I'd be fine with pushing the security fix tonight on just this file unless it conflicts/includes other changes.
Reporter | ||
Comment 23•16 years ago
|
||
(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?
Comment 24•16 years ago
|
||
I want you to since you're more familiar with the fix/patch.
Reporter | ||
Comment 25•16 years ago
|
||
I'm adding another patch that combines my "dont sanitize icondata" fix with rdoherty's patch so we can push this tonight.
Reporter | ||
Comment 26•16 years ago
|
||
bug 460173 filed to push this live
Reporter | ||
Updated•16 years ago
|
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
Comment 28•16 years ago
|
||
Thanks everyone for working on this.
Comment 29•16 years ago
|
||
(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!
Assignee | ||
Comment 30•16 years ago
|
||
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?
Comment 31•16 years ago
|
||
(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.
Assignee | ||
Comment 32•16 years ago
|
||
Here's a test case for the exploit.
Attachment #343408 -
Flags: review?(fwenzel)
Attachment #343408 -
Flags: review?(clouserw)
Comment 33•16 years ago
|
||
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+
Comment 34•16 years ago
|
||
(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.
Assignee | ||
Comment 35•16 years ago
|
||
(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 :)
Assignee | ||
Comment 36•16 years ago
|
||
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 37•16 years ago
|
||
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+
Reporter | ||
Updated•16 years ago
|
Attachment #343427 -
Flags: review?(clouserw) → review+
Reporter | ||
Comment 38•16 years ago
|
||
Comment on attachment 343427 [details] [diff] [review]
Moved testcase to SearchResultTest class
thanks
Assignee | ||
Comment 39•16 years ago
|
||
Comment 40•12 years ago
|
||
Adding keywords to bugs for metrics, no action required. Sorry about bugmail spam.
Keywords: wsec-xss
Updated•9 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•