Last Comment Bug 130821 - buglist: "order" parameter sends unchecked SQL
: buglist: "order" parameter sends unchecked SQL
checked in for 2.14.2. security
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 2.15
: x86 Windows 98
P1 major (vote)
: Bugzilla 2.16
Assigned To: Dawn Endico
: default-qa
Depends on:
  Show dependency treegraph
Reported: 2002-03-14 00:51 PST by Jesse Ruderman
Modified: 2012-12-18 20:46 PST (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

Backported patch for 2.14.2. (1.91 KB, patch)
2002-06-05 20:43 PDT, Matthew Tuck [:CodeMachine]
justdave: review+
bbaetz: review-
Details | Diff | Splinter Review
Try again. (1.63 KB, patch)
2002-06-06 03:24 PDT, Matthew Tuck [:CodeMachine]
bbaetz: review+
myk: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2002-03-14 00:51:58 PST

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.
Comment 1 User image Bradley Baetz (:bbaetz) 2002-03-14 01:55:46 PST
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....
Comment 2 User image Matthew Tuck [:CodeMachine] 2002-03-14 19:16:01 PST
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 User image Matthew Tuck [:CodeMachine] 2002-03-14 19:17:08 PST
This can't be fixed easily.  For example it would break stored queries.
Comment 4 User image Bradley Baetz (:bbaetz) 2002-03-14 19:26:44 PST
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 User image Matthew Tuck [:CodeMachine] 2002-03-14 20:05:34 PST
I probably misunderstood.  It still accepts raw column names, but not arbitrary
Comment 6 User image Bradley Baetz (:bbaetz) 2002-03-14 21:33:49 PST
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 User image Dave Miller [:justdave] ( 2002-03-14 22:19:33 PST
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 User image Matthew Tuck [:CodeMachine] 2002-03-15 20:43:25 PST
Well, FWIW, we knew about this for 2.14.1 and deigned it not even important
enough to fix.
Comment 9 User image Bradley Baetz (:bbaetz) 2002-03-15 21:31:02 PST
Well, its fixed now. So this can be closed or marked a dupe unelss there are
Comment 10 User image Myk Melez [:myk] [@mykmelez] 2002-03-18 12:52:15 PST
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?
Comment 11 User image Dave Miller [:justdave] ( 2002-03-18 21:54:08 PST
So I repeat my question...  how serious was this?  Is it worthy of a security
Comment 12 User image Dave Miller [:justdave] ( 2002-05-08 21:53:10 PDT
munging ccs
Comment 13 User image Myk Melez [:myk] [@mykmelez] 2002-05-09 12:31:41 PDT
>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.
Comment 14 User image Bradley Baetz (:bbaetz) 2002-05-15 22:30:19 PDT
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Comment 15 User image Matthew Tuck [:CodeMachine] 2002-06-05 20:43:12 PDT
Created attachment 86553 [details] [diff] [review]
Backported patch for 2.14.2.

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 16 User image Dave Miller [:justdave] ( 2002-06-05 21:14:09 PDT
Comment on attachment 86553 [details] [diff] [review]
Backported patch for 2.14.2.

this patch is applied on

Works for me.
Comment 17 User image Bradley Baetz (:bbaetz) 2002-06-05 23:48:01 PDT
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 {
Comment 18 User image Matthew Tuck [:CodeMachine] 2002-06-06 03:24:07 PDT
Created attachment 86578 [details] [diff] [review]
Try again.

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.
Comment 19 User image Bradley Baetz (:bbaetz) 2002-06-06 22:53:03 PDT
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.
Comment 20 User image Matthew Tuck [:CodeMachine] 2002-06-06 23:42:42 PDT
Yes, that's intentional, there are already loads of those in the 2.14 codebase.
Comment 21 User image Myk Melez [:myk] [@mykmelez] 2002-06-07 16:29:47 PDT
If it's arbitrary HTML we're worried about, wouldn't it be better to HTML-escape
the SQL error message?
Comment 22 User image Matthew Tuck [:CodeMachine] 2002-06-07 16:34:34 PDT
What SQL error message?  The fragment is html quoted as per your original code,
I don't see what else there is.
Comment 23 User image Myk Melez [:myk] [@mykmelez] 2002-06-07 16:34:48 PDT
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
Comment 24 User image Matthew Tuck [:CodeMachine] 2002-06-07 16:40:44 PDT
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:
Comment 25 User image Bradley Baetz (:bbaetz) 2002-06-07 19:31:26 PDT
Yeah, we don't capture invalid names for 2.14, but we do in 2.16/trunk.
Comment 26 User image Dave Miller [:justdave] ( 2002-06-08 00:01:43 PDT
2.14.2 is out, removing security group.
Comment 27 User image Brandon Perkins 2002-06-13 08:55:31 PDT
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 User image Bradley Baetz (:bbaetz) 2002-06-13 09:16:30 PDT
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 User image Bradley Baetz (:bbaetz) 2002-06-13 09:17:51 PDT
To be more precise, the last 'thing' listed to be sorted is used, but thats it,
according to sqllog.
Comment 30 User image Bradley Baetz (:bbaetz) 2002-06-16 04:29:23 PDT
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.
Comment 31 User image Bradley Baetz (:bbaetz) 2002-06-16 04:34:32 PDT
The new bug is bug 152138.

Note You need to log in before you can comment on or make changes to this bug.