Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security issue with only generating link using only signed user pk #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dat-boris
Copy link

Note this is only a mitigation fix

Scenario

If we only generate the reset link using expiry link and max age, with in the expiry period, anybody with view of the reset link can gain access to the user account.

e.g. if I have access to the browser history since it is a public computer within the expiry period, i can access the reset link and gain access to the account.

I have proposed an example fix with signing with also the password hash. However this fix is just a mitigation. If user reset password with the same password, then this will still be an issue.

Safety in signing the password

Also note that we have hashed the password to be signed, in case if user didnt encrypt their password to prevent accidental leakage.

Help

Do you have any suggestions for a better fix?

  • ensuring the reset password is different than the old one - this changes our behaviour and might require extra options
  • store the state of the invitation link - which unfortunately defeat the purpose of the elegant token generation solution

@dat-boris dat-boris changed the title 💥 Security issue with only generating link using user pk signed with secret Security issue with only generating link using user pk signed with secret Apr 15, 2015
@dat-boris dat-boris changed the title Security issue with only generating link using user pk signed with secret Security issue with only generating link using only signed user pk Apr 15, 2015
@dat-boris
Copy link
Author

As mentioned in #31 shall leave the docs failure to you - thanks!

@brutasse
Copy link
Owner

Using the password hash is fine even is the password is set to the previous one -- the password salt changes which invalidates old URLs.

What we could do is generate URLs in the form <user_id>-<token> where token is the signed value of a hash calculated using a combination of user ID, password hash and if the user model contain this information, the "last login" timestamp.

@dat-boris
Copy link
Author

Great idea on using the last_login information! In that case the link will be immediately be invalidated as soon as user login. +1

Interested in the rational in wanting to expose the user_id information in the URL - if it is already calculated in the token?

@brutasse
Copy link
Owner

We need to expose user_id because the token would be a hash (1-way) instead of a signed value (2-way) like it is now. Verifying a token would mean 1) fetch the user from db, 2) compute hash, 3) check against URL-provided hash.

@dat-boris
Copy link
Author

Ah, I see. I guess you were talking about for the url generated should be:

{userid}-signed(hash(userid,password,last_login))

while i meant to say

signed(userid, hash(password, last_login))

both of with we can extract the user-id (and so does the attacker, if he really want to know the user-id that much, he can break HMAC via controlled plain text attack)

Though I have a slight preference to 2 since it does raise the barrier for exposing information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants