Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate player accounts with the OpenRA forum. #15333

Merged
merged 12 commits into from Jul 28, 2018

Conversation

Projects
None yet
5 participants
@pchote
Copy link
Member

pchote commented Jul 7, 2018

This PR implements a key-based authentication system against the forum to provide:

  • Identification to other players (removing the need to display IPs)
  • Identification to servers (Server.RequireAuthIDs=list,of,forum,userids)
  • Personalisation in the form of badges (and, in the future, ranks - the "Registered User" is a placeholder that we can unlock with forum-side changes)

The forum-side code will be squashed/reviewed/migrated to the OpenRA organization in the next few days. I am filing this PR now in the hopes that we can get the code reviewed in parallel the forum extension and then merge/deploy them both at the same time. This is best reviewed commit by commit. If anyone wants to test this locally in the mean time you can point the API at a local phpBB instance by adding the following to the mod.yaml

PlayerDatabase:
	Profile: http://localhost/phpBB3/openra/info/

The Red Alert / Tiberian Dawn / Dune 2000 icons are examples of default badges that will be available to set on the forum. We also intend to offer default badges for Windows / macOS / Linux, and grant custom badges for users who have contributed on GitHub or participated in forum events (e.g. the RAGL). Badge icons are pulled from the web, so do not rely on game-side changes.

The player tooltips are currently implemented only in the lobby and ingame (where Session.Clients are available). I intend on filing followup PRs to extend this to replays and the MP server list after this initial PR has been properly tested and debugged during the first playtest.

Fixes #2265.

@pchote pchote added this to the Next release milestone Jul 7, 2018

@pchote pchote force-pushed the pchote:forum-authentication branch from 45b78cc to 7c4826f Jul 7, 2018

@jrb0001
Copy link
Contributor

jrb0001 left a comment

I focused on the network part.

One open question is how much informations we want to give to the player if authentication failed. It might be a good idea to differentiate between "no auth information received", "unknown key", "key revoked" and "general verification failure" to help the player without leaking too much.

// assign the player number.
// Validate player identity by asking them to sign a random blob of data
// which we can then verify against the player public key database
var token = CryptoUtil.SHA1Hash(OpenRA.Exts.MakeArray(256, _ => (byte)Random.Next()));

This comment has been minimized.

@jrb0001

jrb0001 Jul 10, 2018

Contributor

There is no need to generate 256 random bytes, SHA-1 will reduce it to 160 bits (20 bytes) anyway. Just convert it to base16 or base64 directly to get the full randomness.

};

if (response.Fingerprint != null)

This comment has been minimized.

@jrb0001

jrb0001 Jul 10, 2018

Contributor

This should also check for request.AuthToken != null which is the case if the server doesn't send an AuthToken (for example an old one which would otherwise be fully compatible).

profile.ProfileName, profile.ProfileID);
}
else
Log.Write("server", "{0} failed to authenticate as {1}", newConn.Socket.RemoteEndPoint, handshake.Fingerprint);

This comment has been minimized.

@jrb0001

jrb0001 Jul 10, 2018

Contributor

Maybe log whether it was caused by revocation or verification failure? Logging doesn't leak any information so we should be as specific as possible.

{
if (Dedicated && Settings.RequireAuthIDs.Any())
{
Log.Write("server", "Rejected connection from {0}; Not in server whitelist.", newConn.Socket.RemoteEndPoint);

This comment has been minimized.

@jrb0001

jrb0001 Jul 10, 2018

Contributor

"Rejected connection from {0}; Not authenticated and whitelist is set."

{
public class PlayerDatabase : IGlobalModData
{
public readonly string Profile = "https://forum.openra.net/openra/info/";

This comment has been minimized.

@jrb0001

jrb0001 Jul 10, 2018

Contributor

This should be configurable through command line arguments, through the mod yaml or both.

This comment has been minimized.

@pchote

pchote Jul 10, 2018

Author Member

IGlobalModData classes can be overriden in mod.yaml. See the PR description for syntax.

This comment has been minimized.

@jrb0001

jrb0001 Jul 10, 2018

Contributor

Ok, I completely forgot about that during the review. Being able to override it through command line arguments would be a bonus, but the advantages of it are a bit questionable.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 10, 2018

It might be a good idea to differentiate between "no auth information received", "unknown key", "key revoked" and "general verification failure" to help the player without leaking too much.

We have that to an extent in the login widget:

  • No info returned → "Failed to connect to the OpenRA forum. Please check your internet connection."
  • Unknown key → "Your authentication key is not connected to an OpenRA forum account." → "Back" button copies to the clipboard and shows the "Add to your UCP" message (see screenshots above).
  • Revoked key → User is automatically logged out (keys destroyed) and the UI reverts to the first step in the link process
  • General verification failure → There is no verification during login, it is just a normal info fetch using the fingerprint. This surfaces when connecting to a server, but I don't see a good way to expose this. Right now it is treated as a non-fatal error and the user is silently downgraded to an anonymous player. I can't think of a case where this would systematically fail except where someone is being malicious, so i'm not sure we want to surface this in the UI.

I will fix the other comments when I have a few spare minutes.

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 10, 2018

I was thinking about sending a message like these in the ServerError order. Maybe also in a chat message (but that's not really useful until an authentication based admin role is implemented). So ok, that's also bonus because such a situation should be unlikely with normal users.

@chrisforbes

This comment has been minimized.

Copy link
Member

chrisforbes commented Jul 12, 2018

There are serious issues with the auth implementation here. Do you want details on this PR, or disclosed privately?

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

Email please.

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 12, 2018

@chrisforbes Are those serious issues already present in bleed? If not, then I don't see any disadvantages in a public disclosure which would help reviewers to check in that area more carefully.

I also found an issue with the design of this implementation which is quite easy to abuse depending on the user behaviour. But there is a relatively easy way to reduce it to just an a active MITM vulnerability which would be considerably harder to pull off. To counter that one as well, TLS with PKI is probably the easiest solution although it means more work for server operators.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

Are those serious issues already present in bleed? If not, then I don't see any disadvantages in a public disclosure which would help reviewers to check in that area more carefully.

This is a fair point. We’re not going to merge this while it has known implementation issues, so there should be little harm in mentioning them here if you would prefer to have them in the open.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

The one design issue that i'm aware of is that a malicious server can lie about the identity of clients to others, and there is no way for other clients to directly validate the identity of others. I considered this a reasonable trade off considering the trust that clients already place in the servers they connect to, and because I don't see a solution for client-client validation that doesn't either force authentication for all players or increases the complexity of the implementation to an impractical level.

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 12, 2018

The big flaw here is that the challenge is fully controlled by the server. A malicious user can open a connection to the desired server and get the challenge, put it into his own server before tricking the victim to connect to it. Then he gets a properly signed response which he can forward to the destination server to get authenticated.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

Can you elaborate on how that could be exploited? ATM I don't see how that gives the malicious user any privileges or information that they don't already have access to through MITMing the connection.

Issues stemming from server-crafted data could be mitigated by having the client salt the data before signing it, and then returning the salt as part of the response, but this wouldn't help with the above situation (which already can't be exploited outside of that one connection session?)

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 12, 2018

If you include the server address in the challenge, then the attack I described won't work anymore, unless the attacker is intercepting the connection. But to avoid that, we can't do it without authenticating the server first and something like PKI to establish trust. At that point, TLS becomes much easier and also has the advantage of encrypting the whole connection.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

I'm missing how having a server actively MITM a connection relates to the feature of this PR. Trust is done at the order level - we are verifying the handshake response, not the connection. I'm assuming you are thinking about server administration commands, but these should also be signed at the order level too. Including the server IP in the data to be signed is a good idea, I can add that.

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 12, 2018

The point is that no authentication at all is usually safer than broken authentication, because the users know that it isn't there and act accordingly.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

Certainly. My point was that I still don't understand how the situation outlined above qualifies as broken, and while I agree that connection-level encryption would be nice it seems to be an unrelated feature / scope creep. This isn't a challenge, but a question - I'm the first person to admit that I don't have enough experience with encryption to make a call on the topic.

If this really is a problem that only connection-level encryption can solve, then I would like to be able to give specific examples to justify why a highly anticipated feature has been dropped at the last minute and will probably never be attempted again (which is the reality of the situation - I don't have the experience or time to learn how to implement that).

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

a malicious server can lie about the identity of clients to others, and there is no way for other clients to directly validate the identity of others.

A simple solution to this would be to sign chat and server orders that could be spoofed for malicious purposes. Clients could then check and show a warning / drop the auth status if the signatures don't validate. This validation wouldn't be needed for game orders (where it may hurt perf), because the player being spoofed would not see the manipulated orders and the game would therefore desync.

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 12, 2018

As long as a connection is encrypted, the MITM can only choose to drop it completely. But as soon as you downgrade to cleartext, he automatically gets full control over the connection.

RSA is an expensive operation, so you definitely don't want to apply it to every single order. And yes, ingame orders including sync frames have to be validated as well because if the MITM drops the sync frames, the desync won't get noticed.

Doing 512 bit private rsa's for 10s: 235565 512 bit private RSA's in 9.99s
Doing 512 bit public rsa's for 10s: 3608567 512 bit public RSA's in 10.00s
Doing 1024 bit private rsa's for 10s: 86835 1024 bit private RSA's in 10.00s
Doing 1024 bit public rsa's for 10s: 1459472 1024 bit public RSA's in 10.00s
Doing 2048 bit private rsa's for 10s: 13616 2048 bit private RSA's in 10.00s
Doing 2048 bit public rsa's for 10s: 454778 2048 bit public RSA's in 10.00s
Doing 3072 bit private rsa's for 10s: 4373 3072 bit private RSA's in 10.00s
Doing 3072 bit public rsa's for 10s: 215525 3072 bit public RSA's in 10.00s
Doing 4096 bit private rsa's for 10s: 1920 4096 bit private RSA's in 10.01s
Doing 4096 bit public rsa's for 10s: 124290 4096 bit public RSA's in 10.00s
Doing 7680 bit private rsa's for 10s: 214 7680 bit private RSA's in 10.02s
Doing 7680 bit public rsa's for 10s: 36692 7680 bit public RSA's in 10.00s
Doing 15360 bit private rsa's for 10s: 40 15360 bit private RSA's in 10.03s
Doing 15360 bit public rsa's for 10s: 9337 15360 bit public RSA's in 9.99s
OpenSSL 1.1.0g  2 Nov 2017
built on: reproducible build, date unspecified
options:bn(64,64) rc4(8x,int) des(int) aes(partial) blowfish(ptr) 
compiler: gcc -DDSO_DLFCN -DHAVE_DLFCN_H -DNDEBUG -DOPENSSL_THREADS -DOPENSSL_NO_STATIC_ENGINE -DOPENSSL_PIC -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DOPENSSLDIR="\"/usr/lib/ssl\"" -DENGINESDIR="\"/usr/lib/x86_64-linux-gnu/engines-1.1\"" 
                  sign    verify    sign/s verify/s
rsa  512 bits 0.000042s 0.000003s  23580.1 360856.7
rsa 1024 bits 0.000115s 0.000007s   8683.5 145947.2
rsa 2048 bits 0.000734s 0.000022s   1361.6  45477.8
rsa 3072 bits 0.002287s 0.000046s    437.3  21552.5
rsa 4096 bits 0.005214s 0.000080s    191.8  12429.0
rsa 7680 bits 0.046822s 0.000273s     21.4   3669.2
rsa 15360 bits 0.250750s 0.001070s      4.0    934.6

So with 4k keys, a sign operation takes 5 ms and a verify operation 0.3ms on my machine. That time would be better spent on the game logic and/or rendering.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

ingame orders including sync frames have to be validated as well because if the MITM drops the sync frames, the desync won't get noticed.

If the MITM drops the sync frames then the game will stall and nobody will see the manipulated order stream. This is something that can already be exploited to ruin games (but is less effective than just spoofing an incorrect hash to trigger a desync), and doesn't have any relationship to player identity.

Also note that this PR uses 2048 bit keys (as recommended by NIST, and likely to be sufficient until 2030 - https://en.wikipedia.org/wiki/Key_size), so the performance overheads are a lot lower than your quoted numbers.

@chrisforbes

This comment has been minimized.

Copy link
Member

chrisforbes commented Jul 12, 2018

@pchote @jrb0001 has already raised the big issue I saw.

To mitigate the bulk, the client should sign a blob containing:

  • server nonce
  • client nonce
  • server ip

As a less serious (but easily fixed) matter, the nonces should be generated from a secure generator, and we should probably not use SHA1 in new designs in 2018.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 12, 2018

Ok, great. That will be a straightforward fix once I find time to work on it (probably not until I'm back in the UK).

@chrisforbes do you have an opinion on the discussion wrt MITM and signing chat etc to help mitigate malicious servers etc?

@chrisforbes

This comment has been minimized.

Copy link
Member

chrisforbes commented Jul 13, 2018

I'm less concerned about active MITM than I am about a player colluding with a server they control in order to fake auth against innocent servers.

Long term, let's look at TLS to pick up some of these other properties (or if we eventually go to UDP, DTLS)

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 13, 2018

Considering the usecase (showing some useless badges), the active MITM vulnerability isn't really a big issue imo. But we should be careful when adding new functionality based on it.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 14, 2018

After an IRC discussion @chrisforbes has offered to work on some of the security worries while I am away. If we aren't able to make progress by the time I am back and have resolved the other playtest blockers then we will have to move on without this.

@chrisforbes's main concern was a variation of what @jrb0001 offered: a malicious person A operates a modified client+server pair and manages to trick a player B to join their server, at which point A can pass them through to an arbitrary server C, authenticating as B while holding the ability to modify the connection traffic going both ways. Instead of trying to manipulate the traffic (which we can detect and defeat without any serious infrastructure changes) A can simply choose to cut the connection to B when the game starts (or at some later point) and then deliberately throw the game, making A look bad.

We reshashed most of the points addressed above, and IMO did not come to any new conclusions. We still have two options:

  • Require a full on network-layer encryption protocol implementation before allowing any kind of verified identity systems to be merged (or, more pessimistically: PR is rejected and give up on ever implementing it)
  • Add a requirement for players to sign chat / lobby orders / surrender orders, and to send a signed packet every N game ticks after the game starts. An innocent server could then detect and drop a malicious connection within a couple of seconds, preventing any serious harm. Protecting against malicious servers would require additional client-side changes to be able to modify the authentication state client-side and also store the key/profile data in a new section in the replay (because this data would now be required for replays - preventing offline playback)
@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Jul 14, 2018

Regarding the second option: Either ingame orders or sync packets (or both) must also be signed or an attacker can take over the player and, for example, sell everything. Also it would probably be better to sign/verify groups of orders instead of every single order individually to reduce the cost. And it should be moved onto its own thread to avoid stalling the main loop.

@chrisforbes

This comment has been minimized.

Copy link
Member

chrisforbes commented Jul 14, 2018

@jrb0001 the best I've got so far is to maintain a running digest of the outgoing order data, and sign it every few seconds. This is reasonably cheap to do.

@pchote pchote force-pushed the pchote:forum-authentication branch from f6415db to 6310999 Jul 28, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 28, 2018

Updated to fix my fingerprint calculation issue above. I dropped the commit enforcing unix-style newlines in the PEM output (no longer necessary), and changed the fingerprint calculation to directly hash the modulus and exponent, without any of the PEM wrapper junk.

The corresponding change on the backend is OpenRA/openrauseraccounts@76a50f9.

This changes the fingerprints for existing keys, so they will need to be readded to the forum.


var client = orderManager.LobbyInfo.ClientWithIndex(clientIndex);
var ping = orderManager.LobbyInfo.PingFromClient(client);
if (latency != null)

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

This check doesn't make sense to me. If latency is null, we'd have crashed a few lines before.

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

Nvm, this seems to be changed in later commits.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jul 28, 2018

budgebadges

  1. Login
  2. Reorder badges
  3. Logout
  4. Login again
@abcdefg30
Copy link
Member

abcdefg30 left a comment

Not sure if this is really relevant...
Also haven't looked at CryptoUtil.cs yet.

};

var ping = orderManager.LobbyInfo.PingFromClient(client);
if (latency != null)

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

Ok, this is the right spot to comment on:

This check doesn't make sense to me. If latency is null, we'd have crashed a few lines before.

if (ip == IPAddress.Loopback.ToString())
return "Local Host";
return "127.0.0.1";

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

I think this is now no different to just return ip; and could be dropped?


try
{
if (File.Exists(filePath))

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

Do we want to log an error message if this condition is not met?

This comment has been minimized.

@pchote

pchote Jul 28, 2018

Author Member

IMO no. This file not existing is an expected state (user is not / chooses not to log in).

try
{
var yaml = MiniYaml.FromString(Encoding.UTF8.GetString(i.Result)).First();
if (yaml.Key == "Player")

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

Do we want logging in case this condition is not met? (Or if i.Error != null?)

@pchote pchote force-pushed the pchote:forum-authentication branch from 6310999 to 0bb535c Jul 28, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 28, 2018

Rebased and added fixup commits that will be squashed after review.

@pchote pchote force-pushed the pchote:forum-authentication branch from 0bb535c to 66a5ef5 Jul 28, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jul 28, 2018

19:57 <+abcdefg30> the auth pr looks good to me now
19:57 <+pchote> abcdefg30: happy for me to squash the fixups down?
19:57 <+abcdefg30> yeah

Squashed and hopefully ready to merge.

@abcdefg30 abcdefg30 merged commit 6ec93bd into OpenRA:bleed Jul 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jul 28, 2018

@Micr0Bit

This comment has been minimized.

Copy link
Member

Micr0Bit commented Jul 29, 2018

🥇

@pchote pchote deleted the pchote:forum-authentication branch Jul 29, 2018

@jrb0001

This comment has been minimized.

Copy link
Contributor

jrb0001 commented Aug 10, 2018

Did I miss something or is the original vulnerability still there (which allows an attacker to use an evil server to "steal" a single authentication from any connecting user for any target server)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.