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

Use OpenSSL instead of crypto++ #4655

Merged
merged 5 commits into from
Apr 29, 2024
Merged

Use OpenSSL instead of crypto++ #4655

merged 5 commits into from
Apr 29, 2024

Conversation

ranisalt
Copy link
Member

Pull Request Prelude

Changes Proposed

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

@Codinablack
Copy link
Contributor

This is a very good call IMO.

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

@nekiro
Copy link
Member

nekiro commented Apr 26, 2024

aff, too bad openssl interface is pain

@divinity76
Copy link

divinity76 commented Apr 26, 2024

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)

DSpeichert
DSpeichert previously approved these changes Apr 26, 2024
Copy link
Member

@DSpeichert DSpeichert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me likey.

openssl was already implicitly required by mariadb-dev in most cases

@ranisalt
Copy link
Member Author

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.

Use OpenSSL for SHA1 hashing and RSA encryption, which is more readily
available in all systems than crypto++
@ranisalt ranisalt marked this pull request as ready for review April 27, 2024 04:09
@ranisalt
Copy link
Member Author

ranisalt commented Apr 27, 2024

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
Copy link
Contributor

Zbizu commented Apr 28, 2024

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 👍

auto expectedPrivateExponent =
"5418925373928586701966836677512206800734033866457254764400459929804603458528232352199600575306819650618162351643553279676157387691020051542771655878881953";
BOOST_TEST(actualPrivateExponent == expectedPrivateExponent,
"expected d = " << expectedPrivateExponent << ", got " << actualPrivateExponent);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll remove the checks below this line, only n, e and d are useful for RSA and the rest is just extra information but isn't used to encrypt/decrypt

@EvilHero90 EvilHero90 merged commit f4dfabb into otland:master Apr 29, 2024
19 checks passed
@gesior
Copy link
Contributor

gesior commented May 11, 2024

@ranisalt
I've replaced transformToSHA1 with your code (+ #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
Copy link
Contributor

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 ranisalt deleted the openssl branch May 12, 2024 00:29
@ranisalt
Copy link
Member Author

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 😎

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

Successfully merging this pull request may close these issues.

None yet

9 participants