otland / forgottenserver

A free and open-source MMORPG server emulator written in C++
https://otland.net
GNU General Public License v2.0
1.57k stars 1.05k forks source link

Use OpenSSL instead of crypto++ #4655

Closed ranisalt closed 4 months ago

ranisalt commented 4 months ago

Pull Request Prelude

Changes Proposed

Use OpenSSL for SHA1 hashing and RSA encryption, which is more readily available in all systems than crypto++

Codinablack commented 4 months ago

This is a very good call IMO.

Cryptopp dropped the support of CMake, which makes this PR all the more valuable!

nekiro commented 4 months ago

aff, too bad openssl interface is pain

divinity76 commented 4 months ago

I haven't kept up to date with the tibia protocol for many years, but last i checked, RSA is only used for login, and XTEA is used for in-game communication, is this still the case?

(If that's the case, the performance between crypto++ and openssl is a non-factor)

ranisalt commented 4 months ago

I haven't kept up to date with the tibia protocol for many years, but last i checked, RSA is only used for login, and XTEA is used for in-game communication, is this still the case?

(If that's the case, the performance between crypto++ and openssl is a non-factor)

That's still the case. RSA is only for the XTEA key, so 128 bytes once per connection. XTEA is now fast enough to be negligible.

ranisalt commented 4 months ago

Would like a few ✅ especially from those on Windows, since installing and using OpenSSL on Linux is a piece of cake 😆

Also: gonna add some tests

Zbizu commented 4 months ago

I know that no one is probably compiling that way, but getting rid of crypto++ will make it possible to build with cmake on windows again so it's another reason to go with this pr 👍

gesior commented 3 months ago

@ranisalt I've replaced transformToSHA1 with your code (+ https://github.com/otland/forgottenserver/pull/4665 ) in my TFS 1.4 and now I cannot login into game anymore.

transformToSHA1 returns 20 bytes with SHA1 digest (binary format) and then it's compared to SHA1 generated by acc. maker (40 bytes, hex format):

if (transformToSHA1(password) != result->getString("password")) {

Is there missing conversion to hex format after these changes?

I added this function to convert transformToSHA1 result and now it works:

std::string bin2hex(std::string_view binary)
{
    const unsigned char *hash = reinterpret_cast<const unsigned char*>(binary.data());
    char hex[binary.size() * 2 + 1];
    for (int i = 0; i < binary.size(); ++i) {
        sprintf(&hex[i * 2], "%02x", (unsigned int) hash[i]);
    }

    return std::string(hex, binary.size() * 2);
}
EvilHero90 commented 3 months ago

It was basicly resolved in #4669 with using UNHEX on mysql password, but yea the transformToSha1 function now returns binary, can't for sure say why this decision was made but it might be due to changing to a different encryption like SCrypt or Argon2 later on (just a guess)

ranisalt commented 3 months ago

No particular reason to do that, although due to small string optimization the 20-byte string may be faster by avoiding the heap. Not significant, but cool :sunglasses: