The current way of converting the S key from a bigint to the value used for the interleaved hash fails on some occasions.
I believe that the error comes from the endianness conversion from bigint to a dynamic vector here in encode_flip which then leaves the hash here with a [leading big endian][trailing little endian] zero removed.

The fault was reproduced by connecting to the server until the server gave an incorrect proof for a known-correct username/password combination. The fault is relatively common, and appears somewhere between once every 100 or 200 times. It can reliably be reproduced just by continuously connecting.

All arrays are little endian and strings are big endian.

The issue can be isolated with the following values acquired from authentication with a 1.12.1 client:

S key before splitting (big endian): "00DBAC635E0DCCB91C5A38B6C2DA7F7685D2D5B0798298C6950A58B85BA07523"
S Key before splitting (little endian): [35, 117, 160, 91, 184, 88, 10, 149, 198, 152, 130, 121, 176, 213, 210, 133, 118, 127, 218, 194, 182, 56, 90, 28, 185, 204, 13, 94, 99, 172, 219, 0]

We do not know which S key the client ends up with, but we can deduce it by calculating the client proof which we know to be correct.

The correct client proof is

Correct client proof: [144, 246, 210, 216, 211, 95, 135, 31, 55, 208, 232, 221, 1, 6, 124, 251, 17, 35, 169, 180] 

While the incorrect client proof generated is

Wrong client proof: [4, 38, 71, 65, 60, 160, 68, 159, 49, 155, 248, 21, 78, 86, 26, 77, 233, 149, 103, 189]

The following values are also required for calculating the proof from the new S key:

username = "A"
password = "A"
Server PrivateKey: [125, 20, 230, 193, 202, 236, 177, 195, 241, 145, 219, 150, 25, 222, 202, 24, 37, 57, 85, 29, 134, 202, 200, 77, 144, 247, 93, 121, 152, 205, 229, 67]
Server PublicKey: [50, 152, 119, 101, 148, 100, 7, 182, 75, 215, 93, 67, 157, 44, 189, 68, 86, 99, 225, 227, 214, 238, 191, 133, 239, 234, 221, 52, 114, 121, 128, 56]
Client PublicKey: [145, 132, 93, 59, 255, 142, 18, 191, 100, 221, 218, 235, 60, 179, 186, 108, 100, 173, 134, 135, 52, 90, 101, 9, 95, 228, 63, 252, 252, 46, 90, 31]
Salt: [206, 208, 252, 32, 25, 77, 89, 249, 236, 80, 232, 50, 74, 86, 58, 117, 101, 83, 52, 20, 55, 227, 40, 174, 40, 103, 157, 70, 25, 218, 250, 38]

For my Rust implementation the following leads to identical behavior as Ember, and a highlighted fix:

pub fn to_equal_slice(&self) -> &[u8] {
	// 32 byte little endian array
	// We return a reference to avoid copying
    let mut s = &self.key[..];

	// This is the part that Ember does implicitly when
	// switching from BigInt to vector
	// Removing it fixes the issue
    if *s.last().unwrap() == 0 {
        s = &s[..s.len() - 1]
    }

	// Normal operation done by Ember in interleaved_hash
	// Quite possibly this should be a `while` instead
	// in case there are more zero pairs deeper in,
	// but I do not have the data to support this
    if *s.first().unwrap() == 0 {
        s = &s[1..];
    }
    if s.len() % 2 != 0 {
        s = &s[1..];
    }

    return s;
}

The following values are included for completeness:

Incorrect S Key: [117, 160, 91, 184, 88, 10, 149, 198, 152, 130, 121, 176, 213, 210, 133, 118, 127, 218, 194, 182, 56, 90, 28, 185, 204, 13, 94, 99, 172, 219], len: 30

Incorrect Session key: "6F1D481B40819CA537BF53DC20430B08A0E4912E0B47D5F70AE40A9F76D31ABBE75861D34EA8C177"
Incorrect SessionKey { key: [119, 193, 168, 78, 211, 97, 88, 231, 187, 26, 211, 118, 159, 10, 228, 10, 247, 213, 71, 11, 46, 145, 228, 160, 8, 11, 67, 32, 220, 83, 191, 55, 165, 156, 129, 64, 27, 72, 29, 111] }

Password verifier: [
        136, 48, 171, 245, 199, 21, 32, 98, 20, 183, 234, 9, 180, 214, 10, 57, 53, 164, 187, 5, 99,
        27, 152, 218, 5, 140, 194, 191, 254, 195, 182, 26,
    ];

Could allow for the DBC memory definition fields to be rearranged for more optimal memory usage. Not particularly/at all important though!

When converting Botan::BigInts back to bytes, the leading zeroes present in the original value are not preserved. This leads the compute_x function to mistakenly use a 31 byte salt value instead of a 32 byte one.

From experimentation, the client appears to use leading zeroes for the computation of M1, meaning that users given a salt with leading zeroes will not be able to log in.
It does not appear possible to inform the client that the salt is only 31 bytes instead of 32, which means that it will always use any potential missing zeroes.

The following tests shows the problem:

TEST(srp6regression, Test) {
	//
    std::string username = "USERNAME123";
    std::string password = "PASSWORD123";

    // Should be a 32 byte value
    Botan::BigInt salt_leading_zeroes(std::string("0x00DECCE60EE2BE6BA4DC6FEDB99E66FFEBDE360F0BE2CEFA984E4CA3E5402CA5"));
    // Should be a 31 byte value
    Botan::BigInt salt_no_leading_zeroes(std::string("0xDECCE60EE2BE6BA4DC6FEDB99E66FFEBDE360F0BE2CEFA984E4CA3E5402CA5"));

    Botan::BigInt x_leading = srp::detail::compute_x(username, password, salt_leading_zeroes, srp::Compliance::GAME);
    Botan::BigInt x_no_leading = srp::detail::compute_x(username, password, salt_no_leading_zeroes, srp::Compliance::GAME);

    // Passes, both are 31 byte values
    ASSERT_EQ(x_leading, x_no_leading);
}

From experience, changing the following in compute_x seems to be consistent with the client:

if(mode == Compliance::RFC5054) {
	hasher->update(Botan::BigInt::encode(salt));
} else {
	const auto& salt_enc = detail::encode_flip(salt);
	hasher->update(salt_enc.data(), salt_enc.size());
}

to

if(mode == Compliance::RFC5054) {
	hasher->update(Botan::BigInt::encode(salt));
} else {
	// Probably make the magic 32 derived from something else
	const auto& salt_enc = detail::encode_flip_1363(salt, 32);
	hasher->update(salt_enc.data(), salt_enc.size());
}

No considerations to endianness are made in the current loader. Fix this!

When database connections are removed from the pool, the metrics reporting outputs the previous value.

The monitoring alerts still report the correct value.

When converting the S key to the interleaved_hash, the client implementation does not pad with zeros, leading to an incorrect session key.

This issue has similarly been reported for the server and fixed, but the client still does it incorrectly.

Changing encode_flip to encode_flip_1363 fixes this for me.

I'm in the process of writing my own emulator and have been using Ember in order to verify operations.
As part of this I've used your SRP6 tests/functions in order to track down issues.

I noticed that the compute_x method here does not use the passed salt paramter, despite RFC2945 defining x as including the salt. Is this an oversight or does the game version not use the salt for x?

Botan::BigInt compute_x(const std::string& identifier, const std::string& password,
                        const Botan::BigInt& salt, Compliance mode) {
	//RFC2945 defines x = H(s | H ( I | ":" | p) )
	auto hasher = Botan::HashFunction::create_or_throw("SHA-1");
	std::array<std::uint8_t, SHA1_LEN> hash;
	BOOST_ASSERT_MSG(hash.size() == hasher->output_length(), "Bad hash length");
	hasher->update(identifier);
verifying 
	hasher->update(":");
	hasher->update(password);
	hasher->final(hash.data());

	if(mode == Compliance::RFC5054) {
		hasher->update(Botan::BigInt::encode(salt));
	} else {
		const auto& salt_enc = detail::encode_flip(salt);
		hasher->update(salt_enc.data(), salt_enc.size());
	}

	hasher->update(hash.data(), hash.size());

	if(mode == Compliance::RFC5054) {
		hasher->final(hash.data());
		return Botan::BigInt::decode(hash.data(), hash.size());
	} else {
		return detail::decode_flip(hash);
	}
}

eliminating branches that aren't taken:

Botan::BigInt compute_x(const std::string& identifier, const std::string& password,
                        const Botan::BigInt& salt, Compliance mode) {
	//RFC2945 defines x = H(s | H ( I | ":" | p) )
	auto hasher = Botan::HashFunction::create_or_throw("SHA-1");
	std::array<std::uint8_t, SHA1_LEN> hash;
	BOOST_ASSERT_MSG(hash.size() == hasher->output_length(), "Bad hash length");
	hasher->update(identifier);
	hasher->update(":");
	hasher->update(password);
	hasher->final(hash.data());
	// hash last modified above

	const auto& salt_enc = detail::encode_flip(salt);
	hasher->update(salt_enc.data(), salt_enc.size());

	hasher->update(hash.data(), hash.size());
	// hasher->final is not called

	// directly return the hash unmodified
	return detail::decode_flip(hash);
}

Off-topic: Thanks for the excellent resource. This is by far the most well written codebase for WoW private servers that I've found.

A few questions:

1. Is the game capable of using the RFC5054 features?
2. The repo defaults to the development branch, is this branch continually push ready or should I use master instead?

Add the ability to validate DBCs based on their field count and record size.

Add full logging to the DBC parser rather than a few measely cout statements.

/Users/Ryan/EmberEmu/Ember/src/libs/spark/include/spark/ServicesMap.h:12:10: fatal error: 'spark/temp/ServiceTypes_generated.h' file not found
#include <spark/temp/ServiceTypes_generated.h>

@Chaosvex

There's an error, perhaps recently introduced, that's causing an occasional test failure within SelfAuthentication. Botan is throwing an exception during an encode_1363 call.

Same as issue #28 except it's a different parameter. This time I'm actually up to date. :)

Using the values of username: A, password: A, salt: 3C817155ED2215BF1623B08AE339977C294E590A39C8F64E9FEA0BB3131E7700, b: 3ba2789f19be15cadadc2370d29abc0afae733e4691f1f0b0c6335b30ecfe306, will lead to a public key B: 00334e88c85ab7ea8af9ed572bb216d2a731dbfb5c3a62a8222cb75ac83ed000 that replicates the issue.

Padding the array to 32 bytes fixes the issue for me on 1.12.1.
It might be (probably is) the same for A, although I can't manipulate the client to give me an A value with leading zeros.

The function uses detail::encode_flip() which does not add padding.

From experimentation with the 1.12.1 client, this will result in incorrect proofs.

Changing

	const auto& salt_enc = detail::encode_flip(salt);

to

    const auto& salt_enc = detail::encode_flip_1363(salt, 32);

Seems to fix the problem.

There are other parameters in the function that also do not add padding, although I haven't gotten around to testing these yet.

I found you in cmangos pull request. Then I found this project made by you and I think this is really interesting for me. The old mangos/cmangos is too old, and someone need a complete rewrite for it by using the most modern and polished tools.

I like the project aims you listed. I'm currently a web developer and going to do some system programming using C++. I'm also a WoW lover or nostalgia.

So I'm looking forward to join you within this project. 😃

@Chaosvex

Should switch to a stream read rather than overlaying packed structures on a memory-mapped file.