garrysmod
garrysmod copied to clipboard
Facepunch
Replaced usages of 'Player.UniqueID' with 'Player.AccountID' function
Brief explanation:
- AccountID is unique (increasing) number for each individual Steam account, it is also known as Steam3 ID ("U:1:N"), where N is the AccountID number (it is 32-bit unsigned integer). (Note, it is safe until integer overflow occurs, it is not half way there yet.)
- UniqueID is not a unique identifier (blame Garry), please refuse from using it in future, it is equivalent to
util.CRC("gm_" .. SteamID .. "_gm"). Thus, there are collisions between different SteamIDs (accounts). - This is to be treated as a rare security exploit, because it was abused in the past - and still can be. (Note, there is a way to force UniqueID in a way, but it goes beyond the scope of this brief explanation...)
It is highly recommended against using Player.UniqueID function in your future code (especially for administration and prop-protection purposes).
Some addons were already patched (yesterday), and some more are still awaiting to be patched but after this is merged (i.e. evolve's ban).
There are several things to patch here:
- Player:UniqueIDTable(key) (
g_SBoxObjects[UniqueID])
If your addon uses this function, your code is most likely not affected (requires no changes).
- numpad's internal table indexing (
GetPlayerIndex)
If your addon uses numpad library, your code is most likely not affected (requires no changes).
- undo.GetTable()[UniqueID]
If your addon uses this function, then switch the indexing value to
Player.AccountID.
- cleanup.GetList()[UniqueID]
If your addon uses this function, then switch the indexing value to
Player.AccountID.
- Entity.FounderIndex or Entity:GetVar("FounderIndex") or Entity:GetTable().FounderIndex
If your addon uses this variable, then its value will now be equal to
Player.AccountIDinstead.
Backward compatibility is void (if you plan to do any kind of conversion from/to UniqueID, it will persist the vulnerability too, so don't do it); for the greater good.
Backward compatibility is void
Well you just gave the reason why this will never be accepted.
It shouldn't be hard to add conversation code for the database (I'd prefer entries to be deleted over making the entire database invalid).
Ideally the database is versioned so that you'd only have to increase the version for this case and provide an upgrade path (removing entries matching the format for example), one can dream, no?
Breaking backward compatibility should be an accepted solution regarding possible exploits and security issues.
And beyond that database issue I don't see any persistent data that relies on UniqueID.
Unmaintained software shouldn't get any more support in the first place, but wasn't it the case that TTT changes should have their own PR anyway?
Well you just gave the reason why this will never be accepted.
Do you want me publish the collision list of all SteamIDs (and their UniqueIDs)?
Wonderful things start to happen due this vulnerability when collision occurs on the server, especially for evolve (you can't ban the twin player). Those players start to share their undo/cleanup tables, prop ownership, and so on.
It is a shame UniqueID has been documented too lately. Devs continue on using it unknowingly...
"hey I'll just use UniqueID because of its name"
Everything about this function is just wrong, I should also mention that CRC computation can have bad impact on server performance if performed in a loop in a Tick hook (due no caching...).
Breaking backward compatibility should be an accepted solution regarding possible exploits and security issues.
Yeah, but for this particular issue, it doesn't make sense to have backward compatibility (i.e. PData SQL), because if you do that, then it would keep the vulnerability for collided IDs. The best thing to do, is just clear out the playerpdata table, as I already suggested in OP. And luckly, I didn't find any (popular) addons which use PData too :D
I think since this can have security impact on the server, it should have higher priority.
It is a shame UniqueID has been documented too lately. Devs continue on using it unknowingly... "hey I'll just use UniqueID because of its name"
Is the giant warning on the wiki page to not use the function not enough?
Is the giant warning on the wiki page not enough?
Well, I suppose not everyone read wiki (addons which are still working before wiki were updated). But, I'm not here to talk about 3rd-party addons (such should be forked/updated by somebody in community). Here we see GMod Lua itself is using UniqueID... Yeah, and that's an irony. The fact is, it is a rare problem, don't ignore the issue, it may seem a lot to take.
A lot of addons use PData, a fucking lot.
Oh, PointShop is one of them (v1, not sure about v2).
Well, if I revert PData, then players with the same UniqueIDs will share the same inventories/stats (and whatever).
As I already explained it a couple times (see previous comments)... It is best to just clear out the playerpdata table, very unfortunately.
It is problem for PointShop and other affected addons which rely on UniqueID functionality. It is their problem honestly, we can just hope for them to push an update, but PData (i.e. inventory items) would be gone, starting from the scratch. It is very frustrating, but yeah 😢
Revert this.
I won't be reverting anything here.
I simply wish Player.UniqueID was never a thing, yet here we are, thanks to whoever coded it (Garry) 😢
How would you fix such a vulnerability? Do you have a better solution than this?
This wont be accepted then. Backwards compatibility is very important for the devs.
This is just fucking with everyones addons.
Well. Postpone this for the next major update ("GMod 14")?
Actually since it would be major update, the better thing to do would be to remove Player.UniqueID function completely.
PData SQL is down to server devs to manually fix up their database for such colliding players, which is pretty painful since you can't tell whose inventory it really is (2 players - 1 inventory)...
GMod is literally dooms day for those players :(
It's unlikely GMod get's a major update anytime soon, especially an update breaking compatibility.
Alright. So, a new PR for TTT changes? By the way, since evolve is not maintained anymore, it would be wise to remove it's banning function from TTT. Do you agree?
Is it breaking the game?
If the server is using evolve, yes. Let me just cut it for you:
Wonderful things start to happen due this vulnerability when collision occurs on the server, especially for evolve (you can't ban the twin player).
Because once you ban the first player, his/her UniqueID is added to the banslist, and now if you try to ban the second player which has the same UniqueID as the first (due collision), it will fail to ban. Simple as that.
What a joke of a PR.
Not.
The devs really should have deprecated and removed UniqueID years ago. That said, the probability of a clash is astronomically low.
Just leave Evolve how it is. People rarely use it, but compatibility and support should be maintained. Also leave the uqid key how it is for backwards compatibility.
I dont understand how a game is supposed to support admin mods in the first place. In my opinion, there should be a hook or the default gmod behaviour should have been used.
I have addressed a fix for singleplayer game, AccountID is returning no value, and for bots too.
It should be all good now.
I had to remove most of the changes as they would conflict with existing addons. The only 2 safe places to change this, I have also changed to use SteamID64, as it is actually unique for every possible player, including bots.