Nothing Special   »   [go: up one dir, main page]

Page MenuHomePhabricator

Proposal: introduce an expiry time for the temporary password
Closed, ResolvedPublic

Description

Hello all,

after a discussion with Anders Wegge because of the bugzilla 2126 [1] (now solved),
please let me propose:

  • to add for security reasons a reasonable expiry time (minutes/hours/days)

for the temporary password,

which the wiki sends on a user's request.

Remark 1:
Currently, the temporary password remain valid until a new is requested (~forever)
Remark 2:
The regular password is never touched by this mechanism; it remains valid until
is is changed by the user in via Special:Preferences.

The expiry time can be handled in a similar way as it was recently introduced by
Brion for the e-mail address confirmation token [2]).

[1] http://bugzilla.wikimedia.org/show_bug.cgi?id=2126
Unable to set new password after using emailed password (= temporary password
can only be used once)
[2] http://bugzilla.wikipedia.org/show_bug.cgi?id=866
EConfirm (EC): e-mail address confirmation by sending a link comprising a token
to the unconfirmed mailaddress


Version: 1.5.x
Severity: enhancement

Details

Reference
bz2242

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 8:29 PM
bzimport set Reference to bz2242.
bzimport added a subscriber: Unknown Object (MLST).

The temporary password now is revoked after the first login with it (which
forces a password change), which should remove some of this pressure. There's
not currently an expiry though.

(In reply to comment #1)

The temporary password now is revoked after the first login with it (which
forces a password change), which should remove some of this pressure. There's
not currently an expiry though.

Brion: sounds reasonable.
Greetings

titoxd.wikimedia wrote:

So is this a WORKSFORME?

ayg wrote:

No, because there's still no expiry. I don't know if we need one, though.

robchur wrote:

The timestamp a new password was last requested is stored in user.user_newpass_time; Tim added it in r17217 to facilitate throttling, but there's no reason it couldn't be used to force expiration, if desired.

Created attachment 5032
Adds 48 hour default expiry time for password reminder emails.

Patch against HEAD: Uses $wgPasswordReminderResetTime to check for expired passwords. Uses user.user_newpass_time.

attachment SpecialUserlogin.diff ignored as obsolete

Couple notes:

  1. Setting the expiration in hours seems sub-ideal to me, since nearly all time-based config options are in seconds.

I'd recommend renaming $wgPasswordReminderResetTime to $wgNewPasswordExpiry and setting it in seconds.

  1. The password reset form also uses checkTemporaryPassword() and it looks like it'll take the 'EXPIRED' return as 'true', indicating that it's ok to do the reset, thus bypassing the expiry.

As a general security principle against exposing information leaks, as well as to avoid any other potential call funkiness, it might be best to simply return false here, considering the expired password to just not match. This would be the same as if the temporary password had been wiped out, say by another new password request or a successful reset completion -- these cases would not tell you that it used to be correct, they'd just consider it invalid.

  1. The new password email text should include the expiry time.
  1. I'd recommend 7 days rather than 2 as the default; I know I don't get around to some websites within 48 hours if I get busy doing something else (say, over the weekend).
  1. Patch appears to be adding UTF-8 BOM characters, need to be removed. :)

Created attachment 5631
Updated diff - Blind check of temporary password

OK, this patch has checkTemporaryPassword() return false if the password is expired, meaning it will only appear as a wrong password.

The only thing I am not sure of is what happens after their password expires. Is there account simply locked out?

attachment TempPass.diff ignored as obsolete

Looking good! A couple quick bits...

+ if( time() < $expiry ) {
+ return false;
+ } else {
+ return true;
+ }

I think you want to reverse these...

+Your temporary password will expire in $5 days.

This should probably use {{PLURAL}} to properly handle the case where it's set to 1 day. Might want to round, also, in case a non-even number is used (7.33327 days :)

The only thing I am not sure of is what happens after their password expires.
Is there account simply locked out?

Once the temporary password is expired, it just won't work anymore. To log in with a temporary password, a new temporary password will have to be requested. The original password continues to work the whole time, unless the temporary password is actually used successfully.

Created attachment 5642
Update to last patch

Updated against the last patch.

(In reply to comment #9)

Looking good! A couple quick bits...

+ if( time() < $expiry ) {
+ return false;
+ } else {
+ return true;
+ }

I think you want to reverse these...

Done.

+Your temporary password will expire in $5 days.

This should probably use {{PLURAL}} to properly handle the case where it's set
to 1 day. Might want to round, also, in case a non-even number is used (7.33327
days :)

Did the PLURAL support, as well as rounded.

Attached:

Went ahead and applied patch in r45450.

<b>Notice</b>: Undefined variable: wgNewPasswordExpiry in <b>/Library/WebServer/Documents/trunk/includes/User.php</b> on line <b>2710</b><br />

Reverted in r45485