Bug 52362 - Restricted validity of the password reset token (sent by mail) broken
Summary: Restricted validity of the password reset token (sent by mail) broken
Status: CLOSED FIXED
Alias: None
Product: UCS
Classification: Unclassified
Component: Self Service
Version: UCS 4.4
Hardware: Other Windows NT
: P5 normal
Target Milestone: UCS 4.4-6-errata
Assignee: Florian Best
QA Contact: Dirk Wiesenthal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-13 16:46 CET by Thorsten
Modified: 2020-11-18 16:44 CET (History)
3 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 3: Will affect average number of installed domains
How will those affected feel about the bug?: 4: A User would return the product
User Pain: 0.343
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): Regression, Security
Customer ID:
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thorsten univentionstaff 2020-11-13 16:46:34 CET
When triggering a password reset by email, the token is much longer valid than the set value in 

umc/self-service/passwordreset/token_validity_period=172800

that should be defined in seconds, afaik.

Instead of 48h a reset link with the token still allowed a password change a week later.

Expected behaviour: Inform the user that the token expired and that the user has to issue a new password reset request.
Comment 1 Erik Damrose univentionstaff 2020-11-13 16:53:17 CET
Please check which version of UCS is in use, we fixed the behavior of UCR  umc/self-service/passwordreset/token_validity_period with UCS 4.4e777 (bug #51287).
Comment 2 Thorsten univentionstaff 2020-11-13 17:41:25 CET
The actual problem is different and was tested after the 4.4e777 patchlevel:

While in the referenced bug #51287 the setting in umc/self-service/passwordreset/token_validity_period was ignored causing the token to be invalid in a default timeframe of 1 or 2 hours the problem now is

that the token is valid much longer that the seconds set in
umc/self-service/passwordreset/token_validity_period
Comment 3 Florian Best univentionstaff 2020-11-13 18:15:15 CET
The code is the following:

 881 »   »   if (datetime.datetime.now() - token_from_db["timestamp"]).seconds >= self.token_validity_period:
 882 »   »   »   # token is correct but expired
 883 »   »   »   MODULE.info("Receive correct but expired token for '{}'.".format(username))
 884 »   »   »   self.db.delete_tokens(token=token, username=username)
 885 »   »   »   raise TokenNotFound()

   55 »   def insert_token(self, username, method, token):
   56 »   »   sql = "INSERT INTO tokens (username, method, timestamp, token) VALUES (%(username)s, %(method)s, %(ts)s, %(token)s);"                                                                                
   57 »   »   data = {"username": username, "method": method, "ts": datetime.datetime.now(), "token": token}

What we see first: We store the local time instead of UTC. We should use datetime.datetime.utcnow().

Second:
>>> (datetime.datetime.utcnow() - datetime.datetime.utcnow()).seconds                                                                                                                                              
86399
>>> n, p = datetime.datetime.utcnow(), datetime.datetime.utcnow()
>>> n-p
datetime.timedelta(-1, 86399, 999990)
>>> p-n
datetime.timedelta(0, 0, 10)
>>> (p-n).seconds
0
>>> (n-p).seconds
86399
>>> (n-n).seconds
0
Comment 4 Florian Best univentionstaff 2020-11-16 23:38:17 CET
The error happens if one sets the value to more than one day (well more specific: if a user comes back one day later) because the timedelta interface groups seconds > 1 day to a different "days" attribute.
The check was wrong, it must use total_seconds().

I changed all timestamps to be stored and evaluated in UTC.
All broken timedelta.seconds check have been corrected to timedelta.total_seconds().

univention-self-service.yaml
169c6c2cbb48 | YAML Bug #52362

univention-self-service (4.0.3-48)
a44831ec46b3 | Bug #52362: fix token validity check if more than one day passed
Comment 5 Dirk Wiesenthal univentionstaff 2020-11-18 14:37:32 CET
OK: Code
OK: YAML