Hercules
Hercules copied to clipboard
HerculesWS
New PBKDF2 Authentication System
Hello.
I've been talking with some guys on irc, and I decided on implementing a sane and secure password authentication system.
What is the problem with the current authentication system? The methods of authenticating are all ridiculously easy to break / are already broken.
The current problems
First, we have the plaintext storage of passwords. I have nothing to comment, I think we should respect our clients enough to not store their passwords on our database. Second, we have the MD5 storage of passwords. Well, this is pretty easy to break with a rainbow table, so it doesn't offer anything else apart from a fake notion of protection. Third, we have the passwordencrypt client authentication method, which asks for a salt to the server and sends the password encrypted with MD5 together with the hash. I couldn't make this work on recent clients, and this forces the storage of unencrypted passwords on the database, what I already said is terrible.
Do I need to change anything to make my server work with it?
My implementation is COMPLETELY backwards compatible. No tutorial will have to be changed, and no conversors will have to be run.
Inf act, my implementation uses all state-of-the-art technologies and works seamlessly: you just update Hercules, the MySQL database, and you're good to go. You don't need to change or execute a single line of anything (except with use_md5_passwords, where you need to set old_md5_passwords to yes and that's it).
How does it work?
Three new columns were added to the login table: auth_hash, auth_salt, auth_iter_count. These three are responsible for the new authentication using PBKDF2 with HMAC-SHA512 and a salt of 64 bytes generated with a cryptographically strong pseudo-random algorithm.
When a new connection is made, the emulator first looks for the old user_pass column. If it is empty, this means that the account already uses the new authentication system, so it just authenticates using PBKDF2.
If user_pass is not empty, Hercules first looks if old_md5_passwords is defined (the same as use_md5_passwords, but renamed to avoid confusion) and authenticates using md5 if it is. If not, it authenticates using the plaintext password stored in the DB. After that, a conversion is made from the legacy authentication system (either plaintext or md5) to the new system. Then, the user_pass column is cleaned.
I've already tested the system using plaintext and md5, and everything is working fine. I didn't test the code with passwordencrypt (when the client sends the password already hashed with MD5 together with a salt) because I couldn't get a client to use it, does any modern client supports it or is it broken? Even so, the code that makes the authentication is still there, it should work normally.
When designing the code, I've restricted any access to the plaintext password to the maximum. The passwords are not printed or written to the logs anymore, there is no global access to the passwords (they are only accessible as local variables in only a handful of functions). I've followed these guidelines when designing the system: https://crackstation.net/hashing-security.htm
New Dependencies
As I believe no one should make their own cryptographic function implementation, there is a new dependency on OpenSSL's libcrypto. All Visual Studio projects have been updated and the makefile has been also updated to require libcrypto. It is a fast and secure library, so I believe this is a good decision.
Drawbacks
All systems that use authentication will have to rewrite the authentication code to work. I am already working at FluxCP to make it work with the new authentication system.
Conclusion
This is kind of a big update, but the extra security added is very important and present in any serious project nowadays. This addition will be good for Hercules.
This change is 
Well, travis does not seem to find OpenSSL`s crypto library... Strange, it works on my Windows 7, on my OS X yosemite and on my Linux (archlinux).
Very nice work! I'll start with a preliminary review. I'll add general comments here, and line comments for specific things.
About the passwordencrypt/passwordencrypt2 authentication method, I'm just speculating, but it might be only (reliably) supported on old clients that still had a built-in login screen (and didn't support the -t:token login option from the command line)
So the passwordencrypt authentication can be safely removed or should I let it there just for the sake of these old clients?
Any chance to make this optional, for those who, knowing the risks they run into, still do not wish to use stronger encryption? (or, any reason not to offer that?)
Okay, so it will stay. Do you know of any old client that works with it so that I can test if passwordencrypt is still working?
Looking at my old folders, I believe 2011-10-25aRagexeRE supports it. It's a client I used to use, and I recall it having the old login screen, unless I'm mixing things up.
Well, I think security is mandatory. Why would you not want to use stronger encryption? To be able to see the user's passwords and login into ther gmail accounts? I do not think we should have that right. This is mandatory in any serious software that stores passwords.
Moreover, if this is not mandatory, chances are that a lot of people will just disable it because they find it less troublesome. People do not like change.
I'll try using it to test passwordencrypt. You don't, by any chance, remember the diff option, do you?
If/when you have time, could you come to the Hercules IRC? There are some concerns unrelated to code review I'd like to discuss
It is added by @CRYPTO_LIBS@ in the src/login/Makefile.in
It is adding -lssl also, but that can be fixed.
I also made the changes you talked about and fixed the tabs.
The Xcode build script doesn't make use of Makefiles though. Linker flags need to be manually added there. (on a side note, we should perhaps consider switching to CMake, so that we only have to maintain one build system, and it generates the rest -- but not now)
I actually wrote the CMake build system for brAthena, I have a lot of experience with it. But someone removed it, I don't know who.
And yes, I don't have any experience with XCode, so it probably isn't working. In fact I had a problem with OS X, because the OpenSSL apple distributes is a very (VERY) outdated version that doesn't support PBKDF2. The solution was to install a new version with homebrew and point the configure script to there.
In XCode a similar measure will have to be done.
About OS X: yes, OpenSSL is deprecated in favor of CommonCrypto. Best option (for cross-platform compatibility) seems to be a self-installed OpenSSL.
What do you mean? Our makefile compiling OpenSSL or distributing a binary for OS X as we do with Windows?
Instead of changing use_md5_passwords to old_md5_passwords, would be better just add a comment like: #DONOTUSE :) 'Cause we have the conf import folder, and it's not a good idea to "break" a conf name.
btw, good job.
Ohh yup, that can cause many problems, of some users opening loads of post about the error saying use_md5_pass not found and blabla.
Yeah, I'll rollback that, it wasn't a very bright idea to change the configuration name.
@RenatoUtsch Anything else remaining in this branch? if not, maybe @MishimaHaruna @shennetsind , could you chime in to review it?
I talked to Haru and I am to implement some kind of DDOS protection on the login before this being accepted, because now login takes more processing time.
I am without much time lately, but I'll see if I can do it by next week.
Yep, the rest of the patch is already reviewed, and ready to merge. For the time being, I'm assigning this PR to myself so that it won't get accidentally merged before we have a better DoS protection.
I am kind of very busy at my university at the moment (since my last comment hehe), this semester is proving more difficult than I expected.
When I get some free time I'll dedicate myself to this and see if I can find a solution to the DoS problem.