Closed Bug 233486 Opened 16 years ago Closed 16 years ago

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

Categories

(Bugzilla :: Administration, task, P2, major)

2.17.4
x86
Windows 2000

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: Felix.Hieronymi, Assigned: bugreport)

Details

(Whiteboard: [does not affect 2.16.x] [fixed in 2.18rc1])

Attachments

(2 files, 3 obsolete files)

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
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?
Summary: Browser hangs while performing editusers.cgi - updated users site is shown incompletely → Browser hangs while performing editusers.cgi - updated users page is shown incompletely
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.
Is your Bugzilla running on a Windows server?
No, our Bugzilla is running on a Linux server (Debian/testing).
Version: unspecified → 2.17.4
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/ .
Status: UNCONFIRMED → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
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?
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
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?
No, this is the only error message.
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.

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

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.
Attachment #141672 - Attachment is obsolete: true
Even though this is an administrator-only privilege escalation, let's make this
secure until fixed.
Group: webtools-security
Even though this is an administrator-only privilege escalation, let's make this
secure until fixed.
Status: UNCONFIRMED → ASSIGNED
CC list accessible: false
Ever confirmed: true
Not accessible to reporter
Attachment #142334 - Flags: review?(justdave)
Assignee: justdave → bugreport
Severity: normal → major
Status: ASSIGNED → NEW
CC list accessible: true
Priority: -- → P2
Accessible to reporter
Target Milestone: --- → Bugzilla 2.18
Status: NEW → ASSIGNED
Felix/Daniel: can you try this new patch in place of your original one and see
if this also fixes it for you?
Whiteboard: [does not affect 2.16.x] [wanted for 2.17.7]
Ok, we tried this new patch and it works.
Comment on attachment 142334 [details] [diff] [review]
Patch that addresses the actual problem

Looks good here, too.
Attachment #142334 - Flags: review?(justdave)
Attachment #142334 - Flags: review?(bbaetz)
Attachment #142334 - Flags: review+
Flags: approval?
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. :)
Flags: approval?
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
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 
??
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.
Whiteboard: [does not affect 2.16.x] [wanted for 2.17.7] → [does not affect 2.16.x] [wanted for 2.18rc1]
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?
Attachment #142334 - Flags: review?(bbaetz) → review-
Flags: blocking2.18+
Attached patch Prettier patch (obsolete) — Splinter Review
OK....	done in both places
Attachment #142334 - Attachment is obsolete: true
Attachment #144200 - Flags: review?(justdave)
Attachment #144200 - Flags: review?(bbaetz)
Attachment #144200 - Flags: review?(justdave) → review+
Comment on attachment 144200 [details] [diff] [review]
Prettier patch

r2=zach
Attachment #144200 - Flags: review?(bbaetz) → review?
Attachment #144200 - Flags: review?
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1] → [does not affect 2.16.x] [ready for 2.18rc1]
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.
This is the same patch, but bitrot cleaned up
Attachment #144200 - Attachment is obsolete: true
Attachment #152374 - Flags: review?(justdave)
Comment on attachment 152374 [details] [diff] [review]
de-bitrotted patch

r=justdave
Attachment #152374 - Flags: review?(justdave) → review+
Flags: approval+
checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Whiteboard: [does not affect 2.16.x] [ready for 2.18rc1] → [does not affect 2.16.x] [fixed in 2.18rc1]
Clearing the security flag on disclosed bugs
Group: webtools-security
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.