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

[sfence]

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.

[HybridDog]

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.

[sfence]

@HybridDog Thanks for the quick feedback and testing.

[sfence]

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?

[sfan5]

structure looks fine in general

[sfence]

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.

[sfence]

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

[sfence]

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]

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]

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]

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

[sfence]

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]

@HybridDog Thanks for checking. Should be fixed now.

[sfence]

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!

[red-001]

This seems like a good change in a lot of ways, but if the concern is that memory dumps include the password, this only partially mitigates that by instead storing a hashed password.

Could the memory for the hashes be allocated separately and marked as inaccessible when not in use, as well as requesting it's not included in any core dump on supported systems? Libsodium does something like that, https://libsodium.gitbook.io/doc/memory_management https://github.com/jedisct1/libsodium/blob/1012bbc380c81bf7782a85d43c2c9ed7caf8c8b9/src/libsodium/sodium/utils.c

I think windows mini dumps shouldn't include PAGE_NOACCESS pages, or at least shouldn't for most configurations https://learn.microsoft.com/en-us/windows/win32/api/minidumpapiset/ne-minidumpapiset-minidump_type

[Zughy]

@sfence rebase needed

[sfence]

@Zughy Rebased, but the main problem of this PR is not the missing rebase but the decision if we want #14129 with or without a password stored in pure text.

[Zughy]

@sfence if nobody answers, my suggestion is to add it as a point of the next meeting