From dd972a7f316e5be5ff472b1b51807aef92b8cc2f Mon Sep 17 00:00:00 2001 From: Lucas Nicodemus Date: Tue, 25 May 2021 22:36:10 -0700 Subject: [PATCH] Warn users about odd password conditions TShock was originally designed to handle many things that Terraria did not. Therefore, TShock always "took over" for the server password prompt. We then added the ability to login via the password prompt if you had an account, so that you could play on a server and login without having to run /login in the chat window. Then, UUIDs were introduced, and we added the ability to login via UUID. This has created a cascading scenario where users are potentially affected by many different things. We have always treated a user's runtime intent as the most important: if a user sets something on the console, it should be taken as the "most true" setting. In other words, we believe that the most recent choice the user made is the valid one. But for some of the config settings we have, we've made it opaque as to how this decision making works. We also aren't clear what certain things do by default. Currently, if UUID login is enabled, a user will login "magically" and bypass any password prompt. Even if this is disabled, though, users are, by default, allowed to enter their passwords at the password prompt instead of the server password. Both of these take priority over the runtime setting. The problem is that we haven't really made it clear if we should override the runtime setting here. This is because the Terraria interactive prompt asks for a server password, and one of the two "bypass" settings is not a password setting at all. What do we respect? I decided that the best approach is to just communicate really loudly about these settings. If a runtime password is set, we'll warn users if either of the bypass settings are "in play." If it's not set, we'll warn users if the server password was set in config.json, just so they know which password is being used. If UUID logins are enabled we'll also warn users about that and the security risks attached, no matter what. I don't know that we should really have this feature, but we shouldn't get rid of it, imho. The only thing I don't think we need to warn about is if login before join is enabled. Login before join just acts as a way to speed up logins for registered users. In an ideal world, users who shouldn't be able to login should be banned. But I split the difference since we're warning about UUID logins. The only real downside to this change is that the PostInit hook gets bigger. But dumping this stuff in another file/area/etc., seems dumb since some of the logic exists here already. I think we can refactor this later, but it's not my most pressing priority. This whole change was inspired by the fact that @Onusai tried to lock down their server but failed because of these settings enabled. We need to be more transparent about logins, and this is a good first step. --- CHANGELOG.md | 1 + TShockAPI/TShock.cs | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 246db4f5..d40ffa85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Fixed torchgod settings to include whether or not torchgod has been fought by the player before and respect `usingBiomeTorches` setting. (@Quinci135) * Fixed /worldmode not synchronising data to players after updating the world state (@bartico6, @Arthri) * Added `OnSendNetData` hook to TSAPI, which enables developers to intercept traffic being sent from the server to clients. (@Stealownz) +* Added warnings for conditions where a password is set at runtime but can be bypassed. The thinking is that if a user sets a password when they're booting the server, that's what they expect to be the password. The only thing is that sometimes, other config options can basically defeat this as a security feature. The goal is just to communicate more and make things clearer. The server also warns users when UUID login is enabled, because it can be confusing and insecure. (@hakusaro, @Onusai) ## TShock 4.5.3 * Added permissions for using Teleportation Potions, Magic Conch, and Demon Conch. (@drunderscore) diff --git a/TShockAPI/TShock.cs b/TShockAPI/TShock.cs index 89e57ba0..c2bfbc39 100644 --- a/TShockAPI/TShock.cs +++ b/TShockAPI/TShock.cs @@ -824,10 +824,45 @@ namespace TShockAPI if (!string.IsNullOrEmpty(Netplay.ServerPassword)) { //CLI defined password overrides a config password + if (!string.IsNullOrEmpty(Config.Settings.ServerPassword)) + { + Log.ConsoleError("!!! The server password in config.json was overridden by the interactive prompt and will be ignored."); + } + + if (!Config.Settings.DisableUUIDLogin) + { + Log.ConsoleError("!!! UUID login is enabled. If a user's UUID matches an account, the server password will be bypassed."); + Log.ConsoleError("!!! > Set DisableUUIDLogin to true in the config file and /reload if this is a problem."); + } + + if (!Config.Settings.DisableLoginBeforeJoin) + { + Log.ConsoleError("!!! Login before join is enabled. Existing accounts can login & the server password will be bypassed."); + Log.ConsoleError("!!! > Set DisableLoginBeforeJoin to true in the config file and /reload if this is a problem."); + } + _cliPassword = Netplay.ServerPassword; Netplay.ServerPassword = ""; Config.Settings.ServerPassword = _cliPassword; } + else + { + if (!string.IsNullOrEmpty(Config.Settings.ServerPassword)) + { + Log.ConsoleInfo("A password for this server was set in config.json and is being used."); + } + } + + if (!Config.Settings.DisableLoginBeforeJoin) + { + Log.ConsoleInfo("Login before join enabled. Users may be prompted for an account specific password instead of a server password on connect."); + } + + if (!Config.Settings.DisableUUIDLogin) + { + Log.ConsoleInfo("Login using UUID enabled. Users automatically login via UUID."); + Log.ConsoleInfo("A malicious server can easily steal a user's UUID. You may consider turning this option off if you run a public server."); + } // Disable the auth system if "setup.lock" is present or a user account already exists if (File.Exists(Path.Combine(SavePath, "setup.lock")) || (UserAccounts.GetUserAccounts().Count() > 0))