Closed Bug 167485 Opened 22 years ago Closed 22 years ago

When "usebuggroups" is on, then assigning of group_id is in error


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




Bugzilla 2.18


(Reporter: ravishk, Assigned: preed)



(Whiteboard: [fixed for 2.16.1][fixed for 2.14.4])


(2 files, 1 obsolete file)

Hi there

I was going through the code of bugzilla. There in "editproducts.cgi" file, I 
found that when any product is updated then if "usebuggroups" is on then group 
is created if group already does not exist.

While creating the new group, double of the max availble bit is being done. 
Which is not proper as if some one deletes any group OR product then that free 
bit remains unused.

In the file "editgroups.cgi", bit for new group is taken in optimum way as it 
first check for any free available bit, before trying for 2*max(bit).

I think this code must be used whenever any group is being created.

code in editproducts.cgi
==== code ==========
            # Group doesn't exist.  Let's make it, the same way as we make a
            # group for a new product above.
            SendSQL("SELECT MAX(bit) FROM groups");
            my $tmp_bit = FetchOneColumn();
            if($tmp_bit < 256) {
                $bit = 256;
            } else {
                $bit = $tmp_bit * 2;
=============================== End of code in editproducts.cgi ===========

code in editgroups.cgi for same purpose
======== code in editgroups.cgi ======================
    # Major hack for bit values...  perl can't handle 64-bit ints, so I can't
    # just do the math to get the next available bit number, gotta handle
    # them as strings...  also, we're actually only going to allow 63 bits
    # because that's all that opblessgroupset masks for (the high bit is off
    # to avoid signing issues).

    my @bitvals = ('1','2','4','8','16','32','64','128','256','512','1024',




    # First the next available bit
    my $bit = "";
    foreach (@bitvals) {
        if ($bit eq "") {
            SendSQL("SELECT bit FROM groups WHERE bit=" . SqlQuote($_));
            if (!FetchOneColumn()) { $bit = $_; }
    if ($bit eq "") {
        ShowError("Sorry, you already have the maximum number of groups " .
                  "defined.<BR><BR>You must delete a group first before you " .
                  "can add any more.</B>");
        PutTrailer("<a href=editgroups.cgi>Back to the group list</a>");
=======end of code in editgroups.cgi =-==============

Hope this will be taken care..
group bits are going away RSN, after which the ids will be allowed to have gaps...
There is a bug here that will impact anyone with more than about 42 bug groups
as well.

The code in editproducts.cgi is using perl to compute the bit values.  Perl
switches to exponential math at around 2**48 and the values that it tries to
inject beyond that point are incorrect.

After adding enough poducts to bring the total number of products to 52, the
groups table starts to suffer from perl (5.6.1) math errors.

mysql> select * from groups order by bit desc limit 5;
| bit              | name       | description            | isbuggroup | userrege
xp | isactive |
| 4503599627370480 | ddddd      | ddddd Bugs Access      |          1 |
   |        1 |
| 2251799813685240 | dddd       | dddd Bugs Access       |          1 |
   |        1 |
| 1125899906842620 | ddd        | ddd Bugs Access        |          1 |
   |        1 |
|  562949953421312 | dd         | dd Bugs Access         |          1 |
   |        1 |
|  281474976710656 | cccccccccc | cccccccccc Bugs Access |          1 |
   |        1 |                                                             

This needs to be fixed in 2.16.1
Severity: normal → critical
Priority: -- → P1
Summary: When "usebuggroups" is on, then assigning of group_id is not optimum → When "usebuggroups" is on, then assigning of group_id is in error
Version: 2.14.1 → 2.16
Whiteboard: [want for 2.16.1][want for 2.14.4]
Target Milestone: --- → Bugzilla 2.18
This error can cause a user regexp to grant creategroups to a broad set of users.

mysql> select * from groups where bit & 4 ;
| bit               | name         | description                    | isbuggroup
 | userregexp | isactive |
|                 4 | creategroups | Can create and destroy groups. |          0
 |            |        1 |
|  1125899906842620 | ddd          | ddd Bugs Access                |          1
 |            |        1 |
| 18014398509481900 | eee          | eee Bugs Access                |          1
 |            |        1 |
Group: webtools-security?

a= justdave for checking to both the 2.14 and 2.16 branches assuming we get a
patch for each and the necessary reviews.  Not sure if I care about 2.18 cuz
groupset math is going away anyway there.  Though this is a bit obscure, I'd
venture to say this is almost a security bug because it could result in
accidently granting access to to groups someone shouldn't be in because of the
Perl math (which is exactly the reason editgroups has a list of the values
instead of doing the math)
Version: 2.16 → 2.14.2
Attached patch Patch for 2.16Splinter Review
Someone with a 2.14 test setup should try this also

NOTE:  This should NOT be committed to 2.17  ... it'll just create merge conflicts

Same patch works on 2.14
Comment on attachment 98834 [details] [diff] [review]
Patch for 2.16

Eww, thats ugly (with the "" and all), but then so is the rest of the

Assuming that you generated those bits with a script of some sort, r=bbaetz
Attachment #98834 - Flags: review+
(Actually, the whole chunk of code was lifted directly from editgroups)
If this weren't going away, the better way would be to use a SELECT 2 *
$lasstbit just to use the DB as a math engine, but this is hopefully at its
end-of-life as the groups system goes away, so consistency with editgroups wins out.

This patch doesn't do the "start at 256" thing that the old code did.

gerv: We don't need to reserve groups, because we know that there will not be
any more before we remove the restriction (besides, we could never use such
group 'spaces' , because noone else reserved them....)
OS: Windows 2000 → All
Hardware: PC → All
Proposed draft of relnote/advisory text

Versions Effected: 
   Bugzilla 2.16 and 2.14.3 as well as earlier versions
Sites impacted: 
   Sites using "usebuggroups" features with a total of 47 groups or more.
   When a new product is added to a Bugzilla site with 47 groups or more abd
usebuggroups is enabled, the new group will be assigned a groupset bit using
perl math that is not exact beyond 2^48.  This results in the new group being
defined with a "bit" that has several bits set.  As users are given access to
the new group, those users will also gain access to spurious lower group privileges.

How to detect the problem:
    On the editgroups page, look for "bit" values that end in a "0" such as
"4503599627370480."  These are indicative of an error in large integer math
handled by perl.

How to repair the problem in sites already impacted:
   Identify all users that are members of the incorrect groups.  Update to the
new version of Bugzilla. In the database, correct the group bit values in the
offending groups, then confirm the correct group permissions for all users who
were members of the erroneous groups.

Actions rerquired in sites not already impacted:
   Update to the new Bugzilla release and run  No further action
is needed.

Attachment #98834 - Flags: review+
Comment on attachment 98834 [details] [diff] [review]
Patch for 2.16

>+        my @bitvals = ('1','2','4','8','16','32','64','128','256','512','1024',
>+                       '2048','4096','8192','16384','32768',

As bbeatz notes, these numbers should be verified.

>+        # First the next available bit
>+        foreach (@bitvals) {

Nit: use a loop variable, not $_; "foreach my $bv (@bitvals) {"

>+                if (!FetchOneColumn()) { $bit = $_; }

Nit: style.

Other than that, r=preed.

Joel: if you want (and have no problems with this): I'll make the changes and
check this in to 2.16/2.14 for ya.
Attached patch Nits (obsolete) — Splinter Review
Joel's patch + fixed my stylistic nits noted in my r=.
Comment on attachment 100130 [details] [diff] [review]

The loop could be better written with an |else last;| in the inner if
somewhere. However, since this is vanishing from mainline this weekend, and
itsnot exactly a perf criticial area, r=bbaetz
Attachment #100130 - Flags: review+
Attached patch Checked in patchSplinter Review
Patch + nits + comments; *this* patch is going in
Attachment #100130 - Attachment is obsolete: true
Checked in on the 2.16 branch:

/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision:; previous revision:

... and the 2.14.1 branch:

/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision:; previous revision: 1.18
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [want for 2.16.1][want for 2.14.4] → [fixed for 2.16.1][fixed for 2.14.4]
Security announcement is posted.  Removing security bit.
Group: webtools-security?
*** Bug 208238 has been marked as a duplicate of this bug. ***
*** Bug 151004 has been marked as a duplicate of this bug. ***
Assignee: justdave → preed
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.