233486 - Browser hangs while performing editusers.cgi - updated users page is shown incompletely

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Felix Hieronymi]

User-Agent:       
Build Identifier: Bugzilla 2.17.4

The problem seems to occur as long as the user has only rights to create/edit
components (no explicit edit user rights). 
If I want to assign other people to a project group the browser will need a lot
of time to perform the editusers script. After a few minutes the "updates user"
site is shown incompletely (no other information, no footer --> see attached
picture). My administrator can't reproduce the problem because he hasn't any
problem to edit users. 
Usres with same rights have the problems too.


Reproducible: Always
Steps to Reproduce:
1.click "edit users" in footer
2.select a user
3.assign group access
4.press "update"
5.wait, wait, ...

Actual Results:  
Site is shown incompletely.

Expected Results:  
Show the result of my action and the page footer.

Browser: Mozilla Firebird / MS Explorer

Comment 1

[Felix Hieronymi]

Attachment: screenshot of browser with incomplete site

Comment 2

[Dave Miller [:justdave]]

I assume you mean the user has bless access to one or more groups, rather than
(or in addition to) having editcomponents access?

So the proper privilege setup to reproduce this is
a) user is not an admin
b) user has bless rights over one or more groups

Is that correct?

Comment 3

[Felix Hieronymi]

correct!

I'm not an admin.
I have only bless rights over one or more groups.

I could reproduce it with my admin. He gave me admin-rights and it worked 
without problems.

Comment 4

[Dave Miller [:justdave]]

Is your Bugzilla running on a Windows server?

Comment 5

[Daniel Raichle]

No, our Bugzilla is running on a Linux server (Debian/testing).

Comment 6

[Dave Miller [:justdave]]

I am unable to reproduce this.  The fact that you are running Debian is very
important.  We do not support Bugzilla on Debian (installed via their package)
because they have made a lot of modifications to the Bugzilla that they
distribute, and many of those modifications often cause new bugs that we don't
have in the core Bugzilla codebase.  I would suggest filing this bug on
http://bugs.debian.org/ .

Comment 7

[Daniel Raichle]

Hi Dave,

(In reply to comment #6)
> important.  We do not support Bugzilla on Debian (installed via their package)

we do not use their package, we downloaded it here at bugzilla.org. I reopened
this bug therefore.

Comment 8

[Dave Miller [:justdave]]

OK, good. :)   But I still can't reproduce it (either on the cvs tip or on 2.17.4)

Do you have any errors in the webserver error log when this happens?

Comment 9

[Daniel Raichle]

Indeed, there is an error in the webserver log:

[Mon Feb 16 08:54:44 2004] [error] [client 10.4.4.75] (70007)The timeout
specified has expired: ap_content_length_filter: apr_bucket_read() failed,
referer:
http://fe60406.fe.de.bosch.com/bugzilla/editusers.cgi?action=edit&user=daniel.raichle%40etas.de

Comment 10

[Dave Miller [:justdave]]

OK, the error message you're getting is indicating that Apache 2.0.x terminated
the process because it took longer than 5 minutes to run.

I'm running Apache 2.0.x on my test system, and I get almost immediate feedback
when I change group permissions (either as an admin or as a user with only bless
rights).  Are there any other related errors in the log before that?

Comment 11

[Daniel Raichle]

No, this is the only error message.

Comment 12

[Daniel Raichle]

The following could possibly help to find the bug:
We found out that it is not the admin group itself that decides whether this bug
shows up or not, it's the editusers group. As all admins automatically are in
the editusers group, too (through the group derivations), we did not find this.

Comment 13

[Daniel Raichle]

I think that I found the place where the edituser.cgi-script hangs. In line 739
of editusers.cgi there is a while loop. The script gets into the while loop but
it does not get out of the while-loop again.

The while-loop looks like this (I left out some unnecessary statements here,
they are marked with "..."):

SendSQL("SELECT id, name FROM groups");
while (my ($groupid, $name) = FetchSQLData()) {
#<-- checkpoint A1
  if ($::FORM{"oldgroup_$groupid"} != ($::FORM{"group_$groupid"} ? 1 : 0)) {
#<-- checkpoint B1
    ... # do: DELETE FROM user_group_map and INSERT INTO user_group_map
  }
#<-- checkpoint A2
  if ($editall && ($::FORM{"oldbless_$groupid"} != ($::FORM{"bless_$groupid"} ?
1 : 0))) {
#<-- checkpoint B2
    ... # do: DELETE FROM user_group_map and INSERT INTO user_group_map
  }
#<-- checkpoint A3
}

for debugging I added some checkpoints into the code (print "$groupid<br>";), I
marked the checkpoints above.

The results for a user with bless rights but not in the editusers group are:
checkpoint A1 is passed 32 times (groupids 1 .. 32)
checkpoint A2 is passed 31 times (groupids 1 .. 31)
checkpoint A3 is passed 31 times (groupids 1 .. 31)
checkpoint B1 is passed never
checkpoint B2 is passed never

The results for a user with bless rights and in the editusers group are:
checkpoint A1 is passed 64 times (groupids 1 .. 64)
checkpoint A2 is passed 64 times (groupids 1 .. 64)
checkpoint A3 is passed 64 times (groupids 1 .. 64)
checkpoint B1 is passed 1 times (groupid 58)
checkpoint B2 is passed never

Does somebody have an idea where the problem could be? What else could I do to
help you to find/reproduce the problem?

Comment 14

[Daniel Raichle]

Attachment: Patch of editusers.cgi that fixes the bug

I don't know why the bug is a bug, because it seems to me that the original
perl code is correct, but with the attached patch the bug does not occur
anymore.

Does anybody have an idea what the problem was?

Comment 15

[Gervase Markham [:gerv]]

Dan: just a note - in future, could you use diff -u (unified)? Most people find
those the easiest to read. No need to redo this patch - it's easy to understand.

Thanks,

Gerv

Comment 16

[Daniel Raichle]

In the meantime, we had the same problem when changing many bugs at once. I
found bug #233645 describing this issue. The problem is exactly the same here.

After I have read bug #233645 I'm very sure:
To reproduce this bug, you will probably need many groups in your bugzilla
system. Here in our bugzilla system we have more than 60 groups.

Comment 17

[Joel Peshkin]

Attachment: Patch that addresses the actual problem

The problem results from edituser's attempt to process group controls for which
the user has no administrative permissions.  This also means that anyone who
has permission to bless any group could hack the forms to bless or unbless any
group.

Comment 18

[Joel Peshkin]

Even though this is an administrator-only privilege escalation, let's make this
secure until fixed.

Comment 19

[Joel Peshkin]

Even though this is an administrator-only privilege escalation, let's make this
secure until fixed.

Comment 20

[Dave Miller [:justdave]]

Felix/Daniel: can you try this new patch in place of your original one and see
if this also fixes it for you?

Comment 21

[Daniel Raichle]

Ok, we tried this new patch and it works.

Comment 22

[Dave Miller [:justdave]]

Comment on attachment 142334 [details] [diff] [review]
Patch that addresses the actual problem

Looks good here, too.

Comment 23

[Dave Miller [:justdave]]

I want a second core team review on this one first, just because of the nature
of it.  It's fairly obvious but another set of eyes won't hurt. :)

Comment 24

[Bradley Baetz (:bbaetz)]

Comment on attachment 142334 [details] [diff] [review]
Patch that addresses the actual problem

Why doesn't the previous select just select only those groups the user can
bless, to start with?

If theres a reason for that, then r=bbaetz if you invert the line, ie

next unless ($editall || UserCanBlessGroup($name));

Its a bit easier to see what we're trying to do that way, I think

Comment 25

[Joel Peshkin]

Adding it to the select gets pretty clunky because of $editall

The syntax does look clearer, but the same operation appears twice in the cgi
and it looks just like this one in the earlier instance.  Should we 
a) Change both to bbaetz's clearer syntax
b) Change just this one
c) Leave both in the original (less clear) syntax 
??

Comment 26

[Dave Miller [:justdave]]

We probably ought to change both.  I'm all for having security-related code be
as easy to follow as possible. :)

2.17.7 is releasing NOW (it's way too late already), 2.18rc1 is approaching
quickly, we can put this in then.

Comment 27

[Dave Miller [:justdave]]

Comment on attachment 142334 [details] [diff] [review]
Patch that addresses the actual problem

denying review on behalf of bbaetz based on comments 24 through 26.  Joel, can
we get a new patch implementing comment 26?

Comment 28

[Joel Peshkin]

Attachment: Prettier patch

OK....	done in both places

Comment 29

[Zach Lipton [:zach]]

Comment on attachment 144200 [details] [diff] [review]
Prettier patch

r2=zach

Comment 30

[Joel Peshkin]

Proposed wording for release notes:

A user with permission to grant membership in a group to other users (i.e.
ususally an administrator) could trick editusers.cgi into granting permissions
to groups other than the group (s)he is supposed to be able to control.

Comment 31

[Joel Peshkin]

Attachment: de-bitrotted patch

This is the same patch, but bitrot cleaned up

Comment 32

[Dave Miller [:justdave]]

Comment on attachment 152374 [details] [diff] [review]
de-bitrotted patch

r=justdave

Comment 33

[Joel Peshkin]

checked in

Comment 34

[Dave Miller [:justdave]]

Clearing the security flag on disclosed bugs