Closed
Bug 109690
Opened 23 years ago
Closed 23 years ago
[security] longlist.cgi doesn't check that $bug is valid
Categories
(Bugzilla :: Query/Bug List, defect, P1)
Tracking
()
VERIFIED
FIXED
Bugzilla 2.16
People
(Reporter: bbaetz, Assigned: bbaetz)
References
()
Details
(Whiteboard: applied to 2.14.1)
Attachments
(4 files, 1 obsolete file)
581 bytes,
patch
|
justdave
:
review-
|
Details | Diff | Splinter Review |
617 bytes,
patch
|
justdave
:
review+
gerv
:
review+
|
Details | Diff | Splinter Review |
549 bytes,
patch
|
Details | Diff | Splinter Review | |
619 bytes,
patch
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Comment 2•23 years ago
|
||
Comment on attachment 57448 [details] [diff] [review] obvious patch r= justdave
Attachment #57448 -
Flags: review+
Comment 3•23 years ago
|
||
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•23 years ago
|
||
Attachment #57448 -
Attachment is obsolete: true
Assignee | ||
Comment 5•23 years ago
|
||
...and checked in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 6•23 years ago
|
||
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-
Assignee | ||
Comment 8•23 years ago
|
||
Attachment #57450 -
Attachment is obsolete: true
Comment 9•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 57451 [details] [diff] [review] oops r= justdave
Attachment #57451 -
Flags: review+
Comment 11•23 years ago
|
||
Comment on attachment 57451 [details] [diff] [review] oops r=gerv. Gerv
Attachment #57451 -
Flags: review+
Assignee | ||
Comment 12•23 years ago
|
||
It was a one line patch, how could it go wrong? Oops.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 13•23 years ago
|
||
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
Comment 14•23 years ago
|
||
Updated•23 years ago
|
Whiteboard: applied to 2.14.1
Comment 15•23 years ago
|
||
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
Updated•23 years ago
|
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Comment 16•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
The current code ignored bugs which didn't exist; we wanted to keep teh same behaviour.
Comment 19•23 years ago
|
||
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?
Comment 21•23 years ago
|
||
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 → ---
Comment 22•23 years ago
|
||
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•23 years ago
|
||
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
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 24•23 years ago
|
||
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•23 years ago
|
||
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
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•