275904 - Deleting a user when logged in as Editor blows away all user flags

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[alanjstr]

I was logged in as an Editor and deleted a test account.  The admin and editor
flags were removed from all users that had them.  Hendikins repaired with PMA. 
He was also unable to repeat this when logged in as an Admin.

Comment 1

[alanjstr]

Correction, PMA was not needed because Hendikins was still logged in, so the
session variable was already set.

Comment 2

[alanjstr]

I think disabling a user did the same thing.  

Comment 3

[alanjstr]

http://lxr.mozilla.org/mozilla/source/webtools/update/developers/usermanager.php#81

WHERE `UserID`='$i'

You're executing
UPDATE `userprofiles` SET `UserMode`= '$mode', `UserTrusted`= '$trusted' WHERE
`UserID`='$i'
for everyone, every time.  And you're not matching it on userid, you're matching
on some integer i

Comment 4

[alanjstr]

I think we need to split the single-user editor from this, too.  There's more
potential for something to get exploited if a low level user is in the same php.

Comment 5

[alanjstr]

Ok, I see how the names are being generated.  The iteration itself looks ok.

Comment 6

[alanjstr]

$selecteduser = escape_string($_POST["selected$i"]);

Maybe this is the problem:
This is a checkbox now.  Checkboxes have values of true and false, checked or
unchecked.  So I don't think its passing in the actual value of the user in
question.  For that, you'd just want to use $i

Since the user is selected, you're doing WHERE `UserID`='TRUE'

I'm impressed that I didn't accidentally delete all users when I went to delete
only 1.  It probably went through and disabled instead of deleting that user.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 8

[alanjstr]

Are you sure that's the right patch?  The only thing that hits is Trusted, and I
think it would break it.  

$i is the userid we're currently processing.  $selected is the checkbox that
tells us if the box on the left is checked.  Compare lines 81 and 91.  91 should
look like 81, not the other way around.  Line 98 should also be like 81.  117,
in turn, would be wrong.  etc.

I'm not sure why we keep doing checkformkey() like that.  Shouldn't we be doing 
if ($selected =="TRUE") ?

$selected == $selecteduser == escape_string($_POST["selected$i"])

If you have to rewrite a large chunk of code so that it makes sense, please do
it.  Put the SQL into a separate file, indent, whatever.  

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #8)
> Are you sure that's the right patch?  The only thing that hits is Trusted, and I
> think it would break it.

It seems to have solved the problem - you can try it in my area:
http://update.ctho.ath.cx:9305/developers

> $i is the userid we're currently processing.  $selected is the checkbox that
> tells us if the box on the left is checked.  Compare lines 81 and 91.  91 should
> look like 81, not the other way around.  Line 98 should also be like 81.  117,
> in turn, would be wrong.  etc.

Are you sure you read the code correctly?  It seems to me like all of those
lines are using effectively the same WHERE clause (plus or minus an
escape_strings call)

> 
> I'm not sure why we keep doing checkformkey() like that.

In the form keys bug, it was decided to check form keys before every DB access,
instead of at the top of the PHP.

> If you have to rewrite a large chunk of code so that it makes sense, please do
> it.  Put the SQL into a separate file, indent, whatever.  

I think it would be better to quickly fix the bugs that are blocking use of
/developers first, but I'll start working on that too.

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I looked more carefully at what's going on, and this patch does seem to be correct:
<INPUT NAME="selected81" TYPE="CHECKBOX" VALUE="81" TITLE="Selected User">
...so grabbing the value would give us the user ID

Comment 11

[alanjstr]

Removing security flag with permission from cbeard.  

Comment 12

[alanjstr]

Comment on attachment 169782 [details] [diff] [review]
patch

After explanation on IRC, I approve of this patch.  Now just get someone to
check it in for you.

Comment 13

[Giorgio Maone [:ma1]]

Asked by Alan, I'll take a look to original code later

Comment 14

[Giorgio Maone [:ma1]]

After a first look, it seems checkbox handling culd be more orthodox, simple,
clear and efficient. More on this later (maybe a patch).

Comment 15

[alanjstr]

mao -
I'm satisfied with the fix now.  We just need cbeard to approve the checkin.

Comment 16

[Wolf [:wolf]]

Comment on attachment 169782 [details] [diff] [review]
patch

Looks good to me. r=wolf

Comment 17

[Wolf [:wolf]]

checked in.

cbeard: this needs to land on the live site.

Comment 18

[alanjstr]

Fixed in CVS.