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

Status: RESOLVED FIXED

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

Description

[R K Singh]

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',
                   '2048','4096','8192','16384','32768',

                   '65536','131072','262144','524288','1048576','2097152',
                   '4194304','8388608','16777216','33554432','67108864',
                   '134217728','268435456','536870912','1073741824',
                   '2147483648',

                   '4294967296','8589934592','17179869184','34359738368',
                   '68719476736','137438953472','274877906944',
                   '549755813888','1099511627776','2199023255552',
                   '4398046511104','8796093022208','17592186044416',
                   '35184372088832','70368744177664','140737488355328',

                   '281474976710656','562949953421312','1125899906842624',
                   '2251799813685248','4503599627370496','9007199254740992',
                   '18014398509481984','36028797018963968','72057594037927936',
                   '144115188075855872','288230376151711744',
                   '576460752303423488','1152921504606846976',
                   '2305843009213693952','4611686018427387904');

    # 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>");
        exit;
    }
=======end of code in editgroups.cgi =-==============


Hope this will be taken care..

Comment 1

[Bradley Baetz (:bbaetz)]

group bits are going away RSN, after which the ids will be allowed to have gaps...

Comment 2

[Joel Peshkin]

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

Comment 3

[Joel Peshkin]

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 |
+-------------------+--------------+--------------------------------+----------

Comment 4

[Dave Miller [:justdave]]

arghh.

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)

Comment 5

[Joel Peshkin]

Attachment: Patch for 2.16

Someone with a 2.14 test setup should try this also

Comment 6

[Joel Peshkin]

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

Comment 7

[Joel Peshkin]

Same patch works on 2.14

Comment 8

[Bradley Baetz (:bbaetz)]

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
script....

Assuming that you generated those bits with a script of some sort, r=bbaetz

Comment 9

[Joel Peshkin]

(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.

Comment 10

[Gervase Markham [:gerv]]

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

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

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....)

Comment 12

[Joel Peshkin]

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.
Impact:
   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 checksetup.pl.  No further action
is needed.

Comment 13

[J. Paul Reed [:preed]]

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.

Comment 14

[J. Paul Reed [:preed]]

Attachment: Nits

Joel's patch + fixed my stylistic nits noted in my r=.

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 100130 [details] [diff] [review]
Nits

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

Comment 16

[J. Paul Reed [:preed]]

Attachment: Checked in patch

Patch + nits + comments; *this* patch is going in

Comment 17

[J. Paul Reed [:preed]]

Checked in on the 2.16 branch:

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

... and the 2.14.1 branch:

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

Comment 18

[Dave Miller [:justdave]]

Security announcement is posted.  Removing security bit.

Comment 19

[Dave Miller [:justdave]]

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

Comment 20

[Dave Miller [:justdave]]

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