109690 - [security] longlist.cgi doesn't check that $bug is valid

Status: VERIFIED FIXED

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

Description

[Bradley Baetz (:bbaetz)]

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)]

Attachment: obvious patch

Comment 2

[Dave Miller [:justdave]]

Comment on attachment 57448 [details] [diff] [review]
obvious patch

r= justdave

Comment 3

[Gervase Markham [:gerv]]

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)]

Attachment: fix style nits

Comment 5

[Bradley Baetz (:bbaetz)]

...and checked in

Comment 6

[Dave Miller [:justdave]]

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]]

back it out, it's broken

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: oops

Comment 9

[Dave Miller [:justdave]]

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]]

Comment on attachment 57451 [details] [diff] [review]
oops

r= justdave

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 57451 [details] [diff] [review]
oops

r=gerv.

Gerv

Comment 12

[Bradley Baetz (:bbaetz)]

It was a one line patch, how could it go wrong?

Oops.

Comment 13

[Dave Miller [:justdave]]

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]]

Attachment: Combination of the previous two patches, applies cleanly to 2.14

Comment 15

[Dave Miller [:justdave]]

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]]

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]]

Attachment: 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)]

The current code ignored bugs which didn't exist; we wanted to keep teh same 
behaviour.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

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)]

Opening security bugs for which fixes have appeared in official bugzilla
release.  As per justdave and his posse.

Comment 21

[Dave Miller [:justdave]]

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 <[e-mail]>
To: <[e-mail]>
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]]

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]]

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)]

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]]

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"