312441 - relogin.cgi allows you to impersonate user accounts you are not allowed to see when 'usevisibilitygroups' is on

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Frédéric Buclin]

I can imagine that some users, for instance project leaders, could have
bz_sudoers privs to enter their developers' accounts (for some internal
reasons). I can also imagine that 'usevisibilitygroups' is turned on to hide
other project groups. But if I have bz_sudoers privs, I can impersonate any user
account, including those I'm not allowed to see. Moreover, when the user account
doesn't exist, the error message says so instead of being more vague:

"Either the account [e-mail] doesn't exist, or you are not allowed to see it
and consequently, you cannot impersonate this account."

You should only be allowed to impersonate user accounts you can see.

Comment 1

[A. Karl Kornel]

Attachment: Patch v1

OK, this covers both the implementation of the fix to this bug, and a number of other cleanups.  Here we go:

In the 'sudo' action, the target_login check has been removed.  There is no need to sanity check the data now, as that is done by the template FILTER, and by processing later.

In the 'sudo-transition' action, I copied in the check to see if a session is already in progress (we don't want to start a new session if one is in progress).  I also removed two checks of the target_user param that were duplicates.  I also put in a check to see if the logged-in user can see the target user (this actually doubles as a check to see if the target user even exists).

In the 'begin-sudo' action, I put in all of the changes mentioned in the 'sudo-transition' action.

If the target user does not exist, or can not be seen, an appropriate error message is thrown (similar to the message displayed by match_field).

Comment 2

[Vlad Dascalu]

>> +    [%- name FILTER html %]</tt>).  Please go back and try again.

Sure, this is nit-picking, but: why should we ask the user to go back and try again? Maybe he realized that there is no account matching his specifications, and there is no point in trying again.

I'd rephrase that to "You can go back and try again." or something similar that doesn't ask the user to go back and try again, no matter what.

Comment 3

[Frédéric Buclin]

Comment on attachment 202175 [details] [diff] [review]
Patch v1

sudo-transition and begin-sudo should be merged. I see no valid reason to log out the user and then ask him to log in again.

I have done this change locally, and it's a big cleanup!

Comment 4

[A. Karl Kornel]

Attachment: Patch v1.5

Modification of attachment 202175 [details] [diff] [review] with respect to comment 2 and comment 3:

> >> +    [%- name FILTER html %]</tt>).  Please go back and try again.
> 
> Sure, this is nit-picking, but: why should we ask the user to go back and try
> again? Maybe he realized that there is no account matching his specifications,
> and there is no point in trying again.

Updated.

> sudo-transition and begin-sudo should be merged. I see no valid reason to log
> out the user and then ask him to log in again.

I had two separate actions to avoid a situation like this: Someone with sudo access logs in and then leaves.  Someone else then comes in soon afterwards, begins an sudo session, and wreaks havoc.  Forcing the user to log in before starting this helps to avoid this situation.

However, I can merge the two actions if I take a page from the userprefs CGI.  I have eliminated the 'sudo-transition' action.  Now the sudo HTML asks you to enter your password.  The userid is also included in the HTML, in a hidden field.  My 'begin-sudo' action checks to see if the login & password were explicitly entered.  If not, an error is thrown.  If a login & password were provided, I continue the login, which always goes to the login/password fields first (if they are available).

However, what if a login method is being used that does not use Bugzilla to get a username/password, such as environment variable authentication?  Here I work off of an assumption:  If you did not use Bugzilla to log in, you can not use Bugzilla to log out, so the can_logout flag will be unset.  I check this.  If an external source is used (like env. authentication), I do not ask for a password, and I do not check to see if they were entered.

Comment 5

[Frédéric Buclin]

Comment on attachment 202765 [details] [diff] [review]
Patch v1.5

I'm waiting for an updated patch, as discussed on IRC.

Comment 6

[A. Karl Kornel]

Attachment: Patch v1.6

Modification of attachment 202765 [details] [diff] [review], with respect to comments made on IRC:

* The action 'sudo' has been changed to 'prepare-sudo' for consistency.
* Perform one additional check of the target user to make sure it is valid.
* The 'user_match_failed' error message has been shortened.

Comment 7

[A. Karl Kornel]

Attachment: Patch v1.7

Modification of attachment 203284 [details] [diff] [review] with respect to comments made on IRC.

* A token is now used to make sure that the user begins a session by first going to the prepare-sudo page.  A new error is thrown if this is not the case.
* A new error is thrown if a login is made, can_logout is true, but a password was not directly supplied.

There have also been numerous nits fixed (too many to list here).

Comment 8

[Frédéric Buclin]

Comment on attachment 203314 [details] [diff] [review]
Patch v1.7

>Index: relogin.cgi

>+    # First, record if Bugzilla_login and Bugzilla_password were provided
>+    my $credentials_provided;
>+    if (defined($cgi->param('Bugzilla_login')) &&
>+        defined($cgi->param('Bugzilla_password'))
>+       )

Nit: please move && at the beginning of the next line and move ) on the previous one.


>+    # At this point, the user is logged in.  However, if they used a method
>+    # where they could have provided a username/password (i.e. CGI), but they 
>+    # did not provide a username/password, then log out and throw an error.

s/log out and //


>+    # (we want it to look like the user never logged in)

Remove this sentence completely. The user is not logged out.


>+    # Did the user actually go trough the 'sudo-prepare' action?

Nit: reword this comment; e.g.: "retrieve the token created in action=prepare-sudo".


>+    my @token_data = Bugzilla::Token::GetTokenData($cgi->param('token'));
>+    unless ((scalar(@token_data) == 3)
>+            && ($token_data[2] eq 'sudo_prepared'))
>+    {

Instead of @token_data, write something like ($userid, $date, $event) to make things clear. This is much better than $token_data[2].
Moreover, not only do you have to validate $event, but you also have to make sure that
$userid == $user->id !
About the lifetime of the token ($date), I don't really care; so we can leave it alone for now.


>+    unless (defined($target_user)
>+            && ($target_user->id > 0)
>+            && $user->can_see_user($target_user))

Nit: "&& $target_user->id" is enough (no need to write "> 0")


> Bugzilla->sudo_request($target_user, Bugzilla->user);

Nit: you could write $user instead of Bugzilla->user.


>Index: template/en/default/admin/sudo.html.tmpl

>+      <input type="password" name="Bugzilla_password">

Nit: a password is limited to 20 characters. Add maxlength="20".


The reason I deny review is the lack of validation of the token passed to begin-sudo. Else the patch looks great!

Comment 9

[A. Karl Kornel]

Attachment: Patch v1.8

Fixing some token usage problems and many nits!

Comment 10

[Frédéric Buclin]

Comment on attachment 203346 [details] [diff] [review]
Patch v1.8

>Index: relogin.cgi

>+    # Keep a temporary record of the user vising this page

Nit: visiting



>Index: template/en/default/admin/sudo.html.tmpl

>+      <input type="password" name="Bugzilla_password" maxlength="20" 
>+      length="20">

'length' is not a valid attribute of <input>. It must be 'size'.


Please fix these two comments on checkin. Your patch works great, r=LpSolit.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

This patch is too large, and it touches too sensitive functionality, to be a low-risk fix.  On the other hand, the fix seems critical enough to justify getting it in before the release.  But I want a second opinion, so cc:ing Dave.  Dave: what say you?

Comment 12

[Dave Miller [:justdave]]

This entire feature set makes me incredibly nervous.  I hate having it incomplete even worse because of the scope of the feature to begin with.  We should either take this or back the rest of it out.  The lesser of the two evils at this point is probably to take this patch, as the patch to back it out would be as huge.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> should either take this or back the rest of it out.  The lesser of the two
> evils at this point is probably to take this patch, as the patch to back it out
> would be as huge.

I agree, and the sooner, the better. ;)

Comment 14

[A. Karl Kornel]

Attachment: Checked-In Version

Comment 15

[A. Karl Kornel]

Checking in relogin.cgi;
/cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v  <--  relogin.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in template/en/default/account/prefs/permissions.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/permissions.html.tmpl,v  <--  permissions.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/admin/sudo.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/sudo.html.tmpl,v  <--  sudo.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/users/userdata.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v  <--  userdata.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.142; previous revision: 1.141
done

Comment 16

[Frédéric Buclin]

Comment on attachment 203676 [details] [diff] [review]
Checked-In Version

>Index: relogin.cgi
>+    # Keep a temporary record of the user visitng this page

yeah... you are closer... :-D