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

Change the way how password is stored in C++ engine. #14196

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Dec 31, 2023

In the discussion about the implementation of transfer_player (#14129) was recognized that the password is stored as pure text in the Minetest engine during all game sessions. Because it allows potential attackers to easily find the password in a memory dump, this PR has been created.

  • Goal of the PR
    This PR should improve the safety of passwords.
  • How does the PR work?
    This PR erases every password storage in C++ as soon as possible.
    Password is no longer kept in struct GameStartData and class Client as pure text for all time the client is connected to the server.
    The New class ClientAuth accepts the password and player name and immediately calculates the hash and everything that can be necessary in the future for authorization and what requires the password to be known.
  • Does it resolve any reported issue?
    I don't think so.
  • Does this relate to a goal in the roadmap?
    Probably not.
  • If not a bug fix, why is this PR needed? What usecases doas it solve?
    This improves safety on the client side. It can be marked as a bugfix, probably.

To do

This PR is a Ready for Review.

How to test

Basic check:

  • Test if auth is working.

Reconnect check:

  • Host/create the server.
  • Connect to the server.
  • Shut down the server by command /shutdown_with_reconnect (included in DevTest).
  • Restart the server.
  • Use the reconnect button on the client.

@wsor4035 wsor4035 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Dec 31, 2023
@HybridDog
Copy link
Contributor

I have used GameConqueror to search the password in the memory.
Without loss of generality, I have chosen bumcivilian as password, which contains (in hexadecimal notation) 62 00 00 00 75 00 00 00 6d 00 00 00 63 00 00 00 69 00 00 00 76 00 00 00 69 00 00 00 6c 00 00 00 69 00 00 00 61 00 00 00 6e when it's encoded in UTF-32, i.e. as wstring on my system.

master branch

When I have entered the password in the main menu but did not join the server yet, I could find one instance of the password formatted as wstring and no ASCII-formatted instance.

After I have joined the server, I could find no wstring-formatted instance and two ASCII-formatted instances.

After I have exited and was back in the main menu, I could find no wstring-formatted instances and three ASCII-formatted instances.

After joining the server again, there were two ascii-formatted instances, and back in the main menu again, there were again three ascii-formatted instances.

This pull request

Like in the master branch, when I have entered the password in the main menu but did not join the server yet, I could find one instance of the password formatted as wstring and no ASCII-formatted instance. This is the expected behaviour.

After joining the server or after going back to the main menu I could no longer find the password formatted as wstring or ASCII.

src/client/clientauth.cpp Outdated Show resolved Hide resolved
src/client/clientauth.cpp Outdated Show resolved Hide resolved
@sfence
Copy link
Contributor Author

sfence commented Jan 1, 2024

@HybridDog Thanks for the quick feedback and testing.

src/util/srp.cpp Outdated Show resolved Hide resolved
src/client/clientauth.h Outdated Show resolved Hide resolved
src/client/clientauth.cpp Show resolved Hide resolved
src/client/clientauth.cpp Show resolved Hide resolved
src/client/clientauth.h Outdated Show resolved Hide resolved
src/client/clientlauncher.cpp Outdated Show resolved Hide resolved
src/client/client.cpp Outdated Show resolved Hide resolved
src/client/clientauth.h Outdated Show resolved Hide resolved
src/util/auth.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 9, 2024
@sfence sfence force-pushed the sfence_fix_password_storage branch from 180d83b to aec7ede Compare January 10, 2024 07:08
@sfence
Copy link
Contributor Author

sfence commented Jan 15, 2024

Again check for clang_14 timeouts:

Waiting for done timed out
Error: Process completed with exit code 1.

Can be clang_14 timeout time changed by some change in the repository?

@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 15, 2024
@sfan5 sfan5 self-assigned this Jan 24, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

structure looks fine in general

src/client/clientauth.cpp Outdated Show resolved Hide resolved
src/client/clientauth.h Outdated Show resolved Hide resolved
src/gameparams.h Outdated Show resolved Hide resolved
src/util/auth.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 20, 2024
@sfence sfence force-pushed the sfence_fix_password_storage branch from 2cb9286 to 3ac47e2 Compare March 29, 2024 18:46
@sfence
Copy link
Contributor Author

sfence commented Mar 29, 2024

For maintenance reasons, all previous commits merged into one. Rebased.

Command to test reconnect added. Reconnect not working.

I will have to store the password hash in srp.cpp file and not free the SRPUser object before returning to the main menu.

src/client/client.cpp Outdated Show resolved Hide resolved
src/network/clientpackethandler.cpp Outdated Show resolved Hide resolved
src/porting.cpp Outdated Show resolved Hide resolved
src/porting.h Outdated Show resolved Hide resolved
@sfence
Copy link
Contributor Author

sfence commented Apr 1, 2024

@HybridDog Can you please recheck if the password is still unreachable by looking for it in memory?

@sfence
Copy link
Contributor Author

sfence commented Apr 1, 2024

Updated How to test steps.
Reconnect now works with this PR as well.
Review points should be fixed. I hope I do not miss something.

@HybridDog
Copy link
Contributor

The password still disappears from memory in the cases which I have mentioned above (#14196 (comment)) and when I register a new player instead of joining with an existing one.

However, after joining a server, if I press Escape to reach the ingame menu, change the password there and then continue playing, I can find one wstring-formatted instance of the password in the memory and no ASCII-formatted instance.
I think in this case the password should not exist in memory.

@sfan5 sfan5 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 3, 2024
@sfan5
Copy link
Member

sfan5 commented Apr 3, 2024

Sorry for changing my opinion after you've already put work into this, but the more code changes go into this PR the less I feel it's worth it.
With a hypothetical attacker that can read the memory of any user process, surely the attacker could also do keylogging or backdoor the Minetest executable/process.

@rubenwardy
Copy link
Member

The main benefit is probably preventing leaks when sharing a dump or debug trace

@sfence
Copy link
Contributor Author

sfence commented Apr 9, 2024

This is still blocking #14129. Would be nice if core developers found a conclusion about safe/unsafe to store passwords in pure text and merge or close this PR.
It makes me able to apply feedback to #14129 blocked by the core developer's disagreement with storing passwords in pure text.

@sfence sfence force-pushed the sfence_fix_password_storage branch from d6c265b to 47a8ee1 Compare April 9, 2024 17:23
@sfence
Copy link
Contributor Author

sfence commented Apr 9, 2024

@HybridDog Thanks for checking. Should be fixed now.

@sfence sfence force-pushed the sfence_fix_password_storage branch from c79abb9 to 74520fb Compare April 9, 2024 19:24
@sfence
Copy link
Contributor Author

sfence commented Apr 9, 2024

Windows build failed due to:

CMake Error at irr/src/CMakeLists.txt:267 (message):
SDL2 is too old, required is at least 2.0.10!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants