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

VERIFIED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Query/Bug List
P1
blocker
VERIFIED FIXED
16 years ago
5 years ago

People

(Reporter: bbaetz, Assigned: bbaetz)

Tracking

2.15
Bugzilla 2.16
x86
Linux

Details

(Whiteboard: applied to 2.14.1, URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
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
(Assignee)

Comment 1

16 years ago
Created attachment 57448 [details] [diff] [review]
obvious patch
Comment on attachment 57448 [details] [diff] [review]
obvious patch

r= justdave
Attachment #57448 - Flags: review+
Comment on attachment 57448 [details] [diff] [review]
obvious patch

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

detaint_natural($bug) || next;

Gerv
Attachment #57448 - Flags: review+
(Assignee)

Comment 4

16 years ago
Created attachment 57450 [details] [diff] [review]
fix style nits
Attachment #57448 - Attachment is obsolete: true
(Assignee)

Comment 5

16 years ago
...and checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
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?
Attachment #57450 - Flags: review-
back it out, it's broken
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

16 years ago
Created attachment 57451 [details] [diff] [review]
oops
Attachment #57450 - Attachment is obsolete: true
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 on attachment 57451 [details] [diff] [review]
oops

r= justdave
Attachment #57451 - Flags: review+
Comment on attachment 57451 [details] [diff] [review]
oops

r=gerv.

Gerv
Attachment #57451 - Flags: review+
(Assignee)

Comment 12

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

Oops.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
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.
Attachment #57450 - Attachment is obsolete: false
Created attachment 58206 [details] [diff] [review]
Combination of the previous two patches, applies cleanly to 2.14
Whiteboard: applied to 2.14.1
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
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
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
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.
(Assignee)

Comment 18

16 years ago
The current code ignored bugs which didn't exist; we wanted to keep teh same 
behaviour.
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.

Opening security bugs for which fixes have appeared in official bugzilla
release.  As per justdave and his posse.
Group: security?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
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.
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"
Status: RESOLVED → VERIFIED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.