garrysmod-issues
garrysmod-issues copied to clipboard
Facepunch
File.write() can be (and is) used by server operators to crash game clients
Details
(I apologize if my terminology is off, it's been a while since I've done dev work on GMod)
This is an old bug that has existed for at least half a decade. A server operator with malicious intentions can crash any client they wish (and even fill up their hard drive or destroy their SSD) by spamming the file.Write() function.
The way this exploit works is simple: The server operator sends some Lua to the client, usually in the form of a script that waits for something to be networked, or like some sort of ULX command... the specific activation isn't really relevant here. This script when activated will activate an infinite loop (or close to infinite), where it will execute file.Write("n.txt","something"), where n is an incrementing number typically starting at 1.
file.Write() conveniently is a blocking call, meaning the game will freeze momentarily while file.Write() does it's thing. So if you consider that this function is being called constantly (sometimes in a while true loop), the game will freeze entirely (but does not fully crash, rather the game becomes completely unresponsive until manually terminated by the user).
In the mean time, while the application is still frozen, it is constantly writing small files to the user's hard drive (usually only a few bytes in size). If using an SSD, this can be detrimental to the user's hardware due to the nature of SSDs (limited read/writes). This could be seen as malicious intent by the server's operator.
Steps to reproduce
- Add this following script to the server as a client-side script (so cl_autoexec.lua or some shit)
function crashMyShit()
i = 1
while true do
file.Write(tostring(i) + ".txt","get crashed kiddo")
i = i + 1
end
end
concommand.Add("crash_my_shit",crashMyShit)
- Either start up a single-player game or join your server running this script.
- Run
crash_my_shitin the client's console. - Watch your
garrysmod/datafolder fill up with junk text files
Recovery for those who are testing
If for some reason you want to delete the text files from your data folder without getting any other text files that were not written by this script, I have a powershell command that will do that for you...
$list = Get-ChildItem *.txt | Where-Object { (Get-Content $_) -match "get crashed kiddo"}; foreach ($line in $list) {Remove-Item $line}
Run that in the context of the data directory and it will delete any file that was written by the above script, one at a time (mainly because windows itself is very slow at finding all the files it needs).
This is already known, just as its already known you can crash/freeze the game or ddos people's networks. Your best defense is to play on servers not owned by asshats. Or set your data directory to read only.
So your argument to "this can be used to crash players and harm their computers" is "dont play on those servers lol"?
I can't tell if a server is going to be owned by an asshat until they prove themselves to be asshats. And while Kefta raises a semi-valid point that you can still crash clients with while true do end, that doesn't damage their hardware like an endless loop of file.write can.
I'm proposing that file.write be internally rate-limited, to prevent it from literally writing a file every time its called. Something like a max rate of 5 or 10 calls per second maybe? This would severely negate the potential damage done to the client's hardware as a result at the very least. Surely it would be irresponsible to leave such a malicious action available to server operators, lest we have a reoccurance of HeX and his ban phone or whatever autism he used to do.
Fyi @thegrb93 I had already set my data folder to read only, however this did not seem to make a difference.
This would break scripts that write a lot to the data/ folder at once, ex. AdvDupe. Abusive server scripts could just call it at the max rate and still fill up your disk over a longer period of time.
Even so, a rate limit would do less harm than good. In the current state, less than 2 minutes of execution time is equivilent to about 100,000 files. All I'm suggesting is we curb that lower so you couldn't possibly write more than 5,000 files in that time. Any script that has a high I/O load that is using that I/O for LEGITIMATE reasons would still not come close to writing more than 5,000 in the span of 2 minutes, whereas my afformentioned point that THIS CAN DAMAGE CLIENT HARDWARE would be immediately mitigated significantly against any MALICIOUS activity.
Alternatively, earlier it was mentioned that making the directory read only was a bandaid fix for this issue. Perhaps making this option more accessible to the user (ie a client setting in Options that prevents Lua from writing files as if the directory was read only) would be a solution. Ultimately there really is no excuse to leaving something in place that has the likely potential to damage a client's hardware. Should widespread use of this bug occur for malicious intentions, I believe Steam would have grounds to remove the title from its store should no action be taken to mitigate it by the developers.
Writing files doesn't damage hardware. If you're talking about SSD wear, that's pretty dumb. Your internet browser's cache does a lot more damage.
At this point it's easier to implement a permission system that exposes tags (or keyvalues for matchmaking) for a set of things that the player should consent with before being able to join, among this writing to the local drive (file.Write and general downloads being separated from each other) or sending lua chunks to a client
But it's as unrealistic as rate-limiting things in a way that older scripts wouldn't break. An easy solution to your "damage hardware" point would be to move the data folder outside your SSD to an HDD, or maybe even a RamDisk.
The issue remains that the server owner has the authority in the server-client model, so if he can't be trusted you still have a decent list of other servers to choose from.
Also, to avoid an exploit being used widespread you shouldn't have made the issue public (as in, used e-mail instead as instructed on the README or CONTRIBUTING files)
Why not buffer the file.Write() function behind a user confirmation like how visiting URLs with the steam browser works? Maybe with a "don't show messages like this again on this server" checkbox option as well so user can opt into the current functionality on the server?
This could break some things because it would make the function asynchronous, but it's really the only solution I can think of that would protect users as well as let it continue to work less limited.
Another issue that would be solved if GMod had a clientside permissions system. There should be a milestone added at this point.
Allow this server (ip: 420.420.420.420) to get your mouse input.
- Allow
- Allow Once
- Never
You're assuming the layman who plays gmod would understand the implications of allowing permissions. Most people would probably deny and then 'oops, addons no longer work for you because you denied permissions' or vice versa they click allow without reading them.
Or allow currently allowed features by default, and only ask for new permissions (running blocked concommands and such).
There are a lot of issues where a permissions system is outside of its scope yet could still all be cumulatively solved by one. Considering there's no alternative solution besides ratelimiting file.Write, it's worth mentioning as it's the best and only solution at the moment.
There's already two solutions posted earlier. Bringing up a huge overhaul over a harmless game freeze is not a good solution.
The only other solutions mentioned were giving the user a way to mark the folder as read-only in-game, or asking them confirmation before using the function, both of which sound like a... permissions system.
What I want to have is cl_downloadfilter to affect MountGMA, so PAC3 won't load me any models or textures
@Kefta the other solution was not playing on that server.
@GitSparTV There's a convar in pac3 to disable custom content.
Not playing the game/server is not a solution. And filling people's harddrives is not a "harmless freeze".
Well it's annoying at worst. Wiping players data folders is far more harmful.
Like robotboy said, you can't simply say "don't play on that server lol" because this bug causes your game to freeze while it's filling the client's hard disk. You cannot leave the server unless you have sufficient knowledge as to be able to open task manager and kill the process. Sometimes you can't even do that, due to the frozen gmod process drawing over the Task Manager window, in which case you either need to know how to kill it in task manager without seeing what you are doing, or you need to know the command line arguments of taskkill (or it's linux/mac counterpart).
Additionally, you cannot possibly know what servers have this functionality and you definitely could not know which servers have operators that will utilize this functionality prior to joining the server and learning more about it's operators. It doesn't fully apply here, but I'm going to paraphrase FPtje here. When the discussion of ABH/Bunnyhopping and whether it should be patched or not, a number of users wished for it to remain, claiming that servers/gamemodes that did not wish for it to be exploited should patch it themselves, that it should not be a mainstream change. FPtje argued that it is a bug, and it always has been. On that definition alone, it should be patched, not left in place because of nostalgia or because people have gotten so good at using it.
No functionality of Garry's Mod should be able to intentionally (and maliciously) cause a remote client to crash. And even less functionality should be able to cause malicious damage to the client's hard disk or filesystem.
Well it's annoying at worst. Wiping players data folders is far more harmful.
This is actually a good point, but a bit outside of the realm of this issue. Hypothetically, what sort of solution could be put in place to protect client data from malicious scripts or server operators? Off the gate I'm thinking some sort of virtual FS where data is stored seperately on a server by server basis (see the way browsers handle cookies (?) ), however this would harm backwards compatibility, especially with things like Advanced Duplicator 2 or Wire's Expression 2. I'd love to brainstorm a potential solution to the possibility that a server could maliciously wipe client data...
Also, to avoid an exploit being used widespread you shouldn't have made the issue public (as in, used e-mail instead as instructed on the README or CONTRIBUTING files)
I've learned that if you wish to get anything done with this game, it's important to bring it to public attention. Take the issues with fake redirect servers in... I think it was 2016 or 2017? One particular server operator continued to abuse cheap cloud servers in other regions that would falsely report the player count of another server, but the correct ping for the cloud server. When clients would connect to a server they believed to be a low ping server, they'd be sent to a server in another part of the world. Nothing was done when users were reporting this silently, so I make an expose on the forums revealing this server's behavior. Within 72 hours something had been done: the servers were blacklisted. I still have memes about it to this day.
I feel the likelyhood that exposing this bug will cause the issue to be taken seriously and patched, vastly outweighs the likelyhood that malicious actors will see this issue, and then utilize the exploit to crash users.
This isn't a bug, it's a feature that can be abused. It's also been well known for years. No need to type an essay defending your position when the devs are already in agreement.
I feel I have to defend my position when the majority of people commenting are not in support of my position. But regardless of that I just want to see this fixed so it cannot be abused going forward.
pastebin.com/VvJdJKNn fixed version.
I managed to brick my friends game, if you write multiple files, hundreds it can harm the players computer. If Garry Newman was so scared of players having there data public (by server) then idk why he would add something witch allows people to brick your game.
