This is a HORRIBLE way of checking the old password, since this is completely open to a timing side-channel attack.
Please, PLEASE, use a constant time comparison function!

For example, use: https://cryptography.readthedocs.org/en/latest/hazmat/primitives/constant-time/

This is very bad way of storing a password!
Please consider using bcrypt for password storage.
The problem is that the SHA family of functions is created in such a way that they are as fast as possible, which you do NOT want with password storage: you want to make it as hard as possible for an offline attacker to try lots of possibilities to check if they match (dictionary and/or brute-force attack).

How is password recovery working then?

Sounds like a good idea, def :thumbsup: for me

In order to improve the security, we may want to move all the code related to securing a password into 1 single function, allowing us adjust this one method to improve things

Using a setup-wide password "seed" (in the industry often called a "salt") is a very bad idea, as it doesn't deter attackers much: they just have to create a rainbow table for your setup instead of reusing one for all instances.

You should use a per-user salt so that they will have to brute-force every user by him-/herself.

Sorry, I meant "using ONLY a setup-wide password" is bad idea.
Using one in combination with a per-user salt is fine (but doesn't add much if the salt is generated securely).

After discussing with @puiterwijk, it seems that using https://pypi.python.org/pypi/bcrypt/ might solve most/all of the mentioned issues.

https://pypi.python.org/pypi/py-bcrypt is packaged for Fedora and EPEL7, so might be a good start

Change needed? :)

Left over I suppose :)

This will work but will break backward compatibility with all the instances currently deployed.

I see no changes in the requirements, which module is being used here?

We'll need unit-tests for this, especially to make sure the form works properly on checking that the password and confirm_password are indeed always equals

This also means we can drop the PASSWORD_SEED no? Check it (git grep should help) and if so please go ahead :)

Name this new_password perhaps?

As Toshio mentioned, this will fail if non-ASCII characters are inserted.
Please make sure to test that case as well.

(If you need test data: you can copy and paste "当たり")

Actually, it doesn't crash, and should work fine.

Perhaps a bit more sanity check? This is a very broad exception, and can mean a lot of things.

I would suggest to instead change the password into something like $1$ and $2$ so you can actually know which version of password storage is in use.

This will require an alembic change during the upgrade to make sure all current passwords are prefixed with $1$.

add intending to pass pep8.

indenting

When you get in this case, you have both the old style password hash and the unhashed pasword.

This means that you can now update the password to the new hash system!

NOTE: You are still using == for password comparison!
This is NOT constant-time, and as such a side channel data leakage!

I believe same_password expect this name

Or check if password[0] and password[2] are != $ no?

Well, sure, but then you're required to always support the version without version-indicator.
Just porting the current values on upgrade makes the most sense to me, as that would make the code neater.

Why this try-catch? You can just always use the to_unicode function.

Why do you have a separate if for this? Just store an password_version=1 and then after checking the password just below this code, convert it.

Why hash it yet again? bcrypt is slow (that's the exact reason you should use it), so this should be done as little as possible.
If you first check (as suggested before), you don't need this.

This is still not constant time.

Maybe extract away the prefixing and hashing of the password into a helper function, so it's easier to switch to $3$ in the future without having to change it at multiple places?

How about instead doing something like:
_, password_version, password = user_obj.password.split('$')

That way you can also handle possible versions like $2,loadfactor=50$ if we ever want to make the load factor variable.

This error message sounds quite strange.

How about instead doing something like:
_, password_version, password = user_obj.password.split('$')

That way you can also handle possible versions like $2,loadfactor=50$ if we ever want to make the load factor variable.

You may want to limit the split, in case the hash itself ever contains a '$'

Right, that's a good call.
_, password_version, password_hash = user_obj.password.split('$', 2)

pep8 would ask you for spaces around the + but this is a detail

what is version going to be on existing sha512 passwords?

In other words, will version ever gonna == '1'?

• This is longer than 80 chars
• How is this going to work for passwords in existing databases?

Similar to above

• This is longer than 80 chars
• Is version ever going to == '1'?

2 lines?

So if the if above is not true, we still say Password changed?

Probably best to do version == '1': and add an else: that just says "Something is wrong with your account". No uncertainties wanted with security.

missing to_unicode.

missing to_unicode.

As said, probably you want a function generate_hashed_password for the storage and updating functions.
That way, if the default every changes from bcrypt to X, you only need to update one place to generate the new schema instead of digging for all places.

Perhaps it's useful to add test cases, since this introduces some pretty interesting code.

Are we ever going to have a version == 1?

Probably best to do version == '1': and add an else: that just says "Something is wrong with your account". No uncertainties wanted with security.

I was more thinking along the lines of:
if there is no version -> check the current approach (sha512)

This allows to keep the app backward compatible w/o any changes to the DB ( I'm
not entirely sure I like the idea of touching the passwords stored in an alembic
migration)

Typo here: retrieve

Why not comparing here directly?

Should these two method remain in the controller?

you do string comparison using is not? Oo

I wonder if this part, that checks the validity of the old password shouldn't be using the same path/functions as we have upon login.

I was more thinking along the lines of:
if there is no version -> check the current approach (sha512)
This allows to keep the app backward compatible w/o any changes to the DB ( I'm
not entirely sure I like the idea of touching the passwords stored in an alembic
migration)

Well, the problem here is: are you 100% sure that the current approach will never create hashes that start with $X$?
Also, in that case you can't just do "_, version, user_password = user_obj.password.split('$', 2)" since that would crash horribly if there were no $'s in the password field.

Ok since bcrypt may use '$', then alembic migration it is, but then, it's still missing :)

Not comparing here directly because I am checking version and the performing the required conversion , comparing here will lead to one additional if in login and change password! Should I do it here ?

Should we move this code to the back-end as it seems to be redundant with what is in the login process?

Moreoever, this would make testing the code much easier.

We will need unit-tests before we can merge this, so after the alembic migration script this is the next big step/requirement.

Could you split this line? place the dict and the synchronize_session in another line (or two).

Also, no need for the ; :)

Two empty lines between functions

No need for two returns if we store the value in a variable

Just do a global 'return' at the end

Also: What happens if version is neither 1 nor 2? Shouldn't we say something?

Adding a new line?

Is this a 3 or 4 spaces indentation?

really sorry for that been writing JS for a while :rabbit:

How do you flask an error message from the backend library?

err, let's try again: how do you flash an error message? :)

I actually kinda wish this was in the backend as well, we have this line present twice at the moment, having it on the backend would allow having it only once.

And that would allow to do the split in the backend as well which is also something that I like :)

Looking at the code I think we may want to drop this, it's used in only one place and doesn't bring much

You are right ! If we are checking in check_password there is no need of this.

The imports here needs to be adjusted as well

While working on the tests I realized:

a) This method does not enforce being logged in?

b) Why requiring the username in the URL? Are we allowing to change someone else's password?

This is the place where I commented about the fact that user are not required to be logged in and that we can basically change anyone's password.

I think this is a bit vague, since if passsword_checks is True it first run this code and then also what comes after the end of the else block.

It would feel more readable if you just have an "if not password_checks" which aborts, and otherwise just falls through, just like in the login code.

Except for the remark with the if-blocks, this code looks good to me.

How can user_obj be None since we check if line 80?

Didn't use py-bcrypt finally?

I think you also touched this file :)

And this one to :)

Btw, this is meant to be below the routes, I had replaced it there but looks like this got lost.

And here?

Add your name to the author list at the top

Let's adjust the © year to 2015-2016 while at it

Official :thumbsup: for me! Congrats @farhaan

@puiterwijk, are you ok as well?

The error message is incorrect, that's not a password lost error

Ack. :thumbsup:

That's the redundant if check

Only partially.
It should still check for password_checks.

agreed

Please note that this means that if someone requests a password reset for my user, I need to go to my email and reset my password before I can log back in: it won't accept my current password anymore.

This might be a Denial of Service if I can't reach my old email anymore (and was just going to update it), or otherwise just a major annoyance.

On the other side, it means if your password gets public that anyone can log in with it even during the time you're busy resetting it.

Well, I don't like using the email token as "security feature" in this way: it just doesn't make sense. I doubt that if someone accidentally publishes their password (in which case they're hosed anyway), they won't use the lost password feature: they'll use change password.

And again :thumbsup: for me :)