[Login] Change of logic for Password reset to use tokens #2856

ridz1208 · 2017-06-05T16:04:48Z

This pull request Changes the Password reset logic to prevent users from resetting other users' passwords and thus blocking their accounts.

List of changes:

Bugfixes:

missing page titles on every login page
_validate functions never called (all saves went straight to processing)
call to undefined function showPasswordExpiryScreen() in https://github.com/aces/Loris/pull/2608/files#diff-2c13360f65cfe03fa685449b9074176eL371
undid delete of the password expiry page under name 'password change'

Logic changes:

Removal of the password expiry logic
Avoid password reset before email confirmation (main point of this PR) Redmine 8525

Some history

Issue 1: any user can enter any other user's username on the "forgot password" page and reset their password. This will generate a new random password and deactivate the current one. the user's account will not be accessible until they use the password sent to them by email.
- solution: entering the username only generates a token which is emailed to the user's email address. the old password is still active until the token from the email is used to reset the password
Issue 2: mudularization of login logic caused the partial loss of the password expiry logic Publicly accessible module support #2608
- solution: completed removal of password expiry logic. in order to make this PR testable
Issue 3: caused by issue 2 Removal of password_expiry logic affected behaviour of create_user and forgot_password functionality.
- The current usage of create_user is
  1. the user requests an account (without choosing a password)
  2. dcc approves the account and generates a new random password and sends it to user
  3. the password is automatically set to expired
  4. the user uses the emailed password to login and is redirected to the password expiry page
- solution 1: create_user logic should not involve password_expiry. Instead the user should be asked for a password at the moment of requesting an account. once the account is approved, the user will have access to loris...
- The current usage of lost_password is
  1. user clicks on forgot password and is redirected to reset page.
  2. entering the username will replace the user's password by a random generated one and set it to expired
  3. the user is emailed a new password
  4. the user uses the emailed password to login and is redirected to the password expiry page
- solution 2: forgot_password functionality was implemented to use the Token sent to the user in order to allow the user to choose a new password.
Issue 4: caused by issue3 solution 1 the delete_account functionality depends on the user's password never being set-up. In order to be able to delete a user, this user should have never been activated (otherwise foreign keys would fail). one of the only indicator of a user never being active is the absence of a password in the database.
- solution: find another indicator of user's inactivity and use it instead to determine if a user can be deleted.
Issue 5: caused by clean-up by avoiding to try to instantiate users which do not already exist in the database (which triggers multiple notices and warnings) the create_user logic breaks since it needs an empty user object
- solution: check if user is in db, otherwise bypass all database calls but return an empty object reday to be populated.

johnsaigle · 2017-06-05T16:30:59Z

LGTM

johnsaigle · 2017-06-06T05:06:21Z

@ridz1208 this needs phpcs

jstirling91 · 2017-06-22T20:19:22Z

@ridz1208 phpcs this

kongtiaowang · 2017-07-19T18:48:09Z

After I input email, got 500 error.

johnsaigle · 2017-07-19T18:50:46Z

@kongtiaowang just so you know this is going to 17.2!

johnsaigle · 2017-08-15T15:45:16Z

Hey @ridz1208 just got reminded of this after making my own PRs: #3044 and #3042.

I like your solutions better but there should be a discussion on this at some point. Just wanted to add a note here to close the above PRs if we go with your solution.

ridz1208 · 2017-08-15T16:41:48Z

@johnsaigle lets discuss

driusan · 2017-10-02T20:18:05Z

I added both major and minor release tags, because the change itself is minor, but it depends on another PR which is major..

ridz1208 · 2017-11-22T16:15:09Z

rebasing on major for this https://github.com/aces/Loris/pull/3023/files

* Remove unused variable in main.php * Fix PHPMD error

ridz1208 · 2017-11-26T22:03:37Z

modules/login/php/authenticatetoken.class.inc

+
+        //validate token and load password change page
+        $validToken = $DB->pselectRow(
+            'SELECT *, TIMESTAMPDIFF(HOUR,time,CURRENT_TIMESTAMP) as token_age 


@driusan should we expire the tokens ? if so, I need to start using this token_age variable

ridz1208 · 2017-11-26T22:04:53Z

modules/login/php/authenticatetoken.class.inc

+        );
+
+        if (!empty($validToken)) {
+            header("Location: {$baseURL}/login/password-change/?token=$token");


token is passed to the password change page to get the username

@driusan is there a better way of passing the username to the password-change page ?

You could look into using the PHP session, but I don't see any problem with doing it this way.

ridz1208 · 2017-11-26T22:10:54Z

modules/login/php/passwordchange.class.inc

+
+        $this->addHidden('token', $_GET['token']);
+        $this->addHidden('username', $username);
+        $this->form->addFormRule(array(&$this, '_validate'));


@driusan shouldn't this be done automatically on all forms ??

without this line, the _validate function is never called

I think that would make sense, but we have a lot of pages which aren't forms that are extending NDB_Form instead of NDB_Page and I think it should be investigated separately from this PR...

ridz1208 · 2017-11-26T22:11:15Z

modules/login/php/passwordchange.class.inc

+            header("Location: {$baseURL}/");
+        }
+
+        $username = \User::getUsernameFromId($validToken['user_id']);


new function... not sure about it

ridz1208 · 2017-11-26T22:12:58Z

modules/login/php/passwordreset.class.inc

+        // TODO: problem, the insert below generate SQL FK errors because there
+        // in no logged in user to track history. see database.class.inc line 895
+        // user is being set to 'unknown' at this stage
+        $DB->insert(


@driusan need to resolve issue above

The history table shouldn't have any foreign keys. It should be denormalized because it's a history table.. I think the solution is just to remove the constraint.

okai so for now can I just ignore the warnings ?

yeah, I think so.

ridz1208 · 2017-11-26T22:15:15Z

php/libraries/User.class.inc


        $row = $DB->pselectRow($query, array('UID' => $username));

+        if (empty($row)) {


@driusan review carefully

do you foresee issues with logic ?

just the usual "0" issues with empty()..

@driusan I meant with moving around the ordering of the calls in the user constructor. if you expand the code u'll notice that I put this check and moved down a few variable declaration. Is any page relying on fetching an empty user ?

(ie probably create_user)

I thought either you or @xlecours had found one when you looked into it a few weeks ago or another reason, but I can't remember the details? (You're likely right about create_user.) If there are any, hopefully the intergration tests would catch it..

As far as changing the logic goes the only issue I see is that it doesn't really accomplish what the comment says it's trying to, because the early return makes it vulnerable to timing attacks, so a sufficiently advanced hacker could still derive the existence/non-existence of the username.

yeah.. i'll put a little sleep function on it to confuse the Russian/Chinese hackers trying to break into loris...

this kind of avoids like 30 notices/warnings so if there is not functional issue I still like it there

also phishing =/= brute forcing =/= timing attacks but now I'm just being mean

The comment says it's not returning an error so that phishers won't know that the user doesn't exist. They can still figure out whether the user exists by timing the response time with this implementation. Just because it's not an advanced technique for statistically deriving a password doesn't mean it's not a timing attack.

alright, I just wanna clear up a couple things.

whether the user exists or not, the "thank you ..." message is still being displayed

if i have to keep the same amount of work done whether the user exists or not, then we have to figure out a way to do all that work without generating errors and warnings

there is a typo on line 68 where it says "does" instead of "does not" that I just corrected

@johnsaigle, in this case, wouldnt you use a bruteforce technique in order to accomplish a phishing attack? I might be confused about termnology but what I mean is:
Assuming the page returns a message like "this user does not exists" (which it doesnt), someone could write a script that accesses the forgot password page and try out usernames in a brute force manner (intelligent or not all combinations of names/words/wtv) to get information about existing users ?? am I wrong??

Assuming the page returns a message like "this user does not exists" (which it doesnt)

^ this is what I was trying to emphasize.

The user fills out and submits a form. Client-side Javascript updates the message. Then the server goes and does some work of validation and potentially sending an email. However it doesn't need to update the form submission page so timing is not an issue. As long as the page doesn't present a different response based on whether a user exists, and this is done client-side, there's no response to time.

In other words I don't think you need to manually ensure the same amount of work is being done given the approach you've taken here. 🥇

In the case that there does exist a different message for users that don't exist, you would enumerate the existing users in that way i.e. by the string "this user does not exist" in the response body rather than the timing of the response.

(I might be mistaking something because I didn't read all the code for this patch but I was basing my comments on modules/login/test/password_reset.md. Everything looks good there.)

@johnsaigle yeah, I saw that yesterday in the test plan and changed it. if you look at the testplan I updated yesterday it should be corrected now :)

ridz1208 · 2017-11-26T22:25:18Z

@driusan I made a couple commits after my comments so some show as "outdated" but they are not. make sure to read all of them

email tests SQL last changes on old branch password reset nada sooooo many things 2017_05_04_password_change_issue

driusan · 2017-11-27T16:45:01Z

SQL/0000-00-00-schema.sql

+  `time` timestamp DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
+  PRIMARY KEY (`ID`),
+  CONSTRAINT `fk_password_recovery_users_1` FOREIGN KEY (`email`) REFERENCES `users` (`Email`) ON DELETE CASCADE ON UPDATE CASCADE
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;


I think this table adds yet another new naming/capitalization scheme to the schema? We really need to come up with one official way..

But anyways, there's also a foreign key missing on the user_id column and I'm not sure why the time column has the on update trigger. What's it supposed to be the time of?

since Alex, every new PR i've been making or reviewing I'm expecting snake case...

unless I am modifying a table already followin another convention.

So no this follows the same convention i've been using and that I assumed was the convention for new structures

I don't think that's ever been the case.

lol then add it to the list

driusan · 2017-11-27T16:45:41Z

htdocs/main.php

        try {
-            $timePoint  =& TimePoint::singleton($_REQUEST['sessionID']);
-            $argstring .= "filter%5Bm.VisitNo%5D=".$timePoint->getVisitNo()."&";
+            $timePoint = TimePoint::singleton($_REQUEST['sessionID']);


Does this need to change? I'm afraid of conflicts with the NoMainPHP PR if this is unnecessary..

looks like this is coming from ur own commit, not sure why it's popping in my PR at all

I tried a rebase -i its still there

driusan · 2017-11-27T16:47:17Z

modules/login/php/authenticatetoken.class.inc

+        );
+
+        if (!empty($validToken)) {
+            header("Location: {$baseURL}/login/password-change/?token=$token");


You could look into using the PHP session, but I don't see any problem with doing it this way.

driusan · 2017-11-27T16:47:55Z

modules/login/php/authenticatetoken.class.inc

+ * @license    http://www.gnu.org/licenses/gpl-3.0.txt GPLv3
+ * @link       https://www.github.com/aces/Loris/
+ */
+class AuthenticateToken extends \NDB_Form


There's not a form on this page. Why not just extend NDB_Page?

driusan · 2017-11-27T16:49:52Z

modules/login/php/passwordchange.class.inc

+
+        $this->addHidden('token', $_GET['token']);
+        $this->addHidden('username', $username);
+        $this->form->addFormRule(array(&$this, '_validate'));


I think that would make sense, but we have a lot of pages which aren't forms that are extending NDB_Form instead of NDB_Page and I think it should be investigated separately from this PR...

driusan · 2017-11-27T16:54:29Z

modules/login/php/passwordreset.class.inc

+        // TODO: problem, the insert below generate SQL FK errors because there
+        // in no logged in user to track history. see database.class.inc line 895
+        // user is being set to 'unknown' at this stage
+        $DB->insert(


The history table shouldn't have any foreign keys. It should be denormalized because it's a history table.. I think the solution is just to remove the constraint.

driusan · 2017-11-27T16:55:02Z

php/libraries/User.class.inc


        $row = $DB->pselectRow($query, array('UID' => $username));

+        if (empty($row)) {


just the usual "0" issues with empty()..

johnsaigle · 2017-11-28T19:59:20Z

modules/login/php/passwordreset.class.inc

        // Always claim success to phishers.
        $this->tpl_data['success'] = 'Password reset. '
-        . 'You should receive an email within a few minutes.';
+            . 'You should receive an email within a few minutes.';


@driusan @ridz1208 I think everything I said in the other thread is actually wrong given this line.

I thought that a message like this would be displayed via Javascript (aka client-side) which would make timing attacks less of a concern since you're not depending on the server finishing up work to update the front-end. Given this approach, the two of you are right -- timing attacks are a concern.

ridz1208 · 2017-11-28T21:27:18Z

Adding blocked because this PR will be broken down into several more concise changes.

After the PRs are issued this one will be closed

ridz1208 added the Category: Bug PR or issue that aims to report or fix a bug label Jun 5, 2017

ridz1208 added this to the 17.1 milestone Jun 5, 2017

ridz1208 assigned johnsaigle Jun 5, 2017

ridz1208 requested a review from driusan June 5, 2017 16:04

johnsaigle removed their assignment Jun 5, 2017

johnsaigle previously approved these changes Jun 5, 2017

View reviewed changes

johnsaigle assigned ridz1208 Jun 6, 2017

ridz1208 modified the milestones: 17.2, 17.1 Jun 21, 2017

johnsaigle dismissed their stale review via d2c5492 July 17, 2017 18:44

ridz1208 mentioned this pull request Jul 18, 2017

[Request Account] Remove unused code, refactor logic, cleanup #2961

Closed

5 tasks

driusan removed this from the 18.1 milestone Oct 2, 2017

driusan added [branch] major labels Oct 2, 2017

driusan added this to the 19.0 milestone Oct 2, 2017

ridz1208 removed this from the 19.0 milestone Oct 20, 2017

ridz1208 removed the [branch] major label Oct 20, 2017

ridz1208 added this to the 19.1 milestone Oct 20, 2017

ridz1208 changed the base branch from 17.1-dev to minor October 21, 2017 17:23

ridz1208 added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Oct 27, 2017

ridz1208 added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed labels Nov 20, 2017

ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Nov 20, 2017

ridz1208 force-pushed the 2017_05_04_password_change_issue branch from d2c5492 to c8b1ec4 Compare November 21, 2017 19:02

ridz1208 force-pushed the 2017_05_04_password_change_issue branch from c8b1ec4 to 3805af9 Compare November 22, 2017 16:16

ridz1208 changed the base branch from minor to major November 22, 2017 16:16

ridz1208 added [branch] major and removed [branch] minor labels Nov 22, 2017

Remove unused variable in main.php (aces#3303)

b4b77a5

* Remove unused variable in main.php * Fix PHPMD error

ridz1208 commented Nov 26, 2017

View reviewed changes

ridz1208 added State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Nov 26, 2017

first stab

3e06123

email tests SQL last changes on old branch password reset nada sooooo many things 2017_05_04_password_change_issue

ridz1208 force-pushed the 2017_05_04_password_change_issue branch from 4a8d336 to 7d4290f Compare November 27, 2017 01:24

remove password expiry

712fec6

ridz1208 force-pushed the 2017_05_04_password_change_issue branch from 7d4290f to 712fec6 Compare November 27, 2017 01:38

testplans

aac5cfb

driusan reviewed Nov 27, 2017

View reviewed changes

Update User.class.inc

274064a

johnsaigle reviewed Nov 28, 2017

View reviewed changes

ridz1208 added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 28, 2017

ridz1208 closed this Jan 31, 2018

ridz1208 mentioned this pull request Feb 23, 2018

2017 05 04 password change issue ridz1208/Loris#12

Closed


		$row = $DB->pselectRow($query, array('UID' => $username));

		if (empty($row)) {

[Login] Change of logic for Password reset to use tokens #2856

[Login] Change of logic for Password reset to use tokens #2856

Uh oh!

Conversation

ridz1208 commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List of changes:

Bugfixes:

Logic changes:

Some history

Uh oh!

johnsaigle commented Jun 5, 2017

Uh oh!

johnsaigle commented Jun 6, 2017

Uh oh!

jstirling91 commented Jun 22, 2017

Uh oh!

kongtiaowang commented Jul 19, 2017

Uh oh!

johnsaigle commented Jul 19, 2017

Uh oh!

johnsaigle commented Aug 15, 2017

Uh oh!

ridz1208 commented Aug 15, 2017

Uh oh!

driusan commented Oct 2, 2017

Uh oh!

ridz1208 commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ridz1208 Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsaigle Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ridz1208 commented Nov 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ridz1208 commented Jun 5, 2017 •

edited

Loading

ridz1208 Nov 28, 2017 •

edited

Loading

johnsaigle Nov 28, 2017 •

edited

Loading

ridz1208 Nov 27, 2017 •

edited

Loading

ridz1208 Nov 27, 2017 •

edited

Loading