Bug 340538: Insecure dependency in exec while running with -T switch at…

Bugzilla/Auth/Verify.pm

@@ -77,6 +77,11 @@ sub create_or_update_user {
               || return { failure => AUTH_ERROR, 
                           error   => 'auth_invalid_email',
                           details => {addr => $username} };
+            # Usually we'd call validate_password, but external authentication
+            # systems might follow different standards than ours. So in this
+            # place here, we call trick_taint without checks.
+            trick_taint($password);
+
             # XXX Theoretically this could fail with an error, but the fix for
             # that is too involved to be done right now.
             my $user = Bugzilla::User->create({ 
@@ -111,9 +116,6 @@ sub create_or_update_user {
         validate_email_syntax($username)
           || return { failure => AUTH_ERROR, error => 'auth_invalid_email',
                       details => {addr => $username} };
-        # Username is more than likely tainted, but we only use it in a
-        # placeholder, and we've already validated it, so it's safe.
-        trick_taint($username);
         $dbh->do('UPDATE profiles SET login_name = ? WHERE userid = ?',
                  undef, $username, $user->id);
     }

Bugzilla/Token.pm

@@ -59,7 +59,6 @@ sub issue_new_user_account_token {
     # an error because the user may have lost his email with the token inside.
     # But to prevent using this way to mailbomb an email address, make sure
     # the last request is at least 10 minutes old before sending a new email.
-    trick_taint($login_name);
 
     my $pending_requests =
         $dbh->selectrow_array('SELECT COUNT(*)

Bugzilla/User.pm

@@ -1490,7 +1490,8 @@ sub is_available_username {
 sub login_to_id {
     my ($login, $throw_error) = @_;
     my $dbh = Bugzilla->dbh;
-    # $login will only be used by the following SELECT statement, so it's safe.
+    # No need to validate $login -- it will be used by the following SELECT
+    # statement only, so it's safe to simply trick_taint.
     trick_taint($login);
     my $user_id = $dbh->selectrow_array("SELECT userid FROM profiles WHERE " .
                                         $dbh->sql_istrcmp('login_name', '?'),
@@ -1525,6 +1526,8 @@ sub validate_password {
     } elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
         ThrowUserError('passwords_dont_match');
     }
+    # Having done these checks makes us consider the password untainted.
+    trick_taint($_[0]);
     return 1;
 }
 
@@ -1966,6 +1969,7 @@ we return an empty string.
 
 Returns true if a password is valid (i.e. meets Bugzilla's
 requirements for length and content), else returns false.
+Untaints C<$passwd1> if successful.
 
 If a second password is passed in, this function also verifies that
 the two passwords match.

Bugzilla/Util.pm

@@ -456,6 +456,10 @@ sub validate_email_syntax {
     my ($addr) = @_;
     my $match = Bugzilla->params->{'emailregexp'};
     my $ret = ($addr =~ /$match/ && $addr !~ /[\\\(\)<>&,;:"\[\] \t\r\n]/);
+    if ($ret) {
+        # We assume these checks to suffice to consider the address untainted.
+        trick_taint($_[0]);
+    }
     return $ret ? 1 : 0;
 }
 
@@ -893,6 +897,7 @@ and tokens.
 
 Do a syntax checking for a legal email address and returns 1 if
 the check is successful, else returns 0.
+Untaints C<$email> if successful.
 
 =item C<validate_date($date)>
 

editusers.cgi

@@ -257,14 +257,13 @@ if ($action eq 'search') {
         my @values;
 
         if ($login ne $otherUser->login) {
-            # Validate, then trick_taint.
+            # Validating untaints for us.
             $login || ThrowUserError('user_login_required');
             validate_email_syntax($login)
               || ThrowUserError('illegal_email_address', {addr => $login});
             is_available_username($login)
               || ThrowUserError('account_exists', {email => $login});
 
-            trick_taint($login);
             push(@changedFields, 'login_name');
             push(@values, $login);
             $logoutNeeded = 1;
@@ -280,9 +279,8 @@ if ($action eq 'search') {
             push(@values, $realname);
         }
         if ($password) {
-            # Validate, then trick_taint.
+            # Validating untaints for us.
             validate_password($password) if $password;
-            trick_taint($password);
             push(@changedFields, 'cryptpassword');
             push(@values, bz_crypt($password));
             $logoutNeeded = 1;
@@ -296,7 +294,6 @@ if ($action eq 'search') {
             $logoutNeeded = 1;
         }
         if ($disable_mail != $otherUser->email_disabled) {
-            trick_taint($disable_mail);
             push(@changedFields, 'disable_mail');
             push(@values, $disable_mail);
         }

token.cgi

@@ -64,9 +64,8 @@ if ($cgi->param('t')) {
   $::token = $cgi->param('t');
   
   # Make sure the token contains only valid characters in the right amount.
-  # Validate password will throw an error if token is invalid
+  # validate_password will throw an error if token is invalid
   validate_password($::token);
-  trick_taint($::token); # Only used in placeholders
 
   Bugzilla::Token::CleanTokenTable();
 
@@ -102,8 +101,10 @@ if ($cgi->param('t')) {
 # If the user is requesting a password change, make sure they submitted
 # their login name and it exists in the database, and that the DB module is in
 # the list of allowed verification methods.
+my $login_name;
 if ( $::action eq 'reqpw' ) {
-    defined $cgi->param('loginname')
+    $login_name = $cgi->param('loginname');
+    defined $login_name
       || ThrowUserError("login_needed_for_password_change");
 
     # check verification methods
@@ -111,27 +112,25 @@ if ( $::action eq 'reqpw' ) {
         ThrowUserError("password_change_requests_not_allowed");
     }
 
-    # Make sure the login name looks like an email address.
-    validate_email_syntax($cgi->param('loginname'))
-      || ThrowUserError('illegal_email_address',
-                        {addr => $cgi->param('loginname')});
+    validate_email_syntax($login_name)
+        || ThrowUserError('illegal_email_address', {addr => $login_name});
 
-    my $loginname = $cgi->param('loginname');
-    trick_taint($loginname); # Used only in a placeholder
     my ($user_id) = $dbh->selectrow_array('SELECT userid FROM profiles WHERE ' .
                                           $dbh->sql_istrcmp('login_name', '?'),
-                                          undef, $loginname);
+                                          undef, $login_name);
     $user_id || ThrowUserError("account_inexistent");
 }
 
 # If the user is changing their password, make sure they submitted a new
 # password and that the new password is valid.
+my $password;
 if ( $::action eq 'chgpw' ) {
-    defined $cgi->param('password')
+    $password = $cgi->param('password');
+    defined $password
       && defined $cgi->param('matchpassword')
       || ThrowUserError("require_new_password");
 
-    validate_password($cgi->param('password'), $cgi->param('matchpassword'));
+    validate_password($password, $cgi->param('matchpassword'));
 }
 
 ################################################################################
@@ -143,13 +142,13 @@ if ( $::action eq 'chgpw' ) {
 # that variable and runs the appropriate code.
 
 if ($::action eq 'reqpw') { 
-    requestChangePassword(); 
+    requestChangePassword($login_name);
 } elsif ($::action eq 'cfmpw') { 
     confirmChangePassword(); 
 } elsif ($::action eq 'cxlpw') { 
     cancelChangePassword(); 
 } elsif ($::action eq 'chgpw') { 
-    changePassword(); 
+    changePassword($password);
 } elsif ($::action eq 'cfmem') {
     confirmChangeEmail();
 } elsif ($::action eq 'cxlem') {
@@ -176,7 +175,8 @@ exit;
 ################################################################################
 
 sub requestChangePassword {
-    Bugzilla::Token::IssuePasswordToken($cgi->param('loginname'));
+    my ($login_name) = @_;
+    Bugzilla::Token::IssuePasswordToken($login_name);
 
     $vars->{'message'} = "password_change_request";
 
@@ -203,11 +203,11 @@ sub cancelChangePassword {
 }
 
 sub changePassword {
+    my ($password) = @_;
     my $dbh = Bugzilla->dbh;
 
     # Create a crypted version of the new password
-    my $cryptedpassword = bz_crypt($cgi->param('password'));
-    trick_taint($cryptedpassword); # Used only in a placeholder
+    my $cryptedpassword = bz_crypt($password);
 
     # Get the user's ID from the tokens table.
     my ($userid) = $dbh->selectrow_array('SELECT userid FROM tokens
@@ -369,13 +369,13 @@ sub request_create_account {
 sub confirm_create_account {
     my (undef, undef, $login_name) = Bugzilla::Token::GetTokenData($::token);
 
-    validate_password($cgi->param('passwd1') || '', 
-                      $cgi->param('passwd2') || '');
+    my $password = $cgi->param('passwd1') || '';
+    validate_password($password, $cgi->param('passwd2') || '');
 
     my $otheruser = Bugzilla::User->create({
         login_name => $login_name, 
         realname   => $cgi->param('realname'), 
-        cryptpassword => $cgi->param('passwd1')});
+        cryptpassword => $password});
 
     # Now delete this token.
     delete_token($::token);

userprefs.cgi

@@ -100,7 +100,6 @@ sub SaveAccount {
 
             if ($cgi->param('Bugzilla_password') ne $pwd1) {
                 my $cryptedpassword = bz_crypt($pwd1);
-                trick_taint($cryptedpassword); # Only used in a placeholder
                 $dbh->do(q{UPDATE profiles
                               SET cryptpassword = ?
                             WHERE userid = ?},
@@ -129,7 +128,6 @@ sub SaveAccount {
             # Before changing an email address, confirm one does not exist.
             validate_email_syntax($new_login_name)
               || ThrowUserError('illegal_email_address', {addr => $new_login_name});
-            trick_taint($new_login_name);
             is_available_username($new_login_name)
               || ThrowUserError("account_exists", {email => $new_login_name});