Security Fix: Empty the password after sending it to server

[nerzhul]

The password is passed to Client class and never cleaned. Then it stay in memory every time the user was connected. It's possible to recover it by accessing to minetest process memory.

This fix clear the string after sending _INIT packet.

Please merge fast

[RobertZenz]

If I'm not completely mistaken, this does not guarantee that the existing memory is overwritten in any way.

[nerzhul]

@RobertZenz you are right, std::string doesn't clear the memory, we only change the pointer here

[RobertZenz]

@nerzhul What's the point then? The password will still be in memory for an undefined amount of time with this change.

[nerzhul]

We change the pointer, then it stay in an unused memory and can be overwrite.

[RobertZenz]

But if you're worried that the password might be scraped from memory, then clearing the pointer to it is not enough. You'd need to overwrite the memory which it is contained in, which the easiest solution would be to make it a char array in the first place, and then zero that.

[rubenwardy]

Maybe you should overwrite it multiple times, so the NSA can't get it?

If a program could read the memory of Minetest, then it could replace it with a fake Minetest or record keystrokes.

http://irc.minetest.ru/minetest-dev/2015-02-01#i_4137808

But this is still an improvement, there is not point keeping the password - although you should blank the string at source.

[gregorycu]

There are APIs that can be used to scrub memory. Anything else is just a waste of time as the reassignment can be optimised out entirely.

[Zeno-]

@nerzhul
Actually I think this should be re-opened. There have been cases where people have pasted gdb backtraces with their password hashes in the dump (and yes, this is an issue).

Doing m_string = ""; isn't going to do much though... no matter the type of string (be it one of the C++ strings or a C str) it's likely to just set the initial char to the NIL character. There's no need to go crazy, but just writing every character over the plain text password and the hashed password with spaces is better than nothing (and will probably be enough for everything)

[gregorycu]

On windows, you can call SecureZeroMemory() on str.c_str() and str.size(). No idea about Linux.

[Zeno-]

I think the problem referred to earlier (about compilers optimising things away) would only come into play in situations like:

{
char mem[SZ];
// ...
memset(mem, 0, sizeof mem / sizeof mem[0]);
}

where the aggregate object goes out of scope immediately after the memory is cleared (or anything after memset does not affect the object in question). I could see that a compiler might reasonably assume in those circumstances that the memset is pointless and optimise it away. This is speculation of course and I've trivialised the example for the sake of discussion, but it makes sense. However since MT keeps both of the objects in scope until the end of the program's lifetime I don't think it likely that a compiler, especially in a multithreaded application, would be likely to optimise a call to memset (or similar) away. Of course maybe "all of program optimisation" and similar optimisation strategies might so it's not outside the realm of possibility but I think for the purposes of a game such as minetest that 100% ensurance that the memory is actually set to 0 (or some other value) might be overkill... (especially considering more serious security concerns ;))

[nerzhul]

Re-opened as @Zeno- asks

[Zeno-]

@nerzhul Thanks. It still needs to be modified slightly, of course. m_password = ""; is unlikely to obscure anything but the first character ;)

[nerzhul]

@Zeno- the password mustn't be stored in the class, i think we must change the password handling for next release, it mustn't be stored, only passed to functions. At this time it's too complicated to architecture the connection (feature freeze)

[HybridDog]

You could call it name2 and not password and set password to a random value for confusion.
And if someone makes a server, which tells him/her the passwords of people, invites people and then he/she joins other servers with these invited peoples' accounts…

[RobertZenz]

@HybridDog That's called "security through obscurity" and is a very bad idea. It only adds obstacles to code maintenance and debugging without adding anything to security. The problem is not (or should not) be the fact that there is a variable called "password", the problem is that the password stays in memory for too long. That's why I argued against the proposed "fix" here because it only changes "stays in memory for the gaming session" to "stays in memory for an undefined amount of time".

If you're connecting to a forged/tempered/hacked server, you're already screwed and there's nothing on the Minetest end to fix that. The same problem exists in the browser world, if you happen to land on a website that looks like PayPal, and you enter your password there, you're screwed. And if you're using the same password on every server, well, that's bad. And if you happen to also use the password for important stuff like your e-mail or paypal account, well, that's your fault.

The only option to fix that would be a Public/Private key authentication scheme as it is already in place in SSH, which seems absolute overkill for a game (and would require key-management on the users end, which most users simply do not want to do for some reason).

[HybridDog]

thanks, it's easier to get someone's password if he/she joins your server than getting it from his/her local memory, isn't it?

[nerzhul]

I close this pull request because the problem will be fixed after release. We cannot fix it without some PR which be merged after release.

[HybridDog]

Maybe you should overwrite it multiple times, so the NSA can't get it?

Overwriting doesn't help against L1TF.