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

Insecure Password Storage #240

Open
TheRook opened this issue Jun 1, 2013 · 7 comments
Open

Insecure Password Storage #240

TheRook opened this issue Jun 1, 2013 · 7 comments

Comments

@TheRook
Copy link

TheRook commented Jun 1, 2013

SHA512 is not an encryption algorithm, it is a cryptographic hash function. This hash function is not suitable for passwords because it is a very light functional that can be processed at high speed with an FPGA or GPU.

Do to this misuse of SHA512 gae-boilerplate is vulnerable to CWE-916: Use of Password Hash With Insufficient Computational Effort (http://cwe.mitre.org/data/definitions/916.html)

bcrypt, scrypt or SHA512+bpkdf2 would be suitable for password storage. For more information on bcrypt:
http://security.stackexchange.com/questions/4781/do-any-security-experts-recommend-bcrypt-for-password-storage

Further more, you cannot use the same salt value for every password self.app.config.get('salt'). Early salting techniques where used to prevent two individuals with the same password form producing the same password hash.

More information on hashing passwords:
http://security.stackexchange.com/questions/211/how-to-securely-hash-passwords

@TheRook
Copy link
Author

TheRook commented Jun 1, 2013

As a cryptographer the use of AES in the hashing() method bothers me. This is not a commonly accepted method of password storage.

The use of an initialization vector (IV) in the hashing() method is incorrect. The point of an IV is to defend against a chosen plaintext attack, therefore it cannot be derived from the plaintext that you are encrypting:
http://stackoverflow.com/questions/3008139/why-is-using-a-non-random-iv-with-cbc-mode-a-vulnerability

@sbucek
Copy link

sbucek commented Jun 17, 2013

Hi TheRook,
I've found the following custom user authentication implementation:

http://blog.abahgat.com/2013/01/07/user-authentication-with-webapp2-on-google-app-engine/

Code: https://github.com/abahgat/webapp2-user-accounts

Do you think it is more secure than gae-boilerplates implementation?
Sorry, I'm just a noob, always learning... :)

Thank you for your answer,
Simon

@TheRook
Copy link
Author

TheRook commented Jun 17, 2013

webapp2_extras.security.generate_password_hash(raw_password, length=12) will generate a salt of length 12 for each password and append it to the resulting hash. Which isn't bad. However, generate_password_hash() defaults to sha1... which isn't good at all. (And I've filed a but report with them over this).

Storing a global salt in a text file is kind of like having a pepper (http://security.stackexchange.com/questions/21263/how-to-apply-a-pepper-correctly-to-bcrypt/21264#21264). However, each password still needs a unique salt.

The solution is pretty simple. Use a python implementation of bcrypt, generate a nonce for each password as a salt. For bonus points use a nonce stored in a textfile to calculate an HMAC and use this as a pepper.

@mcvendrell
Copy link

Hi, TheRook.

Would be a code similar to this a valid solution?

import bcrypt

# Create a hashed password based on a username+password
def hash_password(username, password, salt=None):
    if not salt:
        salt = bcrypt.gensalt(2)
    pw_hashed = bcrypt.hashpw(username + password, salt)
    return '%s|%s' % (pw_hashed, salt)


# Check if a clear password matches with the hashed stored
def validate_password(username, password, hash):
    salt = hash.split('|')[1]
    return hash == hash_password(username, password, salt)

I'm actually storing as the password for users the return of hash_password function.

@TheRook
Copy link
Author

TheRook commented Jul 16, 2013

Yeah that looks really close. Make the salt 16 bytes in size. 2 is too
small.

-Mike

On Thu, Jul 11, 2013 at 8:23 AM, mcvendrell [email protected]:

Hi, TheRook.

Would be a code similar to this a valid solution?

import bcrypt

Create a hashed password based on a username+password

def hash_password(username, password, salt=None):
if not salt:
salt = bcrypt.gensalt(2)
pw_hashed = bcrypt.hashpw(username + password, salt)
return '%s|%s' % (pw_hashed, salt)

Check if a clear password matches with the hashed stored

def validate_password(username, password, hash):
salt = hash.split('|')[1]
return hash == hash_password(username, password, salt)

I'm actually storing as the password for users the return of hash_password
function.


Reply to this email directly or view it on GitHubhttps://github.com//issues/240#issuecomment-20819319
.

@mcvendrell
Copy link

Great.

I've tested it with 12 previously (in my local dev machine, old dual core) but the process was very slow. E.g. with 12, the login page was 8 secs to respond. Then I changed it to 2 and all was immediate.
I've never tested it on the production, so I can't give feedback. Will test, but if it's also too slow I think will be better to compromise a little the size vs the speed, because a user can't be waiting 8 secs for a single page.

@alejcas
Copy link

alejcas commented Apr 29, 2014

Unfortunately you can't neither should user bcrypt in app engine. As long as app engine only accepts pure python, bcrypt is too slow.

It's ok to used in other project outside gae beacuse then you can use the c version which is faster.

My bet here is to use sha512 instead and salt it (which is donde by default).
If you are not going to use it for really high security purposes there is no problem with that.

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

No branches or pull requests

4 participants