Closed Bug 96085 Opened 23 years ago Closed 23 years ago

bypassing group security checks using duplicate bugs

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

Product:
Bugzilla
Component:
Creating/Changing Bugs
Version:
2.13
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.14

People

(Reporter: bbaetz, Assigned: caillon)

References

Details

Attachments

(6 files)

proposed patch for part A of justdave's suggestion
981 bytes, patch
Details | Diff | Splinter Review
alpha patch to fix this with $::FORM{'knob'} hardcoded in CGI.pl
7.33 KB, patch
Details | Diff | Splinter Review
better patch
6.34 KB, patch
Details | Diff | Splinter Review
Patch rev3
6.37 KB, patch
Details | Diff | Splinter Review
Patch rev4; includes Jakes suggestions and other minor fixes.
7.14 KB, patch
Details | Diff | Splinter Review
Patch v1.5
7.16 KB, patch
Details | Diff | Splinter Review 
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
Target Milestone: --- → Bugzilla 2.14
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 ?
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.
The "I can see bugs if I'm not logged in" bug has been filed as bug 96099.

dave: That sounds fine.
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?  :)
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
patch.
Status: NEW → ASSIGNED
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
Status: ASSIGNED → NEW
Really taking bug now...
Status: NEW → ASSIGNED
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)
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
Keywords: patch, review
Small oversight in my latest patch.

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

should be:

if (defined $::FORM{'dup_id'} && $::FORM{'knob'} == "duplicate") {

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.
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).
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
Attachment 47052 [details] [diff] works for me.

r=jake
Checked In.
Status: ASSIGNED → RESOLVED
Closed: 23 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.

Attachment

General

Creator:
Created:
Updated:
Size: