MD5 digest() on ChangePassword and boolean check_pass() on Login

[slook]

The plain text password was saved in the database after successfully changing password, resulting in a locked account. This PR fixes that (although all that was actually missing was a encode_password() call in change_password() method), as well as trying out some fundamental security hardening measures which aim to isolate sensitive data away from being stored in memory.

• Fixed: Impossible to login after changing password because plain text was written to database
• Fixed: Incorrect hash sent back to peer's client after successful Login was a hash of the hash instead of just hashing once
• Renamed: Terminology of pass is declared to indicate that the value is hashed, as opposed to a plain-text password value
• Removed: Don't keep the plain password string hanging around in the User class of the client.d module, there's no need for it
• Removed: Risky function to set any string field with any value is no longer required, since it was only used for password
• Changed: Return only a boolean from the new check_pass() function to indicate success or failure during authentication
• Moved: Refactored all password digestion processes into private functions in the db.d module, this cannot be an overload
• Added: Require that the password string is non-empty before attempting any database queries during pre-authentication
• Added: Match the concatenated username + password hash and require this for the server admin message interface

[mathiascode]

Suggested change

	ubyte[16] digest = md5Of(str);
	string s;
	foreach (u ; digest) s ~= format("%02x", u);
	return s;
	return digest!MD5(str).toHexString!(LetterCase.lower);

[slook]

For some reason I don't understand, this doesn't work in here...

$ DEBUG=1 make
ldmd2 src/client.d src/db.d src/message_codes.d src/messages.d src/pm.d src/room.d src/server.d src/defines.d -Isrc -odobj -ofbin/soulfind -L-lsqlite3 -g -debug=db -debug=msg -debug=user
src/db.d(297): Error: escaping reference to stack allocated value returned by toHexString(digest(str))
make: *** [Makefile:47: bin/soulfind] Error 1
$ 

gdmd -w -c "db.d" (in directory: /home/user/Git/slook/soulfind/src)
db.d:42:5: error: statement is not reachable [-Werror]
   42 |     return;
      |     ^
db.d:276:4: error: statement is not reachable [-Werror]
  276 |    return null;
      |    ^
db.d:297:25: error: escaping reference to stack allocated value returned by toHexString(digest(str))
  297 |   return digest!MD5(str).toHexString!(LetterCase.lower);
      |                         ^
d21: all warnings being treated as errors
Compilation failed.

[slook]

This single-liner method can't work because its not possible to make a toHexString() call when it is encapsulated with secureEqual(), for what I realize now to be for obvious reasons (because the string character representations must be compared in parallel for security reasons to avoid a well-known critical vulnerability).

However, we arn't vulnerable to that anyway since the hashing isn't mandatory, and a timing attack depends on a failed login to be exploitable. In this case, we are actually better of using plain text auth without anything complicated going on, since there is no important data to guard.

It will have do the way it is or otherwise be reverted to how it was before, please advise what you want to do, or is it that I'm struggling with this because maybe we need an entirely different approach?

[mathiascode]

It's beyond the scope of this PR, but I would eventually stop storing MD5 hashes in the DB, and move to a modern alternative with password salting. Of course passwords are still sent to us in plaintext (can't change that), but at least you wouldn't be able to retrieve any passwords with just a copy of the database.

[slook]

Interesting, so is that because you wish to afford the level of privacy that would be required for running something like a distributed cluster of trust-less rendezvous servers?

If that's an eventual aim, then I think it would be a good idea if the servers negotiated with each other on a different port from that which is advertised to regular peers, regardless of if the data being transmitted between them is crypto-graphically secure.

sent to us in plaintext

Obviously when you say "us" you mean the Sdb class itself, which of course needs to be designed in such a way as to make it at least very difficult for any human to easily capture nor record the plaintext, even if they are the operator of the host.

It goes without saying that the contributors of the codebase (ie "us") are not in receipt of client requests, since this public repo can be audited by any interested party to verify that there is nothing fishy going on, but we can't currently guarantee that at runtime (unless we provide a way (such as motd or a chat command) for end-users to remotely compare some kind of build checksum?).

can't change that

Well, we could, if we provide the option for clients omit the plain text and only send their MD5 hash, if they want to and are aware that they have already registered (with any participating server in the pool), then it (i.e the Sdb class) would only require plaintext if were for the purposes of authorizing then securely distributing confirmation of a successful initial registration.

[mathiascode]

Mainly for future proofing. It's highly unlikely that Soulfind will ever be used to host any large server, but storing MD5 hashes for a list of users in 2024 is somewhat embarrassing.

By "us", I mean whoever runs a Soulfind instance. We can't change the Soulseek protocol, so passwords are sent in plaintext over the network, and we'd still have to send a MD5 hash in response. We can do our best to ensure we store passwords in a better way once Soulfind has received them, though.

It would work roughly like this: generate a unique salt every time a new password is received, add the salt to the password and hash it, and store both the hash and the salt in the database. When a user logs in, add the salt to the received password, and check if the hash matches what we've stored.

Of course people can write any server software they want that does whatever it wants with passwords. I don't really care beyond ensuring Soulfind tries its best to do things right by default.

[slook]

That would be a lot more robust.

we'd still have to send a MD5 hash in response

For now, how about storing a hash of the concatenated username~password string in the database, instead of storing only the password (which can be disposed of straight away after it has been sent in response to the Login)?

Edit: No, actually that might be a bad idea because that hash is transmitted in the clear every time the peer logs on.

[slook]

Closing, since this isn't going to be a suitable paradigm because the db.d module shouldn't be aware of users actual passwords, that would pose a potential security vulnerability especially if the database engine connection was perhaps on a different machine, so the hashes would have to be calculated and parsed deeper within the server or client module.

The main bugs which originally drove this investigation have since been fixed in recent commits to the master branch.