diff options
| -rw-r--r-- | Bugzilla/Auth/Verify.pm | 8 | ||||
| -rw-r--r-- | Bugzilla/Token.pm | 1 | ||||
| -rw-r--r-- | Bugzilla/User.pm | 6 | ||||
| -rw-r--r-- | Bugzilla/Util.pm | 5 | ||||
| -rwxr-xr-x | editusers.cgi | 7 | ||||
| -rwxr-xr-x | token.cgi | 40 | ||||
| -rwxr-xr-x | userprefs.cgi | 2 |
7 files changed, 37 insertions, 32 deletions
diff --git a/Bugzilla/Auth/Verify.pm b/Bugzilla/Auth/Verify.pm index 52cebb5ea..deb5f4e95 100644 --- a/Bugzilla/Auth/Verify.pm +++ b/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); } diff --git a/Bugzilla/Token.pm b/Bugzilla/Token.pm index a0f6b0c8e..051514b01 100644 --- a/Bugzilla/Token.pm +++ b/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(*) diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 02f17b85d..33c8535f5 100644 --- a/Bugzilla/User.pm +++ b/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. diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index d346d2547..4a87ff042 100644 --- a/Bugzilla/Util.pm +++ b/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)> diff --git a/editusers.cgi b/editusers.cgi index 19e7ea587..5f356fb40 100755 --- a/editusers.cgi +++ b/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); } @@ -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); diff --git a/userprefs.cgi b/userprefs.cgi index d06e486ef..e8a045c4e 100755 --- a/userprefs.cgi +++ b/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}); |