bypassing group security checks using duplicate bugs

RESOLVED FIXED in Bugzilla 2.14



Creating/Changing Bugs
16 years ago
5 years ago


(Reporter: bbaetz, Assigned: Christopher Aillon (sabbatical, not receiving bugmail))


Bugzilla 2.14



(6 attachments)



16 years ago
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
2. As a user not in that group, file a bug, and dupe it against the bug which
you cannot see (

3. You can now view bug 278


16 years ago
Target Milestone: --- → Bugzilla 2.14
I don't know if bug 278 is open for everyone but I can see that bug.
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

16 years ago
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.
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

16 years ago
The "I can see bugs if I'm not logged in" bug has been filed as bug 96099.

dave: That sounds fine.
Created attachment 46554 [details] [diff] [review]
proposed patch for part A of justdave's suggestion
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

16 years ago
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].
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
Okay, I'm new to accepting bugs.  Guess I've got to change the owner first and
then accept it..  (sorry for spam).
Assignee: justdave → caillon
Really taking bug now...  
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...
Adding justdave to cc (guess he got removed when I grabbed the bug)
Created attachment 46844 [details] [diff] [review]
alpha patch to fix this with $::FORM{'knob'} hardcoded in
Created attachment 47000 [details] [diff] [review]
better patch
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

On IRC, Dave OK'd the part of my patch with the optimization since I am
using similar code in process_bug.cgi
Keywords: patch, review
Small oversight in my latest patch.

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

should be:

if (defined $::FORM{'dup_id'} && $::FORM{'knob'} == "duplicate") {
Created attachment 47001 [details] [diff] [review]
Patch rev3

Comment 18

16 years ago
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

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.
Created attachment 47026 [details] [diff] [review]
Patch rev4; includes Jakes suggestions and other minor fixes.

Comment 20

16 years ago
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).
Created attachment 47052 [details] [diff] [review]
Patch v1.5
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.
Moving to new product Bugzilla...
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → 2.13

Comment 24

16 years ago
Attachment 47052 [details] [diff] works for me.

Checked In.
Last Resolved: 16 years ago
Resolution: --- → FIXED
*** Bug 93508 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.