[security] We allow the user to send arbitary SQL!!!!!

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Query/Bug List
P1
blocker
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: bbaetz, Assigned: Jacob Steenhagen)

Tracking

2.15
Bugzilla 2.16

Details

(Whiteboard: applied to 2.14.1, URL)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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.
s/warnings/Perl comments/
Blocks: 103885
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
(Assignee)

Comment 3

16 years ago
Created attachment 56877 [details] [diff] [review]
patch
(Assignee)

Comment 4

16 years ago
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
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: patch, review
(Reporter)

Comment 5

16 years ago
Comment on attachment 56877 [details] [diff] [review]
patch

r=bbaetz for either this patch or one which just deletes it.
Attachment #56877 - Flags: review+
Comment on attachment 56877 [details] [diff] [review]
patch

It works. r=myk
Attachment #56877 - Flags: review+
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
Last Resolved: 16 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: 1.139.2.1; 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.
Group: security?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.