Insecure hashing algorithm

[nmalcolm]

Hi,

piWallet uses an insecure hashing algorithm to store user passwords, plain MD5. It's weakened by the fact strip_tags() is being run on the plaintext password (why?).

$password = md5(addslashes(strip_tags($password)));

Please consider switching to bcrypt.

https://paragonie.com/blog/2016/02/how-safely-store-password-in-2016

[Martinoc26]

Yup, that's very important

[jfm-so]

*Deleted duplicate comment from @Martinoc26

This really isn't "important", as in a vital security flaw. The importance of not storing in MD5 would be in case of a database breach in mySQL, md5 is easy enough to reverse engineer meaning that users could be compromised on other sites that they reuse the same password.

However, it's not a security "hole" inside of piWallet, just an enhancement, and labeled as such.

[jfm-so]

Didn't mean to close, this will be an enhancement fix in the future.

[ryan-shaw]

@johnathanmartin I think it's quite naive for you to say it's not a security flaw, as this is exactly what it is. md5 is a significantly quicker (along with it's cryptographic flaws) hashing algorithm, so if any DB is leaked you can guarantee all passwords will be cracked, even ones that are reasonably strong, now you have just leaked previously unknown passwords into the wild.

Further to that there is no rate limiting on the login, which means I can just continually attack the login with different passwords, because md5 is very quick compared to proper password hashing algorithm such as bcrypt, I could go through the passwords as fast as the server can hash them.

There's a reason md5 is used for checksum and not password hashing! Switch it, now.

[jfm-so]

Feel free to contribute, Ryan.

[ryan-shaw]

The whole thing needs rewriting, and you can’t seem to acknowledge any issues or criticism with the project, I will certainly avoid contributing.

On Mon, 26 Mar 2018 at 03:53, Johnathan Martin notifications@github.com wrote:

Feel free to contribute, Ryan.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/johnathanmartin/piWallet/issues/36#issuecomment-376024556, or mute the thread https://github.com/notifications/unsubscribe-auth/ACcIQO6aux0NlRDd7Xa5aZFw0bDahbOPks5tiEoQgaJpZM4RPA8t .

[jfm-so]

My private key for my personal bitcoin address in md5 is 2b74909b948d844fcb8a02d40a27b778, go nuts.

[nmalcolm]

@johnathanmartin Your logic is flawed. It isn't going to be possible to crack that hash due to the length and uniqueness of the input. Now, if everyone was using long, random, unique passwords, it wouldn't matter. Alas, that is not the world we live in and therefore we have to design systems to protect the users. A password, on the other hand, without any restrictions in place, can be as simple and short as "1". Even with the following restrictions in place: Must contain a capital letter, a number, and be at least 6 characters long, it is still not enough to be using plain MD5. MD5 is a fast hashing algorithm. It's trivial to compute billions of MD5 hashes every second. With a database dump it would be incredibly easy to crack the passwords of piWallet users. Because you're using plain MD5 with no salt, it makes it even easier to crack those passwords. You don't need to crack every single hash -- if two users are using the same password, they'll have the same password hash. It's difficult to argue that's any more secure than storing user passwords in plaintext. But even with salts, MD5 hashes can be cracked with ease.

bcrypt is slow. It's secure. It's hard to crack. It comes with salts built in. It's widely available. It shows you know the importance of hashing passwords and know how to hash them securely.

MD5 is not the answer and this is not a hill you want to die on.

[nzall]

@johnathanmartin MD5 is not good. https://gist.github.com/epixoip/ace60d09981be09544fdd35005051505 proves that. anyone with a bit of money can calculate 300 BILLION MD5 hashes per second, and that will only increase as time goes on. And they don't need to find YOUR password, just one with the same hash. Are you willing to bet the contents of not just YOUR wallet, but that of the wallet of anyone that uses your software, on whether they can't bruteforce your password?

[ryan-shaw]

I think "secure" should be removed from "piWallet is a popular secure opensource online altcoin wallet' in README.md

[DominoTree]

@johnathanmartin I hate to say it, but this sort of arrogance is why so many Bitcoin exchanges and otherwise have been compromised, and continue to be compromised. You would do well to heed the advice of people who have been around far longer than you. And perhaps, learn a bit about hashing algorithms if you're writing software for cryptocurrencies.

This repository has a TON of blatant security flaws. Learn to use an ORM and build queries without concatenating untrusted input into strings. There's SQLi all over the place here as well as many other issues, and it'd be trivial for anyone who looks at this for 5 minutes to dump the database from any instance they can find.

Even better, use a framework that provides its own secure auth, and ORM mechanisms like Laravel.

https://github.com/johnathanmartin/piWallet/blob/master/classes/User.php#L235

It's negligent for this to even be online.

[tuxxy]

Just wanted to pop in here to say lol, so... lol

[DominoTree]

Holy shit there's a Google search that shows all of the public instances - https://www.google.com/search?q=%22Powered+by+piWallet%22+%22...or+create+a+new+account%22

This is really really really really really bad.

[jfm-so]

If anyone can find a specific security flaw, rather than a theoretical vulnerability which requires conditions x, y and z. I’d really appreciate that.

[ryan-shaw]

@johnathanmartin take a look at https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string and you will see you have flaws everywhere. These aren't theoretical at all.

[jfm-so]

“For Very OBSCURE EDGE CASES!!!”

Quote from link

[tuxxy]

"It's not vulnerable, you're not using it correctly."

[DominoTree]

Okay, if you create a user and enable/disable Google Authenticator, they can manipulate their session data to perform SQL injection and return a dump of the entire database or manipulate it otherwise.

https://github.com/johnathanmartin/piWallet/blob/master/classes/User.php#L235

If they changed the value of $_SESSION['user_id']; to <id>; UPDATE users SET admin=1 WHERE id=<id>; for example, they could gain administrator access to the site.

[jfm-so]

If they changed the value of user_id they could change it to the admins user ID. How would they go about doing that?

[martinsirbe]

👀

[DominoTree]

@johnathanmartin By opening the Inspector in any browser written after 2012 and changing the value

[DominoTree]

You can literally double-click on session values and change them

[DominoTree]

Are you kidding me right now

[ryan-shaw]

@DominoTree bad example, you can't change values in $_SESSION from client side.

[DominoTree]

@ryan-shaw I will admit that it's been a very long time since I've done web-dev and you may be right. Still easy enough to enumerate all of the valid session tokens here.

[nmalcolm]

If anyone can find a specific security flaw, rather than a theoretical vulnerability which requires conditions x, y and z. I’d really appreciate that.

Okay, let's start again. You're running strip_tags() on the input password which has absolutely no purpose. Should a user choose a password such as <secretpassword>, their password will be an empty string and anyone can log into their account without a password.

test.php:

<?php

function hashp($password) {
    return md5(addslashes(strip_tags($password)));
}

echo hashp('<secretpassword>') . " | " . hashp(null);

Output:

d41d8cd98f00b204e9800998ecf8427e | d41d8cd98f00b204e9800998ecf8427e

[daverogers]

https://github.com/johnathanmartin/piWallet/blob/a474b34b05723ba734153726791fc476a043afcf/users.sql#L47-L48 fwiw "changeme" is a terrible password, even if you're asking the installer to change it for production.

there's good reason you should use packages to handle most of this stuff, even if you're not going to use a framework like Laravel or Slim. you don't know what you don't know, which is evident throughout this project. even small things like abiding PSR standards make your project easier for the OSS community to actually contribute to, proper database design, data binding, etc. you don't want to be the guy everybody looks at when an important system gets hacked and money/data are lost just because you're too annoyed to deal with other developers nitpicking "obscure edge cases" (that are community standards by now, might I add; no project >= PHP 5.3 should use md5, strip_slashes, etc. for any security feature). enjoy learning and building your own projects for fun and self-education, but i'd caution against taking on paying customers, or any customers that aren't aware of how risky this code is and/or how inexperienced you appear to be.

[yackermann]

NIST SP800 63B https://pages.nist.gov/800-63-3/sp800-63b.html

Verifiers SHALL store memorized secrets in a form that is resistant to offline attacks. Memorized secrets SHALL be salted and hashed using a suitable one-way key derivation function. Key derivation functions take a password, a salt, and a cost factor as inputs then generate a password hash. Their purpose is to make each password guessing trial by an attacker who has obtained a password hash file expensive and therefore the cost of a guessing attack high or prohibitive. Examples of suitable key derivation functions include Password-based Key Derivation Function 2 (PBKDF2) [SP 800-132] and Balloon [BALLOON]. A memory-hard function SHOULD be used because it increases the cost of an attack. The key derivation function SHALL use an approved one-way function such as Keyed Hash Message Authentication Code (HMAC) [FIPS 198-1], any approved hash function in SP 800-107, Secure Hash Algorithm 3 (SHA-3) [FIPS 202], CMAC [SP 800-38B] or Keccak Message Authentication Code (KMAC), Customizable SHAKE (cSHAKE), or ParallelHash [SP 800-185]. The chosen output length of the key derivation function SHOULD be the same as the length of the underlying one-way function output.

https://latacora.singles/2018/04/03/cryptographic-right-answers.html

Latacora, 2018: In order of preference, use scrypt, argon2, bcrypt, and then if nothing else is available PBKDF2.

DONT USE MD5 FFS

https://security.stackexchange.com/questions/19906/is-md5-considered-insecure

[yackermann]

@johnathanmartin Maybe if you are Emerging Entrepreneur, Seasoned Web Developer you should listen to the advice of field experts, cause you Entrepreneurship will finish with major data breach and death of your future company, cause "I don't store any important info, so MD5 is fine"

[ryan-shaw]

Have you also noticed he's recommending to login as root everywhere? Server and MySQL database!

[ghost]

https://github.com/johnathanmartin/piWallet/pull/80

[foxt]

[fleyerJ]

No shit. He put tons of effort into this without much gain and instead of thanking him, you're bitching and moaning. If you think it's a problem, you can fix it. Why would he waste his time fixing it if he doesn't think it's a problem?

[nmalcolm]

No shit. He put tons of effort into this without much gain and instead of thanking him, you're bitching and moaning. If you think it's a problem, you can fix it. Why would he waste his time fixing it if he doesn't think it's a problem?

I dedicated many years of my life to open source software without any gain, but I never downplayed security issues. It doesn't matter what he thinks, it's not a matter of opinion. Sure, people can submit pull requests to fix these issues, but if the project author won't accept them because he's not willing to admit he's made poor design decisions, what's the point?

This isn't about who's right. This is about what's right.

[jfm-so]

Deleted comment that posted links of sites still using default password. This is an open issue and will be resolved.