Page MenuHomePhabricator

Implement a way for authorized users only to use Special:PasswordReset on other usernames
Closed, ResolvedPublic

Description

During development of OpenID I noticed the following:

when logged-in user (no matter, by which method) goes to Special:PasswordReset, they see an input field for entering their username. This does not make sense.

There are these drawbacks:

  • users need to type their name (efforts and risk of typos)
  • evil users can trigger sending a new password to an arbitrary users

I cannot imagine any other purpose for PasswortReset than the user X wants to send a new passwort to user X (as mentioned "user" is - implictly - a logged persona).

I propose the following change in Special:PasswordReset

if "user" than PaswortReset shows

  • the own username in the Username field
  • this field set to readonly=readonly
  • the onSubmit callback sanitzing the return parameters and checking wether the correct name comes back
  • then mailing the temporary password to user(username)

I also need (for OpenID) a clean way of internally sending directly a temporary password to logged-in user (without the form).


Version: unspecified
Severity: enhancement
URL: http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/specials/SpecialPasswordReset.php
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=20185
https://bugzilla.wikimedia.org/show_bug.cgi?id=29027
https://bugzilla.wikimedia.org/show_bug.cgi?id=30636

Details

Reference
bz29135

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 11:29 PM
bzimport set Reference to bz29135.

Update:

introduce also a new permission "isallowed-to-reset-other-user-password" to make it fine-grained configurable. When the permission is not given, the function shall work as described in comment #1 (input box for Username shows the own Username, but this field being readonly. Value checked on return. The only action the user can perform is to send a temporary password to their own e-mail address.

Status quo:
$wgGroupPermissions["*"]["isallowed-to-reset-other-user-password"] = true;

this bug/proposal proposes:
$wgGroupPermissions["*"]["isallowed-to-reset-other-user-password"] = true;
$wgGroupPermissions["user"]["isallowed-to-reset-other-user-password"] = false;

This is a case where a "user" has less rights than a non-user, but I explained above why it makes sense in my view.

(In reply to comment #0)

  • evil users can trigger sending a new password to an arbitrary users

Then the code is wrong for allowing a user to reset the password of another.

(In reply to comment #1)

Status quo:
$wgGroupPermissions["*"]["isallowed-to-reset-other-user-password"] = true;

That's the status quo? I was under the impression this feature was never implemented (I attempted it, but was reverted).

That's the status quo? I was under the impression this feature was never
implemented (I attempted it, but was reverted).

Chad, I tested in the trunk version as user A.

User A can trigger a password-mail to other user B by accessing (simply in accessing Special:PasswordReset as url and inputting username B into the field) thus I think my observation is correct.

Increased bug to high / major

Dur, I was looking at ChangePassword, not PasswordReset.

Update:

no password throttle if user A resets the own password (user A) by mail:
check against password throttle to be skipped if user reset the own password.

Hello, because I do not feel yet competent enough to change the code in such a sensitive area like login and password issues: can someone of you (experts) please look into the following change request, and apply a fix for it ?

The following is an aggregated summary.

  • Problem to be solved:

User A can trigger a password-mail to other user B by accessing (simply by
accessing Special:PasswordReset and inputting username B into the field)

When logged-in users visit Special:PasswordReset,
they see an _emtpy_ input field for entering a username.

The _empty_ field does not make sense, because:

Logged-in users should - except in special cases like members of a new group -
$wgGroupPermissions["sysop"]["isallowed-to-reset-other-user-password"] = true;

not be allowed to trigger reset password of a different user.

  • Change requests (A), (B) in Special:PasswordReset
  • (A)
  • if "user", then PaswortReset should
  • disallow typing of arbitrary usernames
  • populate the Username field with the current users' username
  • this field set readonly=readonly
  • the onSubmit callback must sanitize the return and check wether the

correct and only allowed current users' username comes back

  • no password throttle if user resets the own password by mail:

(skip check against password throttle if user resets the own password.)
-> then mailing the temporary password to user(username)

  • (B)

I also need (for OpenID) a clean way of internally sending directly a temporary
password (not: e-mail confirmation, this is already implemented) to logged-in user (without the form). Such a function could be re-used by change request (A) method.

(In reply to comment #6)

  • Problem to be solved:

User A can trigger a password-mail to other user B by accessing (simply by
accessing Special:PasswordReset and inputting username B into the field)

Is this really a problem? The user could just log out and do the same thing.

When logged-in users visit Special:PasswordReset,
they see an _emtpy_ input field for entering a username.

If it were pre-filled with the user's username, it would be consistent with the current behavior of Special:UserLogin. (I never figured out whether that behavior was intentional or just a quirk.)

(

When logged-in users visit Special:PasswordReset,
they see an _emtpy_ input field for entering a username.

If it were pre-filled with the user's username, it would be consistent with the
current behavior of Special:UserLogin. (I never figured out whether that
behavior was intentional or just a quirk.)

I do no mean the browser cache for text /username/ input fields. I mean the MediaWiki should expressly fill-in the current (logged-in) username, and set it read-only, so that it cannot be changed.

(In reply to comment #8)

(

When logged-in users visit Special:PasswordReset,
they see an _emtpy_ input field for entering a username.

If it were pre-filled with the user's username, it would be consistent with the
current behavior of Special:UserLogin. (I never figured out whether that
behavior was intentional or just a quirk.)

I do no mean the browser cache for text /username/ input fields. I mean the
MediaWiki should expressly fill-in the current (logged-in) username, and set it
read-only, so that it cannot be changed.

I believe MediaWiki _does_ expressly fill in the current username for Special:UserLogin (am I wrong about this?).

In any event, maybe Special:PasswordReset should do the same thing, but I don't see what the benefit of making the field read-only would be (if it's really an issue, it would probably make more sense to just disable the special page altogether for logged-in users).

(In reply to comment #9)

In any event, maybe Special:PasswordReset should do the same thing, but I don't
see what the benefit of making the field read-only would be (if it's really an
issue, it would probably make more sense to just disable the special page
altogether for logged-in users).

Let me explain for a third time:

As mentioned several times above, users who are logged-in by OpenID, do not have a password set in the database. As maintainer of E:OpenID I need to find a clean way, using the existing methods, to allow them to set up a password _after_ having created their accounts by OpenID. This is a fail-sa

(In reply to comment #10)

(In reply to comment #9)

In any event, maybe Special:PasswordReset should do the same thing, but I don't
see what the benefit of making the field read-only would be (if it's really an
issue, it would probably make more sense to just disable the special page
altogether for logged-in users).

Let me explain for a third time:

As mentioned several times above, users who are logged-in by OpenID, do not
have a password set in the database. As maintainer of E:OpenID I need to find a
clean way, using the existing methods, to allow them to set up a password _after_ having created their accounts by OpenID. This is a fail-safe and also required, if they use their wiki account as OpenID for another OpenID-aware system.

A non-sysop User X should not be allowed to trigger password reset for any other User Y. That's the point. But User X should beallowed to trigger passwort rest for exactly User X.

happy.melon.wiki wrote:

In ordinary vanilla MediaWiki, a non-sysop User X cannot be *prevented* from triggering password reset for User Y, because User X can simply log out and become Nobody. Therefore there is no point in restricting access to Special:PasswordReset from logged-in users.

What you are describing is a special situation generated by the use of the OpenID extension. Therefore any changes that you want to make to the behaviour of Special:PasswordReset should be achieved by hooks in the special page and hook functions in the extension. If extra hooks are needed, by all means ask for them to be added. But much of this behaviour doesn't make any sense in a standard MediaWiki environment.

(In reply to comment #12)

In ordinary vanilla MediaWiki, a non-sysop User X cannot be *prevented* from
triggering password reset for User Y, because User X can simply log out and
become Nobody. Therefore there is no point in restricting access to
Special:PasswordReset from logged-in users.

Agreed, as I already said this in the introduction.

What you are describing is a special situation generated by the use of the
OpenID extension.

Yes and no. Go to standard wiki and to Special:PasswordReset and you can trigger PasswordReset of Tim or Brion or Jimbo. This is unwanted. Be careful: your username will be revealed in the password mail they receive, I have tested this.

(Well, as mentioned you can logoff and PasswordReset as anon)

As courtesy to a logged-in user X (yes I know: user==logged-in ) and slight improvement of UI, and security, the only meaningful action is: X may only trigger PasswordReset for X .

Test this live:

I just have sent you a password reset mail. The next 24 hours you are blocked and cannot have a second chance

Goto http://www.translatewiki.net
Login as Happy-melon
Goto http://translatewiki.net/wiki/Special:PasswordReset
See what I mean ? You can send password resets to Brion, Tim, .... me, myself and I.

Just for test purposes, I have sent you a password reset mail on 12:46 UTC. The next 24 hours you are blocked and cannot have a second chance.

This hole is what I want to close.

This hole is what I wished it would be closed by you experts (not by me as newbie)

Tim, Brion: only for test purposes, I have sent you PasswortReset, too. (Will not do this again. It was only for demonstrating, that this should be fixed)

happy.melon.wiki wrote:

(In reply to comment #13)

(In reply to comment #12)

In ordinary vanilla MediaWiki, a non-sysop User X cannot be *prevented* from
triggering password reset for User Y, because User X can simply log out and
become Nobody. Therefore there is no point in restricting access to
Special:PasswordReset from logged-in users.

Agreed, as I already said this in the introduction.

Good.

What you are describing is a special situation generated by the use of the
OpenID extension.

Yes and no. Go to standard wiki and to Special:PasswordReset and you can
trigger PasswordReset of Tim or Brion or Jimbo. This is unwanted.

This is precisely contrary to what you just said. A logged-in user triggering an unwanted password reset is better than an anon triggering it because, as you say, there is better logging.

the only meaningful action is: X may only trigger PasswordReset for X .

This is not meaningful in a standard MW install; if User X goes to trigger PasswordReset, then User X must have previously logged in as User X, so knows his password, so doesn't need to reset it.

This hole is what I want to close.

You cannot "close" it because, as mentioned several times, anyone acting maliciously can simply log out to reopen it. In r86482 I introduced the option for wikis to require an email address for password resets, which 'closes' the vulnerability if wikis are concerned about it. 'Fixing' this for the sake of fixing it is definitely a WONTFIX, IMO. Adding hooks to allow extensions to modify the behaviour, on the other hand, is entirely reasonable.

for wikis to require an email address for password resets, which 'closes' the
vulnerability if wikis are concerned about it. 'Fixing' this for the sake of
fixing it is definitely a WONTFIX, IMO. Adding hooks to allow extensions to
modify the behaviour, on the other hand, is entirely reasonable.

Can you do me a favor, and write such a module for me?

Requirements:
the requested module - when "hooked" - should do this:

if user X goes to Special:PasswordReset:

  • they should sees the own name X,
  • prefilled in the input field, and
  • "read-only",

and the script allows only X as return value. Any other value Y should throw an error.

happy.melon.wiki wrote:

(In reply to comment #17)

for wikis to require an email address for password resets, which 'closes' the
vulnerability if wikis are concerned about it. 'Fixing' this for the sake of
fixing it is definitely a WONTFIX, IMO. Adding hooks to allow extensions to
modify the behaviour, on the other hand, is entirely reasonable.

Can you do me a favor, and write such a module for me?

I can add the hooks for you if you want (in a couple of weeks when I'm free to code again, that is). I'm afraid I don't have much interest in working on functionality which is only included with extensions I don't use.

happy.melon.wiki wrote:

Actually, the hooks you need are already in place: SpecialPasswordResetModifyFormFields is passed the HTMLForm descriptor array; hook a function on to that in the OpenID extension that changes the Username field to type 'info' and sets its value to the current username; and SpecialPasswordResetOnSubmit to audit and reject prohibited requests.

(In reply to comment #20)

when implementing this, check against effects from or to
https://bugzilla.wikimedia.org/show_bug.cgi?id=20185 .

Tested ok, no negative side effects found (bug20185, rev92924):

you still can have the OpenID option "update the following information from OpenID persona every time I log in: E-mail address" set; extension OpenID updates that then without prompting the user for their password.

see also comment https://bugzilla.wikimedia.org/show_bug.cgi?id=27060#c2 .

ashishmittal.mail wrote:

Modification to Special:PasswordReset to preset the username field

Hello,

I have added a patch containing modification to the PasswordReset page to preset the name of logged users in the 'username' field. This would simply set the name for logged-in users and does not contain any modifications based on permissions. Kindly suggest if it is fine or it needs to be improved.

Thanks,
Aashish

Attached:

Thanks fixed in r111287 .

I changed your patch slightly to make it shorter

if( $this->getUser()->isLoggedIn() ) {

				$a['Username']['default'] = $this->getUser()->getName();

Please allow me only two very kind remarks:

Perhaps, if possible, can you always make sure to have the latest version from trunk ("svn up") to which you then apply your patch and make a diff, this makes life even more esaier? Thanks in advance.

Regarding the braces, there are MediaWiki Code conventions you can visit
https://www.mediawiki.org/wiki/Manual:Coding_conventions#General_style
(MediaWiki's indenting style is similar to the so-called "One True Brace Style").

ashishmittal.mail wrote:

Strange, I did perform an 'svn up' to the codebase before creating the patch. Will take care regarding the coding conventions.

Thanks for the commit.

Regards,
Aashish

(In reply to comment #24)

Strange, I did perform an 'svn up' to the codebase before creating the patch.
Will take care regarding the coding conventions.

I only found that the line numbers in your diff were different. Not a big issue. Thanks.

Hopefully the others like the patch, too. I think, it is an improvement of the GUI and does neihter introduce a regression nor a security flaw.