(In reply to comment #0)
> This was brought to my attention by polly on IRC.
> 
> Server operators can currently run a server to collect player guid's, and those
> of players who are admins on other servers. Players can then change their own
> guid to match the collected admin's guid and gain admin privileges on the other
> server.
> 
> A possible fix would be to use a public/private key combination. Each server
> has a public key and each client has a private key. The client receives the
> server's public key and combines it in a (practically) non-reversible manner
> with its private key and send the result back as the guid. This would work as a
> kind of one-way signed communication.
> 
I've been thinking about this too (and think it's a good idea), but you need to look up how public key cryptography works. The server could have a public/private keypair, but the main use of that would be to prove that the server really is the server you think you're connecting to (which isn't very useful for online games). The client would need to have a public/private keypair, and the public key would be published like cl_guid is, the server would generate some random string, store it, and send it to the player, encrypted with his public key, if the player fails to produce the original string in a reasonable amount of time, then the server should either ignore his guid, or kick him. (and before someone yells at me for it, yes the algorithm should use padding to keep someone from trying to get the unencrypted string by encrypting random characters and comparing it to the encrypted string)

(In reply to comment #1)
> The client would need to have a public/private
> keypair, and the public key would be published like cl_guid is, the server
> would generate some random string, store it, and send it to the player,
> encrypted with his public key, if the player fails to produce the original
> string in a reasonable amount of time, then the server should either ignore his
> guid, or kick him.
Sounds a lot like adding a handshake sequence using something like PGP. In that case, the server should also have public and private key pairs to ensure it is what it claims to be.

I don't know that that is a good solution, but it is a little inconsistent that the server gets trusted not to misuse guids, the only thing to guarantee you are who you say you are, but not with much else.

The easiest solution is to just regenerate the md5 digest using the server's IP:PORT in addition to the qkey string.  This would make the guid unique for each server.

The only caveat is that the when your server's IP changes all your admin definitions will be junk.  I assume most servers don't change their IP very regularly though.

(In reply to comment #3)
> The easiest solution is to just regenerate the md5 digest using the server's
> IP:PORT in addition to the qkey string.  This would make the guid unique for
> each server.

I doubt this is cryptologically secure, but how many cryptologists do we have playing anyway? ;)

I like this solution because we don't need to touch the server to implement it. The client simply reports a different guid to each server it connects to. Problem solved?

Created attachment 1253 [details]
svn1028 cl_guidServerUniq

This adds a cvar cl_guidServerUniq (defaults to 1).  If enabled, this will cause the cl_guid to be unique for each server IP:PORT.

This seems to work, but could probably use some testing.

(In reply to comment #5)
> This adds a cvar cl_guidServerUniq (defaults to 1).  If enabled, this will
> cause the cl_guid to be unique for each server IP:PORT.

BTW the reason I think this deserves a cvar is that it provides a server operator an option of having a "static" guid in the case where his/her server has frequent IP Address changes.  This person can leave cl_guidServerUniq disabled when playing on his/her own server, but then toggle it on when playing on untrusted servers.

Created attachment 1254 [details]
svn1038 cl_guidServerUniq

In order for this to work, the FS_SV_ file functions need to be used.  This places the "qkey" file in the main user dir instead of in the base dir.  This is important because otherwise fs_game could be something different and the qkey file will be missing.  This wasn't a problem before when the file was only read at init.

I've tested this pretty well now.

Created attachment 1257 [details]
svn1038 cl_guidServerUniq

* cl_guid will be set to "" if no valid QKEY_FILE can be read
* update cl_guid when disconnecting too
* QKEY_FILE is now checked for validity every time a cl_guid is generated
* add warning when QKEY_FILE is regenerated because of improper file size

added at revision 1041

(In reply to comment #4)
> (In reply to comment #3)
> > The easiest solution is to just regenerate the md5 digest using the server's
> > IP:PORT in addition to the qkey string.  This would make the guid unique for
> > each server.
> 
> I doubt this is cryptologically secure, but how many cryptologists do we have
> playing anyway? ;)
> 
> I like this solution because we don't need to touch the server to implement it.
> The client simply reports a different guid to each server it connects to.
> Problem solved?
> 

you know md5 of (username1+password1) and username1
can you get password1 or md5 of (username2+password1)
similarly you know md5 of (serverip1+qkey) and serverip1
can you get qkey or md5 of (serverip2+qkey)

both are possible but extremely hard
imho this is a safe method

another approach is creating qkey files per server which is not as good

srand(time(0)) is not good enough at initializing the random number generator for the qkey file generation.

Created attachment 1265 [details]
svn1045 Com_RandomBytes()


Adds a new function:

  void Com_RandomBytes( byte *string, int len )

This depends on the system-specific:

  qboolean Sys_RandomBytes( byte *string, int len )

On windows this uses CryptGenRandom(), on unix platforms it uses /dev/urandom.

If Sys_RandomBytes() returns qfalse, then the srand(time(0))/rand() implementation is used and a warning is printed.

I still need to test the win32 implementation and there are potential compatability problems on some of the unix platforms too.

Configurable prefix used to compute the MD5 serving as a GUID will still be a security hole. The only difference is that the attacked will have first to join the server it wants to hack to record the prefix used there before configuring his own server with the same prefix value.

As it is now, it is still possible to hack the system ( although highly unlikely ) if you can use a tool like tcpdump to capture the packets between your victim and the server. Although greatly limited over the internet, such hack method can still be used in more constrained environments like a big LAN sharing a single internet connection.

A good solution which wouldn't involve changing too much the client and the server consists in using the Diffie-Hellman key agreement system. Such system is considered secure against eavesdroppers as long as the keys are carefully chosen. And that way, server operators can still share a single server key between many servers and change the server IP.

http://en.wikipedia.org/wiki/Diffie-Hellman_key_exchange

Although the page says to discard a and b in the end. We want to keep them here since they are the new qkeys.

And for the implement you say? What about libcrypto, part of OpenSSL?
http://www.openssl.org/docs/crypto/dh.html

Created attachment 1266 [details]
svn1045 Com_RandomBytes()

works for win32 now, still need to test win98 though

(In reply to comment #13)
> Configurable prefix used to compute the MD5 serving as a GUID will still be a
> security hole. The only difference is that the attacked will have first to join
> the server it wants to hack to record the prefix used there before configuring
> his own server with the same prefix value.
> 
> As it is now, it is still possible to hack the system ( although highly
> unlikely ) if you can use a tool like tcpdump to capture the packets between
> your victim and the server. Although greatly limited over the internet, such
> hack method can still be used in more constrained environments like a big LAN
> sharing a single internet connection.
> 
> A good solution which wouldn't involve changing too much the client and the
> server consists in using the Diffie-Hellman key agreement system. Such system
> is considered secure against eavesdroppers as long as the keys are carefully
> chosen. And that way, server operators can still share a single server key
> between many servers and change the server IP.
> 
> http://en.wikipedia.org/wiki/Diffie-Hellman_key_exchange
> 
> Although the page says to discard a and b in the end. We want to keep them here
> since they are the new qkeys.
> 
> And for the implement you say? What about libcrypto, part of OpenSSL?
> http://www.openssl.org/docs/crypto/dh.html

I don't see how we can add any type of key exchange protocol without breaking the game protocol.  Breaking the game protocol conflicts with ioq3 goals I think.

Yes, it is possible to brute-force the qkey once you know the cl_guid a client uses when connecting to your server.  It would also be possible to snoop that information from a dump of UDP traffic I suppose.  These are excellent reasons for not ever using cl_guid for granting access to your credit card number, but I think it's "good enough" for providing access to some limited administration commands in the game code.

The current patch is for adding better randomization for qkey file generation.  As R1CH demonstrated, it can be very easy to brute force the qkey if the seed used for the randomization is narrowly limited (e.g. time(0)).

Oh well, you're right, it would require big changes in the client/server connection protocol. I guess that's what you get for posting things when you are tired.


But at least, the libcrypto could still be a good idea since it has the code you need to generate keys in a secure way, and it also all the public+private key routines you need to make the system secure and not be dependent on the server current IP:Port :p

(In reply to comment #16)
> But at least, the libcrypto could still be a good idea since it has the code
> you need to generate keys in a secure way, and it also all the public+private
> key routines you need to make the system secure and not be dependent on the
> server current IP:Port :p

Anything we could add in this regard would be less secure that using IP:PORT since the server's private key could be brute forced.  Then an impersonating server could be set up to harvest client's cl_guids without the need to brute force them.

I guess it's just a question of whether or not it's worth increasing ease-of-use at the cost of having even less security.  IMO IP:PORT is fine since most servers don't change IP address very often.

That said, I suppose it's worth considering this, maybe as a server-side setting.  Such a keying system could be created using the existing qkey/md5 functions:

Client sends:

getchallenge ... md5(part of qkey)

Server sends:

challengeResponse ... md5(string from getchallenge + part of server's qkey)

If server sends back a string at the end of challengeResponse, the client
sets cl_guid as:

md5(string from challengeResponse + other part of qkey) 

Otherswise, it uses IP:PORT as the prefix instead.

At least I _think_ this could be added without breaking any compatability.

Wait wait wait! I can agree that the MD5 of IP:Port+QKEY is probably secure enouth for what we want, but I have to say that I have MUCH more faith in the security of a public/private 1024bit RSA key pair than all the MD5 hashes you can make.

Also, MD5 is considered flawed and potentialy breakable for quite some time now. If you had used SHA-128 or SHA-256 I would have been better.

(In reply to comment #18)
> Wait wait wait! I can agree that the MD5 of IP:Port+QKEY is probably secure
> enouth for what we want, but I have to say that I have MUCH more faith in the
> security of a public/private 1024bit RSA key pair than all the MD5 hashes you
> can make.

Seems like overkill to me.  Not to mention that the strings required for 1024-bit RSA may not fit nicely in the "getchallenge"/"challengeResponse" messages.

> Also, MD5 is considered flawed and potentialy breakable for quite some time
> now.

IANAC (I am not a cryptographer), but I don't think that the well-publicized collision vulnerabilities have any effect on our use of MD5.  It doesn't make brute forcing an MD5 hash any easier, and this step would still be required.

> If you had used SHA-128 or SHA-256 I would have been better.

cl_guid must be a 32 character string in order to play nice with the one that is available in the id's Quake 3 client (via punkbuster).

Created attachment 1267 [details]
svn1045 Com_RandomBytes()

* removes NULL-termination in Com_RandomBytes()
* uses correct byte instead of char for buffer in Com_MD5File()

I've  tested this on win98se too and it seems to work fine.

(In reply to comment #20)
> Created an attachment (id=1267) [details]
> svn1045 Com_RandomBytes()

Applied at revision 1046

I'm closing this bug, the keying stuff belongs in a new "enhancement" ticket I guess.

Setting a QA contact on all ioquake3 bugs, even resolved ones. Sorry if you get a flood of email from this, it should only happen once. Apologies for the incovenience.

--ryan.