Bug 312441: relogin.cgi allows you to impersonate user accounts you are not allowed to see when 'usevisibilitygroups' is on - Patch by A. Karl Kornel <karl@kornel.name> r=LpSolit a=justdave

5 files changed, 100 insertions, 105 deletions

diff --git a/relogin.cgi b/relogin.cgi
index 8c4517f0c..57e5e22aa 100755
--- a/relogin.cgi
+++ b/relogin.cgi
@@ -31,6 +31,7 @@ use Bugzilla;
 use Bugzilla::BugMail;
 use Bugzilla::Constants;
 use Bugzilla::Error;
+use Bugzilla::Token;
 use Bugzilla::User;
 use Bugzilla::Util;
 use Date::Format;
@@ -43,8 +44,8 @@ my $action = $cgi->param('action') || 'logout';
 my $vars = {};
 my $target;
 
-# sudo: Display the sudo information & login page
-if ($action eq 'sudo') {
+# prepare-sudo: Display the sudo information & login page
+if ($action eq 'prepare-sudo') {
     # We must have a logged-in user to do this
     # That user must be in the 'bz_sudoers' group
     my $user = Bugzilla->login(LOGIN_REQUIRED);
@@ -60,77 +61,44 @@ if ($action eq 'sudo') {
         ThrowUserError('sudo_in_progress', { target => $user->login });
     }
 
-    # We may have been given a value to put into the field
-    # Don't pass it through unless it's actually a user
-    # Could the default value be protected?  Maybe, but we will save the
-    # disappointment for later!
-    if (defined($cgi->param('target_login')) &&
-        Bugzilla::User::login_to_id($cgi->param('target_login')) != 0)
-    {
-        $vars->{'target_login_default'} = $cgi->param('target_login');
-    }
+    # Keep a temporary record of the user visitng this page
+    $vars->{'token'} = Bugzilla::Token::IssueSessionToken('sudo_prepared');
 
     # Show the sudo page
-    $vars->{'will_logout'} = $user->get_flag('can_logout');
+    $vars->{'target_login_default'} = $cgi->param('target_login');
+    $vars->{'reason_default'} = $cgi->param('reason');
     $target = 'admin/sudo.html.tmpl';
 }
-# transition-sudo: Validate target, logout user, and redirect for session start
-elsif ($action eq 'sudo-transition') {
-    # Get the current user
-    my $user = Bugzilla->login(LOGIN_REQUIRED);
-    unless ($user->in_group('bz_sudoers')) {
-        ThrowUserError('auth_failure', {  group => 'bz_sudoers',
-                                         action => 'begin',
-                                         object => 'sudo_session' }
-        );
-    }
-    
-    # Get & verify the target user (the user who we will be impersonating)
-    unless (defined($cgi->param('target_login')) &&
-            Bugzilla::User::login_to_id($cgi->param('target_login')) != 0)
-    {
-        ThrowUserError('invalid_username', 
-                       { 'name' => $cgi->param('target_login') }
-        );
-    }
-    my $target_user = new Bugzilla::User(
-        Bugzilla::User::login_to_id($cgi->param('target_login'))
-    );
-    unless (defined($target_user) &&
-            $target_user->id != 0)
-    {
-        ThrowUserError('invalid_username', 
-                       { 'name' => $cgi->param('target_login') }
-        );
-    }
-    unless (defined($cgi->param('target_login')) &&
-            $target_user->id != 0)
+# begin-sudo: Confirm login and start sudo session
+elsif ($action eq 'begin-sudo') {
+    # We must be sure that the user is authenticating by providing a login
+    # and password.
+    # We only need to do this for authentication methods that involve Bugzilla 
+    # directly obtaining a login (i.e. normal CGI login), as opposed to other 
+    # methods (like Environment vars login).  We assume that if a user can log 
+    # out, they can also log in:
+
+    # First, record if Bugzilla_login and Bugzilla_password were provided
+    my $credentials_provided;
+    if (defined($cgi->param('Bugzilla_login'))
+        && defined($cgi->param('Bugzilla_password')))
     {
-        ThrowUserError('invalid_username', 
-                       { 'name' => $cgi->param('target_login') }
-        );
+        $credentials_provided = 1;
     }
-    if ($target_user->in_group('bz_sudo_protect')) {
-        ThrowUserError('sudo_protected', { login => $target_user->login });
-    }
-    
-    # If we have a reason passed in, keep it under 200 characters
-    my $reason = $cgi->param('reason') || '';
-    $reason = substr($reason, $[, 200);
-    my $reason_string = '&reason=' . url_quote($reason);
     
-    # Log out and redirect user to the new page
-    Bugzilla->logout();
-    $target = 'relogin.cgi';
-    print $cgi->redirect($target . '?action=begin-sudo&target_login=' .
-                         url_quote($target_user->login) . $reason_string);
-    exit;
-}
-# begin-sudo: Confirm login and start sudo session
-elsif ($action eq 'begin-sudo') {
-    # We must have a logged-in user to do this
-    # That user must be in the 'bz_sudoers' group
+    # Next, log in the user
     my $user = Bugzilla->login(LOGIN_REQUIRED);
+    
+    # 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 throw an error.
+    if ($user->get_flag('can_logout') && !$credentials_provided) {
+        ThrowUserError('sudo_password_required',
+                       { target_login => $cgi->param('target_login'),
+                               reason => $cgi->param('reason')});
+    }
+    
+    # The user must be in the 'bz_sudoers' group
     unless ($user->in_group('bz_sudoers')) {
         ThrowUserError('auth_failure', {  group => 'bz_sudoers',
                                          action => 'begin',
@@ -138,35 +106,41 @@ elsif ($action eq 'begin-sudo') {
         );
     }
     
-    # Get & verify the target user (the user who we will be impersonating)
-    unless (defined($cgi->param('target_login')) &&
-            Bugzilla::User::login_to_id($cgi->param('target_login')) != 0)
-    {
-        ThrowUserError('invalid_username', 
-                       { 'name' => $cgi->param('target_login') }
-        );
+    # Do not try to start a new session if one is already in progress!
+    if (defined(Bugzilla->sudoer)) {
+        ThrowUserError('sudo_in_progress', { target => $user->login });
     }
-    my $target_user = new Bugzilla::User(
-        Bugzilla::User::login_to_id($cgi->param('target_login'))
-    );
-    unless (defined($target_user) &&
-            $target_user->id != 0)
+
+    # Did the user actually go trough the 'sudo-prepare' action?  Do some 
+    # checks on the token the action should have left.
+    my ($token_user, $token_timestamp, $token_data) =
+        Bugzilla::Token::GetTokenData($cgi->param('token'));
+    unless (defined($token_user)
+            && defined($token_data)
+            && ($token_user == $user->id)
+            && ($token_data eq 'sudo_prepared'))
     {
-        ThrowUserError('invalid_username', 
-                       { 'name' => $cgi->param('target_login') }
-        );
+        ThrowUserError('sudo_preparation_required', 
+                       { target_login => $cgi->param('target_login'),
+                               reason => $cgi->param('reason')});
     }
-    unless (defined($cgi->param('target_login')) &&
-            $target_user->id != 0)
+    Bugzilla::Token::DeleteToken($cgi->param('token'));
+
+    # Get & verify the target user (the user who we will be impersonating)
+    my $target_user = 
+        Bugzilla::User->new_from_login($cgi->param('target_login'));
+    unless (defined($target_user)
+            && $target_user->id
+            && $user->can_see_user($target_user))
     {
-        ThrowUserError('invalid_username', 
+        ThrowUserError('user_match_failed',
                        { 'name' => $cgi->param('target_login') }
         );
     }
     if ($target_user->in_group('bz_sudo_protect')) {
         ThrowUserError('sudo_protected', { login => $target_user->login });
     }
-    
+
     # If we have a reason passed in, keep it under 200 characters
     my $reason = $cgi->param('reason') || '';
     $reason = substr($reason, $[, 200);
@@ -175,13 +149,13 @@ elsif ($action eq 'begin-sudo') {
     my $time_string = time2str('%a, %d-%b-%Y %T %Z', time+(6*60*60), 'GMT');
 
     # For future sessions, store the unique ID of the target user
-    Bugzilla->cgi->send_cookie('-name'    => 'sudo',
-                               '-expires' => $time_string,
-                               '-value'   => $target_user->id
+    $cgi->send_cookie('-name'    => 'sudo',
+                      '-expires' => $time_string,
+                      '-value'   => $target_user->id
     );
     
     # For the present, change the values of Bugzilla::user & Bugzilla::sudoer
-    Bugzilla->sudo_request($target_user, Bugzilla->user);
+    Bugzilla->sudo_request($target_user, $user);
     
     # NOTE: If you want to log the start of an sudo session, do it here.
 
@@ -205,7 +179,6 @@ elsif ($action eq 'end-sudo') {
     Bugzilla->login(LOGIN_OPTIONAL);
     my $sudoer = Bugzilla->sudoer;
     if (defined($sudoer)) {
-        Bugzilla->logout_request();
         Bugzilla->sudo_request($sudoer, undef);
     }
 
@@ -225,10 +198,6 @@ elsif ($action eq 'logout') {
 
     Bugzilla->logout();
 
-    my $template = Bugzilla->template;
-    my $cgi = Bugzilla->cgi;
-    print $cgi->header();
-
     $vars->{'message'} = "logged_out";
     $target = 'global/message.html.tmpl';
 }
diff --git a/template/en/default/account/prefs/permissions.html.tmpl b/template/en/default/account/prefs/permissions.html.tmpl
index 2de04328d..dd6e1785b 100644
--- a/template/en/default/account/prefs/permissions.html.tmpl
+++ b/template/en/default/account/prefs/permissions.html.tmpl
@@ -74,7 +74,7 @@
       [% IF user.groups.bz_sudoers %]
         <br>
         You are a member of the <b>bz_sudoers</b> group, so you can 
-        <a href="relogin.cgi?action=sudo">impersonate someone else</a>.
+        <a href="relogin.cgi?action=prepare-sudo">impersonate someone else</a>.
       [% END %]
     </td>
   </tr>
diff --git a/template/en/default/admin/sudo.html.tmpl b/template/en/default/admin/sudo.html.tmpl
index 12aa586a6..4e781796c 100644
--- a/template/en/default/admin/sudo.html.tmpl
+++ b/template/en/default/admin/sudo.html.tmpl
@@ -66,7 +66,8 @@
   
   <p>
     Next, please take a moment to explain why you are doing this:<br>
-    <input type="text" name="reason" size="80" maxlength="200">
+    <input type="text" name="reason" size="80" maxlength="200" value="
+    [%- reason_default FILTER html %]">
   </p>
   
   <p>
@@ -75,21 +76,27 @@
     are impersonating them.
   </p>
   
-  <p>
-    Finally, click the button to begin the session:
-    <input type="submit" value="Begin Session">
-    <input type="hidden" name="action" value="sudo-transition">
-  </p>
-  
-  [% IF will_logout %]
+  [% IF user.get_flag("can_logout") %]
     <p>
-      When you press the button, you may be logged out and asked to log in 
-      again.  This is done for two reasons.  First of all, it is done to reduce 
+      Finally, enter your [% terms.Bugzilla %] password: 
+      <input type="hidden" name="Bugzilla_login" value="
+      [%- user.login FILTER html %]">
+      <input type="password" name="Bugzilla_password" maxlength="20" size="20">
+      <br>
+      This is done for two reasons.  First of all, it is done to reduce 
       the chances of someone doing large amounts of damage using your 
       already-logged-in account.  Second, it is there to force you to take the 
       time to consider if you really need to use this feature.
     </p>
   [% END %]
+  
+  <p>
+    Click the button to begin the session:
+    <input type="submit" value="Begin Session">
+    <input type="hidden" name="action" value="begin-sudo">
+    <input type="hidden" name="token" value="[% token FILTER html %]">
+  </p>
+
 </form>
 
 [% PROCESS global/footer.html.tmpl %]
diff --git a/template/en/default/admin/users/userdata.html.tmpl b/template/en/default/admin/users/userdata.html.tmpl
index f606bb73d..96c9df515 100644
--- a/template/en/default/admin/users/userdata.html.tmpl
+++ b/template/en/default/admin/users/userdata.html.tmpl
@@ -32,7 +32,7 @@
                value="[% otheruser.login FILTER html %]" />
         [% IF !otheruser.groups.bz_sudo_protect %]
           <br />
-          <a href="relogin.cgi?action=sudo&amp;target_login=
+          <a href="relogin.cgi?action=prepare-sudo&amp;target_login=
           [%- otheruser.login FILTER html %]">Impersonate this user</a>
         [% END %]
       [% END %]
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index be86ae506..e911b39d2 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -1128,6 +1128,20 @@
     An sudo session (impersonating [% target FILTER html %]) is in progress.  
     End that session (using the link in the footer) before starting a new one.
 
+  [% ELSIF error == "sudo_password_required" %]
+    [% title = "Password Required" %]
+    Your [% terms.Bugzilla %] password is required to begin a sudo 
+    session. Please <a href="relogin.cgi?action=prepare-sudo&target_login=
+    [%- target_login FILTER html %]&reason=
+    [%- reason FILTER html %]">go back</a> and enter your password</a>.
+    
+  [% ELSIF error == "sudo_preparation_required" %]
+    [% title = "Preparation Required" %]
+    You may not start a sudo session directly.  Please
+    <a href="relogin.cgi?action=prepare-sudo&target_login=
+    [%- target_login FILTER html %]&reason=
+    [%- reason FILTER html %]">start your session normally</a>.
+
   [% ELSIF error == "sudo_protected" %]
     [% title = "User Protected" %]
     The user [% login FILTER html %] may not be impersonated by sudoers.
@@ -1202,6 +1216,11 @@
     [% title = "Login Name Required" %]
     You must enter a login name for the new user.
 
+  [% ELSIF error == "user_match_failed" %]
+    [% title = "Match Failed" %]
+    <tt>[% name FILTER html %]</tt> does not exist or you are not allowed 
+    to see that user.
+
   [% ELSIF error == "votes_must_be_nonnegative" %]
     [% title = "Votes Must Be Non-negative" %]
     Only use non-negative numbers for your [% terms.bug %] votes.