Closed
Bug 130821
Opened 22 years ago
Closed 22 years ago
buglist: "order" parameter sends unchecked SQL
Categories
(Bugzilla :: Query/Bug List, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: jruderman, Assigned: endico)
References
()
Details
(Whiteboard: checked in for 2.14.2. security)
Attachments
(1 file, 1 obsolete file)
1.63 KB,
patch
|
bbaetz
:
review+
myk
:
review+
|
Details | Diff | Splinter Review |
http://bugzilla.mozilla.org/buglist.cgi?field0-0-0=reporter&type0-0-0=substring&value0-0-0=jrud&order=bugs.bug_id%20AND%20ALLYOURBASEAREBELONGTOUS SQL Error: ... GROUP BY bugs.bug_id ORDER BY bugs.bug_id AND ALLYOURBASEAREBELONGTOUS: Unknown column 'ALLYOURBASEAREBELONGTOUS' in 'order clause' at globals.pl line 222. I noticed this at b.m.o and didn't test bugzilla-tip because landfill is down.
Comment 1•22 years ago
|
||
Doh. a . in the name doens't make it ok. Anyway, I think myk has fixed this with his patch which went in yesterday to templatise this file. myk: is that right? Not that you can do anything with this except affect sorting, but still....
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Comment 2•22 years ago
|
||
Why is this is in the security group and targetted at 2.16? We've known about this for ages and there are no known exploits.
Comment 3•22 years ago
|
||
This can't be fixed easily. For example it would break stored queries.
Comment 4•22 years ago
|
||
Jesse filed it in that group - sending untrusted html is a security hole. Its probably exploitable in mysql4, which supports union queries - just select the union of a valid query and all queries with groupset != 0. I'd have to confirm the syntax though- the mysql docs aren't really explicit. The postgresql syntax doesn't seem to allow it, btw - order has to be after the last subquery. As for fixing it breaking stuff - myk has fixed this in the buglist rewrite, so if it breaks stuff, then its broken in current CVS. What can it break? Is there stuff we used to allow sorting on which we currently don't? And if so, we can just manually strip those before testing.
Comment 5•22 years ago
|
||
I probably misunderstood. It still accepts raw column names, but not arbitrary SQL?
Comment 6•22 years ago
|
||
Well, it used to accept arbitary column names. Which could just happen to include spaces, or commas, or any character from a-z. So yues, it allows arbitary html. But its fixed now, I think
Comment 7•22 years ago
|
||
OK, as far as I can tell this does appear to be fixed already on the tip... it this worthy of yet another security announcement?
Comment 8•22 years ago
|
||
Well, FWIW, we knew about this for 2.14.1 and deigned it not even important enough to fix.
Comment 9•22 years ago
|
||
Well, its fixed now. So this can be closed or marked a dupe unelss there are problems.
Comment 10•22 years ago
|
||
This is fixed on the tip by the check-in for bug 103778, so resolving fixed. Note that the code does not allow the use of "AND" to separate sort columns (it requires the use of commas). Is "AND" supported at all in MySQL?
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 11•22 years ago
|
||
So I repeat my question... how serious was this? Is it worthy of a security announcement?
Comment 12•22 years ago
|
||
munging ccs
Comment 13•22 years ago
|
||
>So I repeat my question... how serious was this? Is it worthy of a security
>announcement?
I don't think it was that serious. I can't think of an exploit that would cause
data to be compromised. It may be possible to use this vulnerability to
construct a DOS attack, however. I suggest that 2.16 be released with a message
that the release fixes a number of security bugs of varying severity and that
all users are advised to upgrade. Alternately, we could work up a patch for
just this problem (and others that exist in 2.14) and apply it to the 2.14
branch for a new security release, but I think that's more work than it's worth.
Comment 14•22 years ago
|
||
moving secure bugzilla/webtools bugs from mozilla security group to the new bugzilla security group.
Group: security? → webtools-security?
Comment 15•22 years ago
|
||
I backported Myk's code to deal with this. This is mostly the same - the major difference is that it checks field names have valid syntax eg identifier.identifier, rather than checking the field actually exists as Myk did. I think this is OK to solve what I am worried about anyway, UNIONs in MySQL 4.
Comment 16•22 years ago
|
||
Comment on attachment 86553 [details] [diff] [review] Backported patch for 2.14.2. this patch is applied on http://landfill.bugzilla.org/bugzilla-2.14/ Works for me.
Attachment #86553 -
Flags: review+
Updated•22 years ago
|
Whiteboard: wanted for 2.14.2
Comment 17•22 years ago
|
||
Comment on attachment 86553 [details] [diff] [review] Backported patch for 2.14.2. >Index: buglist.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v >retrieving revision 1.139.2.2 >diff -u -u -r1.139.2.2 buglist.cgi >--- buglist.cgi 30 Dec 2001 05:41:44 -0000 1.139.2.2 >+++ buglist.cgi 6 Jun 2002 03:28:05 -0000 >@@ -1051,10 +1051,11 @@ > my $query = GenerateSQL(\@fields, undef, undef, $::buffer); > > >- >+my $order_from_cookie = 0; > if ($::COOKIE{'LASTORDER'}) { > if ((!$::FORM{'order'}) || $::FORM{'order'} =~ /^reuse/i) { > $::FORM{'order'} = url_decode($::COOKIE{'LASTORDER'}); >+ $order_from_cookie = 1; > } > } > >@@ -1068,7 +1069,26 @@ > > ORDER: for ($::FORM{'order'}) { > /\./ && do { >- # This (hopefully) already has fieldnames in it, so we're done. >+ # A custom list of columns. Make sure each column is valid. >+ foreach my $fragment (split(/[,\s]+/, $::FORM{'order'})) { >+ next if $fragment =~ /^asc|desc$/i; this needs to be /^(?:asc|desc)$/. The current regexp actually means /(^asc)|(desc$)/. Yes, this means trunk is broken too - try an order of 'ASCASDASA' Also, I'm not conviced that the split is correct, either. We should split on comma, then trim the result. I think we should do this on trunk/2.16, unless someone thinks that its not important. We can construct invalid sorts, but thas a db issue, possibly. It still would be nice to give a sensible error in such cases. >+ >+ my $ident_regexp = "[A-Za-z_][0-9A-Za-z_]*"; >+ if ($fragment !~ /${ident_regexp}\.${ident_regexp}/) { Given the above, this should then be /^${ident_regexp}\.${ident_regexp}(\s+(asc|desc))?$/i. Since we have to use /i then, you can simplify ident_regexp. Note the added ^ and $ - even if you dno't want to slpit that way to handle asc|desc, you need that. >+ my $qfragment = html_quote($fragment); >+ my $error = "The custom sort order you specified in your " >+ . "form submission contains an invalid column " >+ . "name <em>$qfragment</em>."; >+ if ($order_from_cookie) { >+ my $cookiepath = Param("cookiepath"); >+ print "Set-Cookie: LASTORDER= ; path=$cookiepath; expires=Sun, 30-Jun-80 00:00:00 GMT\n"; >+ $error =~ s/form submission/cookie/; >+ $error .= " The cookie has been cleared."; >+ } >+ DisplayError($error); >+ exit; >+ } >+ } > last ORDER; > }; > /Number/ && do {
Attachment #86553 -
Flags: review-
Comment 18•22 years ago
|
||
This patch takes the above comments into account. It also fixes problems I discovered - the old patch referred to the cookiepath parameter which isn't in 2.14. Also the cookie removal code didn't work, so I removed it.
Attachment #86553 -
Attachment is obsolete: true
Comment 19•22 years ago
|
||
Comment on attachment 86578 [details] [diff] [review] Try again. If we have multipart on, then I get the error under the "please wait" thing. I think that this is ok, since no 'normal' user should ever see this error, and we shouldn't have to rearrange the code for 2.14. It works correctly for 2.16, with the templatisation, anyway.
Attachment #86578 -
Flags: review+
Comment 20•22 years ago
|
||
Yes, that's intentional, there are already loads of those in the 2.14 codebase.
Comment 21•22 years ago
|
||
If it's arbitrary HTML we're worried about, wouldn't it be better to HTML-escape the SQL error message?
Comment 22•22 years ago
|
||
What SQL error message? The fragment is html quoted as per your original code, I don't see what else there is.
Comment 23•22 years ago
|
||
Comment on attachment 86578 [details] [diff] [review] Try again. Also, shouldn't I see a Bugzilla error message when I enter a nonsensical value? Oh, I suppose that can't happen unless we trap SQL errors, which is beyond the scope of this bug. I guess this fix solves the problem; I'm not sure it's the best solution, but TMTOWTDI. r=myk
Attachment #86578 -
Flags: review+
Comment 24•22 years ago
|
||
There are already checks in place to check for invalid characters in the SQL order. Checked into 2.14 branch: Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.139.2.3; previous revision: 1.139.2.2 done
Whiteboard: wanted for 2.14.2 → checked in for 2.14.2
Comment 25•22 years ago
|
||
Yeah, we don't capture invalid names for 2.14, but we do in 2.16/trunk.
Comment 27•22 years ago
|
||
I know this is a resolved and fixed bug, but since this is the fix that caused my problem, I feel inclined to report it here. I just patched our 2.14.1 installation up to 2.14.2. We immediately noticed that we could no longer sort our buglists after the change. Sure enough, if I back-out the patch sorting works again. Somewhere along the line the ORDER gets corrupted. As I'm not intimately familiar with Bugzilla code, I only made a cursory attempt to fix the problem, but came up empty. What we have found is that if you are only sorting on one index (for example order=bugs.priority), you're fine, but if you have two or more (for example order=bugs.bug_severity%2C%20bugs.priority), then the sort does not occur for either index. As soon as I backed out this particular change (not the whole 2.14.2 patch) sorting was returned to us. Since this bug report is the verbatim patch that I found, I decided to report it here.
Comment 28•22 years ago
|
||
As wierd as that is, I'm going to have to confirm that. Removing the trim from the $fragment just before the regexp fixes this (although the regexp will need changing so that queries still work) debug printing shows that $fragment is ok each time, but $::FORM{'ORDER'} chnages. I have no clue what could be causing this - split returns copies, not references, so $fragment shouldn't have any related ot $::FORM{'ORDER'} Any ideas, anyone? Maybe we should file a new bug for this.
Comment 29•22 years ago
|
||
To be more precise, the last 'thing' listed to be sorted is used, but thats it, according to sqllog.
Comment 30•22 years ago
|
||
As pointed out on npm.webtools by "Randall M! Gee", the problem here is that 2.14's trim modifies $_ directly, whilst CVS HEAD uses a 'real' variable. I'm not sure what to do about this; the (untested) fix is to replace that line with: if ($fragment !~ /^\s*${ident_iregexp}\.${ident_iregexp}(\s+(asc|desc))?\s*$/i) { In any event, I'll open a new bug.
Comment 31•22 years ago
|
||
The new bug is bug 152138.
Reporter | ||
Updated•21 years ago
|
Whiteboard: checked in for 2.14.2 → checked in for 2.14.2. security
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•