Last Comment Bug 96085 - bypassing group security checks using duplicate bugs
: bypassing group security checks using duplicate bugs
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.13
: All All
: -- critical (vote)
: Bugzilla 2.14
Assigned To: Christopher Aillon (sabbatical, not receiving bugmail)
: default-qa
Mentors:
: 93508 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-08-20 11:20 PDT by Bradley Baetz (:bbaetz)
Modified: 2012-12-18 20:46 PST (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch for part A of justdave's suggestion (981 bytes, patch)
2001-08-20 20:39 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
alpha patch to fix this with $::FORM{'knob'} hardcoded in CGI.pl (7.33 KB, patch)
2001-08-22 19:43 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
better patch (6.34 KB, patch)
2001-08-23 23:54 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
Patch rev3 (6.37 KB, patch)
2001-08-24 00:12 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
Patch rev4; includes Jakes suggestions and other minor fixes. (7.14 KB, patch)
2001-08-24 09:12 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
Patch v1.5 (7.16 KB, patch)
2001-08-24 13:33 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review

Description Bradley Baetz (:bbaetz) 2001-08-20 11:20:24 PDT
Its possible for the bugzilla group security stuff to be bypassed by filing a
duplicate bug. This will add you to the cc list, and so, by default, you'll be
able to see the bug.

To reproduce:

1. Have a restricted bug which is open to people on the cclist
(http://landfill.tequilarista.org/bugzilla-tip/show_bug.cgi?id=278)
2. As a user not in that group, file a bug, and dupe it against the bug which
you cannot see (http://landfill.tequilarista.org/bugzilla-tip/show_bug.cgi?id=279)

3. You can now view bug 278
Comment 1 Matthias Versen [:Matti] 2001-08-20 12:27:38 PDT
I don't know if bug 278 is open for everyone but I can see that bug.
(foodreplicator....)
I'm not logged in but I can see this bug with NS4.7x and Mozilla but not with IE 
(I never used it on bugzilla-> no bugzilla cookies). With IE I get ".exp problem 
....)


Maybe I can see this bug because I'm watching Bradley ?
Comment 2 Bradley Baetz (:bbaetz) 2001-08-20 12:57:20 PDT
Um, I could see that bug when I wasn't logged in. but I couldn't when logged in
as another user (matti, its a sepatate db, so tyou need a separate account)

This is, like, bad.
Comment 3 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-08-20 13:18:38 PDT
OK, what we need to do here is:
a) you can't dupe a bug against a bug you can't see.
b) if someone else dupes the bug, if the reporter can't see the new bug, the CC
should not be added.

For case B it probably wouldn't hurt to put up an intermediate screen warning
the user doing the duping that the reporter can't see it.
Comment 4 Bradley Baetz (:bbaetz) 2001-08-20 13:31:30 PDT
The "I can see bugs if I'm not logged in" bug has been filed as bug 96099.

dave: That sounds fine.
Comment 5 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-20 20:39:34 PDT
Created attachment 46554 [details] [diff] [review]
proposed patch for part A of justdave's suggestion
Comment 6 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-20 20:40:36 PDT
Not sure how well that patch works, although I'm pretty sure it gets the job
done.  My database is down right now so I can't test it.  Maybe someone else
can?  :)
Comment 7 Jacob Steenhagen 2001-08-21 05:52:21 PDT
Using ValidateBugID() negates the need for the following block:

707  SendSQL("SELECT bug_id FROM bugs WHERE bug_id = " . SqlQuote($num));
708  $num = FetchOneColumn();
709  if (!$num) {
710      PuntTryAgain("You must specify a valid bug number of which this bug " .
711                   "is a duplicate.  The bug has not been changed.")
712  }

Of course, this patch doesn't do anything towards part b) [and its sub-part].
Comment 8 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-21 10:18:07 PDT
I know, Jake.  I didn't have time to work on part B last night.  I mentioned it
was only for part A.  I've got some more time now so I'll take this bug.... 

I'll remove the part you mentioned in the patch and post that later with the new
patch.
Comment 9 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-21 10:21:23 PDT
Okay, I'm new to accepting bugs.  Guess I've got to change the owner first and
then accept it..  (sorry for spam).
Comment 10 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-21 10:23:05 PDT
Really taking bug now...  
Comment 11 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-22 19:40:50 PDT
I have a somewhat functional patch with a few small glitches in it that I need
to work out.  I'm posting it for some general feedback and ideas.

1) $::FORM{'knob'} gets set to numeric value '2' in the code somewhere.  I need
to figure out why it does that.  At that point however, it should always be set
to "duplicate" so for the time being, I hard coded it in there with a print
statement displaying the value of it.
2) If both the reporter and the duper don't have access to the bug, the header
displays twice.  I'll have a fix for that shortly but I've been spending most of
my time trying to debug problem 1.

I'm a bit new to the Bugzilla code so I'm sure I've overlooked some stuff but
any help/feedback are definitely welcome.  In any case, if you can see a bug and
are duping a bug with a reporter that can't, you get to choose whether to add
them to CC or not.  Or throw away changes.  This is a start to fixing bug 92078.
 Attachment on the way...
Comment 12 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-22 19:41:54 PDT
Adding justdave to cc (guess he got removed when I grabbed the bug)
Comment 13 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-22 19:43:51 PDT
Created attachment 46844 [details] [diff] [review]
alpha patch to fix this with $::FORM{'knob'} hardcoded in CGI.pl
Comment 14 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-23 23:54:57 PDT
Created attachment 47000 [details] [diff] [review]
better patch
Comment 15 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 00:00:12 PDT
The patch I just uploaded looks like it will fix everything.  Initially I went
about it the wrong way.  I realized that when I studied the bugzilla code some
more.  Dave just applied the patch on http://landfill.tequilarista.org/bz96085/

On IRC, Dave OK'd the CGI.pl part of my patch with the optimization since I am
using similar code in process_bug.cgi
Comment 16 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 00:09:19 PDT
Small oversight in my latest patch.

if (defined $::FORM{'dup_id'}) {

should be:

if (defined $::FORM{'dup_id'} && $::FORM{'knob'} == "duplicate") {
Comment 17 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 00:12:43 PDT
Created attachment 47001 [details] [diff] [review]
Patch rev3
Comment 18 Jacob Steenhagen 2001-08-24 05:56:25 PDT
In DuplicateUserConfirm() you have:

+    # Make sure the bug exists in the database.
+    if (!MoreSQLData()) {
+        DisplayError("Bug #$original does not exist.") && exit;
+    }

Which really isn't needed because you already called:

+    ValidateBugID($::FORM{'dup_id'});

Which made sure both the $::FORM{'dup_id'} was numeric and exits in the
database.  I guess it really doesn't hurt anything because it's not another
database call, but just thought it'd be worth mentioning :)

I'm also not fond of the relying on the words "Do NOT" from the confirm page.  I
almost think that be better as option buttons:

Add the user to the CC list so they can see the bug?:
( ) Yes   ( ) No
[Commit]

The text should also, IMHO, tell the person  who is doing the duping whether
adding the user to the CC list will allow them to immediately see the bug
(cclist_accessible == 1) or just if it's just a possiblity down the line that
someone will turn on the CC list accessibility.
Comment 19 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 09:12:18 PDT
Created attachment 47026 [details] [diff] [review]
Patch rev4; includes Jakes suggestions and other minor fixes.
Comment 20 Jacob Steenhagen 2001-08-24 10:24:59 PDT
The initial scan of attachment 47026 [details] [diff] [review] looks good (I've only looked at the code, I
haven't tested anything).  I think this will probably have a conflict with bug
92266 (not a code conflict, but the changes in the two patches are close enough
together that CVS will probably flag it... it's easy enough to fix, though,
seeing how there isn't a code conflict).
Comment 21 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 13:33:59 PDT
Created attachment 47052 [details] [diff] [review]
Patch v1.5
Comment 22 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 13:36:15 PDT
Attachment 47052 [details] [diff] (1.5) is the same as 47026 (1.4) except that I have added
checked="checked" for the "NO" radio button.  Otherwise, yes was the default and
IMO, users should have to say yes as opposed to having to say no.
Comment 23 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-24 18:34:40 PDT
Moving to new product Bugzilla...
Comment 24 Jacob Steenhagen 2001-08-26 11:55:25 PDT
Attachment 47052 [details] [diff] works for me.

r=jake
Checked In.
Comment 25 Dave Miller [:justdave] (justdave@bugzilla.org) 2005-04-27 07:26:32 PDT
*** Bug 93508 has been marked as a duplicate of this bug. ***

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