[pull] master from mangosone:master - #264
Merged
Merged
Conversation
…oyment Thirty lines of cipher were costing a DLL. OpenSSL 3 moved RC4 to the legacy provider, which is not compiled into libcrypto -- it is a separate module discovered on disk at run time. So a 256-byte permutation dragged in ossl-modules/legacy.dll beside the executable, an OPENSSL_MODULES search path, CMake that hunted for the file and copied it, CI that pointed an environment variable at it, an installer obliged to ship it, and a start-up check in both daemons that REFUSED TO BOOT without it. It was also a loaded gun. The legacy provider is deprecated; the release that finally drops it would have stopped this emulator starting, for want of a cipher any competent programmer can write from the specification. And the protocol needs RC4 forever, because the 2.4.3 client is never going to be updated -- a cipher the wire mandates in perpetuity belongs in the tree, not behind a compatibility shim someone else maintains for our convenience. So: ARCFOUR, written out. A key schedule, a permutation, a stream that XORs. The class keeps its interface, so no caller changed -- AuthCrypt and Warden were not touched. CryptoStressTest was not touched either, and that is the point: `Crypto_arc4_matches_the_published_vector` passes against the same published vector it always checked, so the equivalence is demonstrated rather than claimed. Two details worth keeping. Init rebuilds the permutation from the identity every time, because it is a RE-key as often as a first key and keying on top of a used state would make the stream depend on how much traffic preceded it; and it rewinds the stream position, forgetting which is the classic way to build a cipher that decrypts the first session and nothing after it. OpenSSLProvider and its test are deleted outright. RC4 was the only thing this tree ever asked the legacy provider for; SHA-1, SHA-256, AES and the bignum work all live in the DEFAULT provider, which is inside libcrypto and needs nothing on the side. The mangosd binary now contains no reference to ossl-modules, to OSSL_PROVIDER, or to a legacy provider at all. Linking is unchanged and still dynamic. This removes a module that had to be FOUND at run time, which is a different problem from where libcrypto itself comes from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The published vector proves nine bytes from a fresh cipher. Both classic ways
of getting RC4 wrong live after those nine bytes, and both produce a server
that works perfectly for the first player to log in:
* Init keys on top of the permutation the previous session left behind, so
the stream depends on how much traffic preceded it;
* Init rebuilds the permutation but forgets to rewind the stream position, so
the cipher decrypts one session and nothing after it.
Seven cases, in a file of their own so that CryptoStressTest stays the
untouched proof that the in-tree implementation reproduces the published bytes.
Every expectation is derived inside the test -- from that vector, or by
comparing two ciphers -- so none of it rests on a constant recalled from
somewhere else.
The one that carries the weight runs four kilobytes through a cipher first and
only then re-keys it, because a clean state hides the first bug entirely: the
two implementations agree until the permutation is dirty, which in a server
means until a session has carried some traffic.
Writing this found something, though not in the cipher. THE KEY LENGTH IS FIXED
AT CONSTRUCTION: Init(uint8*) takes no length and reads exactly as many bytes as
the constructor declared, so re-keying with a longer key silently uses its
prefix and with a shorter one reads past the caller's buffer. That was equally
true of the OpenSSL implementation -- the length was set on the context in the
constructor there too -- but nothing said so anywhere, and the first draft of
this file re-keyed a three-byte cipher with seven and could not see why the
streams differed. It has a test of its own now, so the next person is told
instead of left to find out.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pin moves from 4d256b4 to 2b8b7f8, both on mangos/realmd, which is the URL .gitmodules names -- so a fresh recursive clone gets it, which a pin into the fork would not have. Two commits come with the move and both belong here: #39 removes realmd's start-up demand for the OpenSSL legacy provider. It has to travel with this branch: the header that check reached for was deleted on the core side, so the pin and the core are only consistent together. This is the second half of a change that spans two repositories, and until now this branch did not build with BUILD_REALMD=1 on a clean checkout. #38 is the SRP6 width fix, which had already been merged upstream while the pin sat one commit behind it. Picking it up is incidental to this change and entirely welcome. Verified against this exact submodule commit rather than against a working tree that merely resembled it: a full build on FreeBSD/clang with BUILD_REALMD=1, 191 tests green, and neither binary containing a reference to ossl-modules, to OSSL_PROVIDER, or to a legacy provider. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Plain ARCFOUR: the keystream starts at byte zero and nothing is thrown away. That was already true, and it was nowhere written down, which is the whole problem -- because the obvious "improvement" is wrong here and looks right. WotLK and later are documented as using RC4-drop1024. A reader who knows that will eventually bake the drop into Init, and they will believe they are fixing an omission. The drop does not belong to the cipher: where the protocol wants it, THE CALLER does it, visibly, in its own source -- AuthCrypt keys both directions and then pushes 1024 zero bytes through UpdateData. Warden uses this same class and drops nothing. A discard inside Init would be right for one caller and wrong for the other, and there is no way for the cipher to know which it is serving. The failure would be the expensive kind. World traffic would decrypt to garbage and Warden would break in some different way, and both would present as a protocol fault -- a client that connects and then cannot read a single packet -- rather than as a crypto one. Nobody would look at the cipher. The published vector would have caught it, but as a hex mismatch, and whoever added the drop would have spent an afternoon doubting their key schedule. Arc4_TheKeystreamStartsAtByteZero fails with a name that says what happened. It checks both constructors, because a discard added to Init alone would slip past a test that only used the keying one, and it asserts that the head of the stream is emphatically not the stream at offset 1024 -- which is exactly what a baked-in drop would hand back. Worth recording where this bites. On this core AuthCrypt does not use ARC4 at all: 2.4.3 has its own inline header crypt, and the only user of this class is Warden, which performs no drop. So the trap is not armed here today. It arms itself the moment this implementation is carried across to a core whose AuthCrypt does use ARC4 -- which is the direction these trees are travelling in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The extractor writes data.manifest, which is SHA-256, which is EVP -- so a tool that touches no network and no database nevertheless carries an OpenSSL dependency. Nothing shipped it. The tools directory worked only when something else had already put libcrypto on PATH, which on a developer's machine it always had. It is also the one binary here an END USER runs by hand, which makes it the worst place for a missing-DLL box: the audience least equipped to work out what libcrypto-3-x64.dll is, holding a tool that will not start. So it travels with the binary, in the build tree and in the install, following the pattern realmd already uses for its own test executable -- matching the DLL family rather than pinning a major, because the runtime name carries the version and OpenSSL 4 is coming. Verified on Windows rather than reasoned about, since the whole block is inside if(WIN32) and FreeBSD cannot execute a line of it: MSVC 2022 with OpenSSL 3, find_file resolved both DLLs, cmake_install.cmake carries both, and after building mangos-extractor they are sitting next to the executable. That test also settled a question the comment had been hedging. dumpbin reports the built baker importing libcrypto and NOT libssl -- the linker drops an import nothing references. libssl is copied anyway: that outcome belongs to this compiler and this OpenSSL build rather than to the design, and a spare DLL costs a megabyte beside a data set measured in gigabytes, while a missing one costs a bug report from someone who cannot read a dependency walker. A runtime that cannot be found warns and configures; a server build has no business refusing over a tool's convenience, and PATH may well still provide it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI caught this and I did not, because I ran the test binary directly instead of ctest -- and build_policy is a ctest, not a case inside mangos_tests. The suite reported 191 passing while the twelfth test was never asked. The rule required mangosd to leave through `return 1` when the OpenSSL provider manager failed to initialise. That is a good rule for the reason it gives: returning 0 tells a supervisor the process exited cleanly, so nothing restarts it. It has simply run out of subject. RC4 was the only algorithm this tree ever fetched from the legacy provider, it is implemented here now, and mangosd performs no fallible crypto init at start-up at all -- SHA-1, SHA-256 and the bignum work come from the default provider, which is inside libcrypto and cannot be absent while libcrypto is present. Deleted rather than loosened, and replaced by its own inverse: mangosd must not mention OpenSSLProvider again. A policy that matches nothing is worse than no policy, because it passes for the right reason today and for the wrong reason the moment somebody reintroduces a start-up check and forgets its exit code. This way, whoever brings a fallible crypto init back is told to bring the exit rule with it. 12 of 12 ctest cases pass, which is how this should have been checked the first time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )