Last Comment Bug 109690 - [security] longlist.cgi doesn't check that $bug is valid
: [security] longlist.cgi doesn't check that $bug is valid
Status: VERIFIED FIXED
applied to 2.14.1
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 2.15
: x86 Linux
: P1 blocker (vote)
: Bugzilla 2.16
Assigned To: Bradley Baetz (:bbaetz)
: default-qa
:
Mentors:
http://bugzilla.mozilla.org/long_list...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-11-11 21:09 PST by Bradley Baetz (:bbaetz)
Modified: 2012-12-18 20:46 PST (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
obvious patch (582 bytes, patch)
2001-11-11 21:12 PST, Bradley Baetz (:bbaetz)
justdave: review+
gerv: review+
Details | Diff | Splinter Review
fix style nits (581 bytes, patch)
2001-11-11 21:41 PST, Bradley Baetz (:bbaetz)
justdave: review-
Details | Diff | Splinter Review
oops (617 bytes, patch)
2001-11-11 21:45 PST, Bradley Baetz (:bbaetz)
justdave: review+
gerv: review+
Details | Diff | Splinter Review
Combination of the previous two patches, applies cleanly to 2.14 (549 bytes, patch)
2001-11-17 00:37 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Splinter Review
patch that works on post-2.14, pre-tip versions (619 bytes, patch)
2001-12-14 13:14 PST, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Splinter Review

Description Bradley Baetz (:bbaetz) 2001-11-11 21:09:58 PST
See above url, again made more complicated to avoid stalling bmo. This one comes
very close to giving up the comments, too, but fails the attachments table
lookup with an invalid column id. You can get arround that by adding something
like 1<2, but then you get every single comment in the db. You don't want that :)

patch coming
Comment 1 Bradley Baetz (:bbaetz) 2001-11-11 21:12:29 PST
Created attachment 57448 [details] [diff] [review]
obvious patch
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-11 21:22:52 PST
Comment on attachment 57448 [details] [diff] [review]
obvious patch

r= justdave
Comment 3 Gervase Markham [:gerv] 2001-11-11 21:33:00 PST
Comment on attachment 57448 [details] [diff] [review]
obvious patch

r=gerv, under duress. I still think it should be 

detaint_natural($bug) || next;

Gerv
Comment 4 Bradley Baetz (:bbaetz) 2001-11-11 21:41:42 PST
Created attachment 57450 [details] [diff] [review]
fix style nits
Comment 5 Bradley Baetz (:bbaetz) 2001-11-11 21:43:29 PST
...and checked in
Comment 6 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-11 21:43:43 PST
Comment on attachment 57450 [details] [diff] [review]
fix style nits

uhhh...  That just doesn't look right.	(should be either "if" or "||", not
both, shouldn't it?
Comment 7 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-11 21:45:01 PST
back it out, it's broken
Comment 8 Bradley Baetz (:bbaetz) 2001-11-11 21:45:56 PST
Created attachment 57451 [details] [diff] [review]
oops
Comment 9 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-11 21:46:17 PST
Ladies and Gentlemen....

this is why ANY change to a patch (even if the reviewers already said they'd
approve it if you made that change) needs to be reviewed over again before it
gets checked in.

Please don't check in revised patches without posting to the bug first.
Comment 10 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-11 21:47:34 PST
Comment on attachment 57451 [details] [diff] [review]
oops

r= justdave
Comment 11 Gervase Markham [:gerv] 2001-11-11 21:50:40 PST
Comment on attachment 57451 [details] [diff] [review]
oops

r=gerv.

Gerv
Comment 12 Bradley Baetz (:bbaetz) 2001-11-11 21:52:21 PST
It was a one line patch, how could it go wrong?

Oops.
Comment 13 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-17 00:32:44 PST
Comment on attachment 57450 [details] [diff] [review]
fix style nits

removing the obsolete flag on this patch, because it still has to be applied
before the next patch here to have a complete patch.
Comment 14 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-17 00:37:04 PST
Created attachment 58206 [details] [diff] [review]
Combination of the previous two patches, applies cleanly to 2.14
Comment 15 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-17 00:39:50 PST
Attachment 58206 [details] [diff] checked into 2.14.1 branch:

/cvsroot/mozilla/webtools/bugzilla/long_list.cgi,v  <--  long_list.cgi
new revision: 1.15.2.1; previous revision: 1.15
Comment 16 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-12-10 17:27:11 PST
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
Comment 17 Myk Melez [:myk] [@mykmelez] 2001-12-14 13:14:51 PST
Created attachment 61779 [details] [diff] [review]
patch that works on post-2.14, pre-tip versions

Try this patch if attachment 58206 [details] [diff] [review] does not work for you.  I used it to upgrade
b.m.o. with the fix, since b.m.o. was running a version with nearby changes
that caused the other patch to be rejected.

Incidentally, why doesn't this code use ValidateBugID?	This situation is
exactly what that function was designed for, and it would provide a sensible
error message if someone entered an invalid bug ID, even if that ID was a
number.
Comment 18 Bradley Baetz (:bbaetz) 2001-12-14 13:24:11 PST
The current code ignored bugs which didn't exist; we wanted to keep teh same 
behaviour.
Comment 19 Myk Melez [:myk] [@mykmelez] 2001-12-14 14:19:01 PST
Are there common cases in which users pass a combination of valid and invalid
bug IDs to long_list.cgi and expect Bugzilla to selectively return information
about only the valid ones?  If so, that *might* be a reason to try to maintain
the old behavior.

Otherwise, there's no reason to maintain behavior that was an accident of our
failure to validate, and instead of failing silently we should do the normal
thing when a validation check fails, which is to display a message to the user
about it.

Comment 20 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-01-05 16:02:22 PST
Opening security bugs for which fixes have appeared in official bugzilla
release.  As per justdave and his posse.
Comment 21 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-01-08 14:46:02 PST
Looks like we're doing a 2.14.2 after all.

------------- begin forwarded message --------------

Date: Sun, 6 Jan 2002 12:34:01 +0100 (CET)
From: funkysh <funkysh@sm.pl>
To: <bugtraq@securityfocus.com>
Subject: Inproper input validation in Bugzilla <=2.14 - exploit



Since advisory and patched version is already released, here goes
description of vulnerabilities I discovered in Bugzilla almost year
ago.


1. Creating files on remote server.
-----------------------------------

Nothing spectacular, but this vulnerability may allow us easily
(at least when using Bugzilla with MySQL) to create files on
remote server in some cases, using MySQL's INTO OUTFILE.

long_list.cgi:

my $generic_query  = "
select
  bugs.bug_id,
  ...
  from bugs,profiles ...
  where assign.userid = ... and";

$::FORM{'buglist'} = "" unless exists $::FORM{'buglist'};
    foreach my $bug (split(/:/, $::FORM{'buglist'})) {
    SendSQL("$generic_query bugs.bug_id = $bug");

[..]

As we can see $::FORM{'buglist'} (submitted by user) isn't quoted here,
also script doesn't check if bug_id is numeric value. So we are able to
add extra SQL command into $generic_query.

ok, let's try..  after login we request:

http://site/bugzilla/long_list.cgi?buglist=1%20INTO%20OUTFILE%20%27/tmp/pussycat%27

We are lucky, if everything works, we'll see only little message:
"Full Text Bug Listing", so we then know file is created. If any problem
occur script will happily inform us.

[funkysh@note] $ ls -l /tmp/pussycat

-rw-rw-rw-    1 mysql    mysql         118 Jan 13 20:41 /tmp/pussycat

This may be serious problem if i.e. remote server running PHP,
and we have any writable dir inside DOCUMENT_ROOT reachable from
outside, we can create some evil php script. (Bugzilla by default creates
directory 'data' with permissions sets to 777 afair, it is also not a
problem to find out real path.)

Btw. this one seems to be still unpatched in 2.14.1.

[remainder of message about existing exploits we fixed omitted]
------------- end of forwarded message --------------


I can reproduce this on both the 2.14.1 release and on the tip.  This is NOT fixed.
Comment 22 Myk Melez [:myk] [@mykmelez] 2002-01-08 15:11:12 PST
I can only reproduce this on the tip if I comment out line 76:

    detaint_natural($bug) || next;

In both cases I get the same HTML response, but /tmp/pussycat only gets created
when that line is commented out, so this problem is, in fact, fixed.  2.14.1 has
the same validation code as the tip, so this problem should also be fixed on it,
but I have not tested that.
Comment 23 Myk Melez [:myk] [@mykmelez] 2002-01-08 15:17:31 PST
Tested on Bugzilla 2.14.1 installation.  With vanilla installation,
/tmp/pussycat does not get created when I attempt to exploit this bug.  If I
comment out line 76 of bug_list.cgi, however, that file does get created.  This
bug is fixed.
Comment 24 David D. Kilzer (ddk) 2002-01-08 15:21:10 PST
I can not reproduce this on my CVS-tip unless I comment out the 
one-line fix.  I have not tested this on a local 2.14.1 install,
though.
Comment 25 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-01-08 17:36:12 PST
Verified.  I tested on both 2.14.1 release version and the cvs tip on local
installs and I'm unable to reproduce without removing the patch from this bug. 
We could probably use a nicer message when there's no bugs to display, but
"that's another bug"

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