Closed Bug 108812 Opened 22 years ago Closed 22 years ago
[security] We allow the user to send arbitary SQL!!!!!
So, buglist.cgi has an sql paramater. And, as long as that doen't contain a semicolon, we send it. Oops. The above url will allow you to see the title of a test security group bug. Its more complicated than it looks - bugzilla.mozilla.org/buglist.cgi?sql=1%3E2)%20OR%20(bugs.bug_id=106544 (%3E is >, and %20 is space) would work as well, except that then we join the users table cubed then, without conditions, and that takes a long long long long long time on bmo. This was presumably added for advanced query support, or something. Lets kill it. Its a huge hole, and even if we someone patch it up it sitll makes me nervous. This turned up another bug, which I discovered over the weekend - my SelectVisible stuff does "WHERE (security conditions) AND other stuff" rather than "WHERE (security conditions) AND (other stuff)". That wouldn't have helped in this case, though, because the extra bracketing could have been easily removed. Fixing that may be interesting, since the where clause isn't the end - we have to search for HAVING/GROUP BY in the rest of the where, while still letting people search for bugs containing the word having. I have no idea how to do that. I was vaguely aware of this when I wrote the support, but all places which use that code use AND. If someone can tell me how to fix it, I will.
OK, options here appear to be one or more of: 1. Commenting it out and giving appropriate end-of-the-world warnings. I'm hesitant to remove it as someone might have a use for it and have a closed installation. 2. Properly parsing the SQL condition to ensure it is valdi. A bracket matching algorithm would work out if the bracket count went below 1, however GROUP BY does sound like it's another exploit. 3. Restricting usage of this parameter to some appropriate trusted group.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
My personal preference would be to just yank the code altogether. However, I instead followed Matthew's reccomendation and commented it out w/a prepended warning (perl comment).
Assignee: endico → jake
Comment on attachment 56877 [details] [diff] [review] patch r=bbaetz for either this patch or one which just deletes it.
Comment on attachment 56877 [details] [diff] [review] patch It works. r=myk
Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.153; previous revision: 1.152 done
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
This applied to the 2.14.1 branch with no changes. /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 126.96.36.199; previous revision: 1.139
Whiteboard: applied to 2.14.1
Hmm, it seems the bulk change thinks I'm not changing anything if all I do is add names to the CC list, so I guess I have to make a comment. Anyhow, adding the representatives from the organizations we know of that support Bugzilla distributions so they're aware of our upcoming security release
Opening security bugs for which fixes have appeared in official bugzilla release. As per justdave and his posse.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.