39524 - process_bug.cgi doesn't check viewing permissions

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P3)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Find a restricted bug (bug 28698, for example).
2. Save the page you're reading right now (a normal show_bug.cgi page).
3. Change the form action to an absolute URL on the saved page.
4. Modify hidden form elements so they read:
<INPUT TYPE=HIDDEN NAME="delta_ts" VALUE="19950000000000">
<INPUT TYPE=HIDDEN NAME="longdesclength" VALUE="0">
<INPUT TYPE=HIDDEN NAME="id" VALUE=28698>     (use bug number from step 1)
5. Load the modified page.
6. Click "commit".

Actual result:
All of the comments on bug 28698 are displayed.

Expected result:
process_bug.cgi says something to the effect of "You don't have permission to 
view this bug, so you're not allowed to change it either."

Notes:
No "mid-air collision" items are listed.  This probably has something to do 
with the bogus timestamp.

Comment 1

[Jesse Ruderman]

Is anyone working on this set of bugs?

Comment 2

[Andreas Franke (gone)]

Attachment: Hidden Bug Viewer Tool

Comment 3

[Andreas Franke (gone)]

This bug makes bug groups for security purposes pretty useless, in all bugzilla
installations. Upping severity. Maybe there's a simple fix?

Comment 4

[Andreas Franke (gone)]

If you have editbugs permissions, you'll see all comments immediately.
If you are logged in as someone who doesn't have editbugs priviledges, you get a
message that you are not allowed to change the component from "Security: Crypto"
to "Tracking". By modifying the form and iterating over all fields it may be
possible to obtain all settings for the bug. I haven't tried it though.

Comment 5

[Jesse Ruderman]

Blake says it's possible to use this to modify a restricted bug, for example to 
change the groupset and make it no longer be restricted.

Comment 6

[Dave Miller [:justdave]]

*** Bug 69913 has been marked as a duplicate of this bug. ***

Comment 7

[Dave Miller [:justdave]]

Bug 61193 is what Jesse just said Blake said.  There's another version of the 
"hacker's tool" attached to bug 61193 with description of how to actually change 
things in the bug.  I'm making it a dupe of this one because it's the same 
problem at the core.  When one is fixed, the other will be also.

Comment 8

[Matthew Tuck [:CodeMachine]]

I got the impression from the other bug you could change the bug number with
that tool.  Is that correct?

Comment 9

[Dave Miller [:justdave]]

moving to real milestones...

Comment 10

[Blake Ross]

Matthew: You can't change the bug number, but you can change any other field.  
See the attachment to that bug; enter the number of the bug you want to change 
at the topic, modify any field, and commit.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

I'm working on some code to fix this, so reassigning the bug to myself.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch to validate bug IDs and authorize users to modify them

Comment 13

[Jacob Steenhagen]

My personal preference for the routine:
    if ($i =~ /^id_/) {
      push @idlist, substr($i, 3);
    }

would be:
    if ($i =~ /^id_(\d+)/) {
        push @idlist, $1;
    }

----

  ($groupset & $::usergroupset) == $groupset

There's a comment somewhere in bugzilla that groupset is 64-bit and the
submitter isn't sure if perl handles 64 bit math (I'm also not sure either way,
but thought it worth mentioning).

-----

If perl is capable of the math, then I'll try to test this patch tomarrow.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch that abstracts out bug id validation

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Perl can handle integers of arbitrary size using its Math::BigInt, which appears
to have been part of the default install since at least 5.004, so we could
either use that or do it in a query.  I will investigate further.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: uses MySQL for bitset operations

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: fixes typo in previous patch

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: takes comments in bug 39527 into account

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Oops, I meant "takes comments in bug 39526 into account".  Hopefully the latest
patch is the last.  It contains a slight variation of Jake's preferred regex and
doesn't use the unnecessary GroupExists and UserInGroup functions to check
whether or not the user is authorized to access the bug if "usebuggroups" is
turned on (since checking $::usergroupset against the groupset of the bug
accomplishes the same goal).

Comment 20

[Jacob Steenhagen]

Checked in.
r=jake

Comment 21

[Dave Miller [:justdave]]

Moving to Bugzilla product