26F-Studio / Techmino

Techmino:方块研究所唯一官方仓库(Github)
https://www.studio26f.org
GNU Lesser General Public License v3.0
532 stars 66 forks source link

More secure server communication #246

Open Not-A-Normal-Robot opened 3 years ago

Not-A-Normal-Robot commented 3 years ago

[tl;dr: don't store or send passwords in plain text, instead use a salted hash; use SHA2 and not MD4/MD5; use secure connection for server communication; add 2-factor authentication (unless if you want to use OAuth, which is also another pretty good way for accounts)]

Currently, the game remembers your account by storing it in a file that has the email and password of your Techmino account in plain text. This is extremely insecure, because hackers can easily take over your entire account just by grabbing the file, and if you use the same password for everything, then they can take over everything you own on the Internet. Youtube: How not to store passwords (shortened: only store a salted hash of the password, or use OAuth to log in. Use SHA and not MD4/MD5.)

Secondly, I've noticed the game sends the email and password to the server in plain text. This is also another security flaw. Either use a secure/encrypted connection (HTTPS), or send it as a salted hash, or more preferably, both.

Trebor-Huang commented 3 years ago

wtf I thought any sensible programmer won't do that

Not-A-Normal-Robot commented 3 years ago

It teaches you what not to do about storing passwords and it also teaches you the right way at the end of the video. A weird method to teach the viewers, sure, but then you know the pros and cons of each way of storing passwords

Trebor-Huang commented 3 years ago

I mean it's a glaring serious problem and anyone responsible for writing that code should be disqualified and hanged :(

Not-A-Normal-Robot commented 3 years ago

When is this gonna be worked on :eyes:

ParticleG commented 3 years ago

When is this gonna be worked on 👀

For the first problem, Techmino is currently fully open sourced. So any local encryption may not be so safe.

For the second problem, in the next distributed Techmino backend, Server would only use HTTPS/WSS. But the client should also add support for SSL encryption. Currently, the Techmino client doesn't support SSL at all.

MrZ626 commented 3 years ago

@ParticleG should add an API of changing password first, or new hashed passwords cannot be available

ParticleG commented 2 years ago

Client SSL lib now implemented. Need more testing, also need to modify iOS love source codes

zer0x64 commented 2 years ago

The part about "it cannot be secure locally because it's open source" is blantantly wrong. This is what keychains are for, and closed source software doesn't really help anything as software can be reverse-engineered pretty easily.

There are mutliple ways to go about it, but it's possible to implement mechanism, such as OPAQUE or hashed passwords + long term tokens, so that am attacker would get access to less sensitive informations instead of a cleartext password that may be reused somewhere else.

I'd be willing to work on the authentication aspect of the issue if I can be of any help (I don't know if you accept pull requests)

Trebor-Huang commented 2 years ago

The part about "it cannot be secure locally because it's open source" is blantantly wrong. This is what keychains are for, and closed source software doesn't really help anything as software can be reverse-engineered pretty easily.

I stressed this multiple times, and I've given up. Probably it will be fixed in a few weeks.

Trebor-Huang commented 2 years ago

I'd be willing to work on the authentication aspect of the issue if I can be of any help (I don't know if you accept pull requests)

Pull requests are very welcome.

MrZ626 commented 2 years ago

Actually it's in the plan, but there is no way to change password now, so after finishing our new server we'll convert the stored password to hashed one automatically.

MrZ626 commented 2 years ago

PR is welcomed but it won't be merged for a while, or players will be unavailable to login

flaribbit commented 2 years ago

Considering luasec for ssl connection, win/linux/mac/android are easy to set up. As for ios, the luasec and ssl library should be staticly linked to love2d. Then we may preload these modules:

luaopen_ssl_core(L);
luaopen_ssl_context(L);
luaopen_ssl_x509(L);
luaopen_ssl_config(L);

copy ssl.lua to source folder, then follow luasec wiki:

require("socket")
require("ssl")

-- TLS/SSL client parameters (omitted)
local params

local conn = socket.tcp()
conn:connect("127.0.0.1", 8888)

-- TLS/SSL initialization
conn = ssl.wrap(conn, params)
conn:dohandshake()
--
print(conn:receive("*l"))
conn:close()

it should work

Another problem is that, my new async client code doesn't handle ssl handshake properly. We should still use old sync version with love2d's thread.

hope there's a builtin ws module in LÖVE12

ParticleG commented 2 years ago

@flaribbit This would be added in next major update of Techmino

ImpleLee commented 2 years ago

Is my keyword still stored locally in plain text?

ParticleG commented 2 years ago

Haven't transfer the database yet. Would work on that soon

Not-A-Normal-Robot commented 1 year ago

this issue is done, i think

the password is now hashed before being stored locally on /conf/user

or obfuscated, at least. too lazy to actually check

ParticleG commented 1 year ago

Hmmm, but SSL is still not applied

Not-A-Normal-Robot commented 1 year ago

ok I take that back, ig passwords aren't stored in plaintext anymore but communications aren't encrypted yet

ParticleG commented 1 year ago

Server side SSL is set. Now we wait for the client side

ParticleG commented 1 year ago

@flaribbit wrote a SSL + Websocket library mwebsocket, should try that @MrZ626