123077 - token.cgi password change accepted with blank second field

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P2)

Description

[Geoff Morrison]

If a user goes to change his/her password and leaves the second password field blank, Bugzilla will change the user's password to the contents of the first field.
Reccomend changing line 838 of globals.pl (ValidatePassword function) from
   elsif ( $matchpassword && ...)
to
   elsif ( defined($matchpassword) && ...)

Comment 1

[Dave Miller [:justdave]]

Zippy's QA just duplicated this....

Comment 2

[Dave Miller [:justdave]]

*** Bug 210967 has been marked as a duplicate of this bug. ***

Comment 3

[Christian Reis]

Vladd, this one may interest you :)

Comment 4

[Vlad Dascalu]

Sure.

Comment 5

[Vlad Dascalu]

I can't reproduce this on landfill, marking fixed.

There are 2 interfaces for ValidatePassword in globals.pl, with one and
respectively 2 params. The sub works right now okay in both modes, but if we add
the change proposed in item0, it will fail for calls that use only one param
(you can grep the source with grep -R "ValidatePassword" * in order to see the
interfaces and their utilization).

In userprefs.cgi we have:

($pwd1 eq $pwd2) || ThrowUserError("passwords_dont_match");
$::FORM{'new_password1'} || ThrowUserError("new_password_missing");
ValidatePassword($pwd1);

which assures that we check for non-empty password on the 2nd password field.

Suggesting WORKS FOR ME or something similar.

Comment 6

[Vlad Dascalu]

(not marking fixed after all, leaving that to someone else who can confirm what
I said)

Comment 7

[Christian Reis]

> The sub works right now okay in both modes, but if we add the change proposed
in > item0, it will fail for calls that use only one param (you can grep the source 
> with grep -R "ValidatePassword" * in order to see the interfaces and their 
> utilization).

Hmm, I don't see how checking for defined would break calls that only use one
argument. It's practically what happens today -- when it's undefined it will
evaluate as false -- except that when the password *is* supplied and is undef,
it will validate (incorrectly).

This bug is reproducible in CVS HEAD. It happens *only* in token.cgi, however,
which means that you need to enter your email to request a change on the login
page (it doesn't affect userprefs, which only uses one argument).

Now that I look at it, it actually seems userprefs should also use the
double-argument function, since it duplicates the check, doesn't it? We could
clean that up..

Comment 8

[Vlad Dascalu]

Attachment: Version 1

Comment 9

[Dave Miller [:justdave]]

reproduced this on the 2.16 branch as well...  I'll take this for 2.17.7 if it
makes it, but it's my intention to release 2.17.7 before I sleep again if I can
manage it ;)

Comment 10

[Christian Reis]

Comment on attachment 142770 [details] [diff] [review]
Version 1

Low-risk and nice!

Please ensure that this works and doesn't regress anything (places to test are
editusers.cgi password changing, userprefs.cgi password changing, and token.cgi
password changing :-)

The 2.16 branch could also use the globals.pl patch.. let's see what justdave
says to that.

Comment 11

[Vlad Dascalu]

Attachment: Backport for BUGZILLA-2_16-BRANCH

Comment 12

[Christian Reis]

Comment on attachment 142782 [details] [diff] [review]
Backport for BUGZILLA-2_16-BRANCH

How crufty that looks :-)

(Same yadda yaddas apply to this patch, I can't do any testing today)

Comment 13

[Christian Reis]

Does this bug really go all the way back to 2.10?

Comment 14

[Christian Reis]

Low-risk fixes for a mild dataloss/user annoyance problem.

Comment 15

[Vlad Dascalu]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.255; previous revision: 1.254
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.52; previous revision: 1.51
done

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.20; previous revision: 1.169.2.19
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.35.2.4; previous revision: 1.35.2.3
done

Comment 16

[Dave Miller [:justdave]]

err... didn't mean to do that