Closed Bug 130821 Opened 22 years ago Closed 22 years ago

buglist: "order" parameter sends unchecked SQL

Categories

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

Product:
Bugzilla
Component:
Query/Bug List
Version:
2.15
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: jruderman, Assigned: endico)

References

(
URL
)

Details

(Whiteboard: checked in for 2.14.2. security)

Attachments

(1 file, 1 obsolete file)

Try again.
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.
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
SQL?
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
problems.
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
So I repeat my question...  how serious was this?  Is it worthy of a security
announcement?
munging ccs
>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.
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
did.

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 http://landfill.bugzilla.org/bugzilla-2.14/

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 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-
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
TMTOWTDI. r=myk
Attachment #86578 - Flags: review+
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
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
with:

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.

Attachment

General

Creator:
Created:
Updated:
Size: