Closed Bug 130821 Opened 23 years ago Closed 23 years ago

buglist: "order" parameter sends unchecked SQL


(Bugzilla :: Query/Bug List, defect, P1)

Windows 98



Bugzilla 2.16


(Reporter: jruderman, Assigned: endico)




(Whiteboard: checked in for 2.14.2. security)


(1 file, 1 obsolete file)

SQL Error:
column 'ALLYOURBASEAREBELONGTOUS' in 'order clause' at line 222.

I noticed this at b.m.o and didn't test bugzilla-tip because landfill is down.
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
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.
This can't be fixed easily.  For example it would break stored queries.
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.
I probably misunderstood.  It still accepts raw column names, but not arbitrary
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
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?
Well, FWIW, we knew about this for 2.14.1 and deigned it not even important
enough to fix.
Well, its fixed now. So this can be closed or marked a dupe unelss there are
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?
Closed: 23 years ago
Resolution: --- → FIXED
So I repeat my question...  how serious was this?  Is it worthy of a security
munging ccs
>So I repeat my question...  how serious was this?  Is it worthy of a security

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.
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Group: security? → webtools-security?
Attached patch Backported patch for 2.14.2. (obsolete) — Splinter Review
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

I think this is OK to solve what I am worried about anyway, UNIONs in MySQL 4.
Comment on attachment 86553 [details] [diff] [review]
Backported patch for 2.14.2.

this patch is applied on

Works for me.
Attachment #86553 - Flags: review+
Whiteboard: wanted for 2.14.2
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
>diff -u -u -r1.139.2.2 buglist.cgi
>--- buglist.cgi	30 Dec 2001 05:41:44 -0000
>+++ 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-
Attached patch Try again.Splinter Review
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 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+
Yes, that's intentional, there are already loads of those in the 2.14 codebase.
If it's arbitrary HTML we're worried about, wouldn't it be better to HTML-escape
the SQL error message?
What SQL error message?  The fragment is html quoted as per your original code,
I don't see what else there is.
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
Attachment #86578 - Flags: review+
There are already checks in place to check for invalid characters in the SQL

Checked into 2.14 branch:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision:; previous revision:
Whiteboard: wanted for 2.14.2 → checked in for 2.14.2
Yeah, we don't capture invalid names for 2.14, but we do in 2.16/trunk.
2.14.2 is out, removing security group.
Group: webtools-security?
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.
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.
To be more precise, the last 'thing' listed to be sorted is used, but thats it,
according to sqllog.
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

if ($fragment !~ /^\s*${ident_iregexp}\.${ident_iregexp}(\s+(asc|desc))?\s*$/i)

In any event, I'll open a new bug.
The new bug is bug 152138.
Whiteboard: checked in for 2.14.2 → checked in for 2.14.2. 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.