Fix: [Network] Reading beyond the length of the server's ID when hashing passwords #9176
Conversation
This file contains 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
…ing password Under normal circumstances the server's ID is 32 characters excluding '\0', however this can be changed at the server. This ID is sent to the server for company name hashing. The client reads it into a statically allocated buffer of 33 bytes, but fills only the bytes it received from the server. However, the hash assumes all 33 bytes are set, thus potentially reading uninitialized data, or a part of the server ID of a previous game in the hashing routine. It is still reading from memory assigned to the server ID, so nothing bad happens, except that company passwords might not work correctly.
rubidium42
added
the
backport requested
PeterN
approved these changes
May 2, 2021
LordAro
added
backported
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation / Problem
Under normal circumstances the server's ID is 32 characters excluding '\0', however this can be changed at the server. This ID is sent to the server for company name hashing. The client reads it into a statically allocated buffer of 33 bytes, but fills only the bytes it received from the server. However, the hash assumes all 33 bytes are set, thus potentially reading uninitialized data, or a part of the server ID of a previous game in the hashing routine.
It is still reading from memory assigned to the server ID, so nothing bad happens, except that company passwords might not work correctly.
Description
Get the length of the network/server ID string when hashing, and interpret anything after that length as '\0'.
Kept the result of the hashing the same, except when the overread would occur.
Limitations
Xor-ing for salting is not a common practice anymore, usually strings are just appended after each other. I chose not to break those for now as that could break (custom) servers that save the company passwords locally, so it would be better to change the hashing logic when there is a way to denote versions/types of hashes.
Using std::string would make some things easier/nicer, but to be able to backport this I have done the minimal amount. I have a branch where it does convert it to std::string, but that requires many related commits so not suitable for a backport.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.