search

Facepunch / garrysmod-requests

Feature requests for Garry's Mod
83 stars 24 forks source link

Change file.Write's extension whitelist into a blacklist #1552

Open Kefta opened 4 years ago

Kefta commented 4 years ago

As I suggested in https://github.com/Facepunch/garrysmod-requests/issues/1262,

You can use RegQueryValueEx in Windows.h with the "Content Type" key to detect if the file is an application or executable - you could allow any non-executable to be written and have executables refer to the whitelist.

For Linux, you can use stat in POSIX, but you could honestly hardcode the blacklist for that without much worry.

If this really won't be done for whatever reason, here's a list of extensions that the whitelist should allow that currently aren't that should cover almost all use cases:

Also, why not allow extensionless files?

ExtReMLapin commented 4 years ago

The issue with a blacklist, is there always be something that will be left out and later leads to security issues.

Kefta commented 4 years ago

Of course, but all of those security issues boil down to server owners telling clients to go to their data folder and run something. I believe if the standard executeables are blocked (.exe, .jar, .bat) then the benefits outweigh the potential cons.

Kefta commented 4 years ago

Of course, this could all be resolved if GMod had a proper permissions system.

XAYRGA commented 4 years ago

In my opinion, this isn't a good idea.

Different people / different computers have different file associations. The whitelist covers only formats that are not executable, which is fine for its current purpose.

If in the event somebody has a file associated with something that treats it as an executable form of data, this is potentially a security flaw. So now if a shell execute exploit is found in addition to this (Usually, not difficult) not only would people have a chance to WRITE data, but they would have a surface of which to execute it, too. If an exploit of the like is found now, somebody can execute existing data, but can't really do anything terribly useful with it, as they can't make their own data to execute. Even if you use RegQueryValueEx, or stat as you mentioned, it would really be more work than it's worth, especially if it can't consistently be implemented over multiple systems.

Nothing is going to stop people from being curious and poking around in their data folder and opening things, either.

You shouldn't need more than .dat or .txt, most things in Garry's Mod already have some form of MiME type detection (Bass, Material(), etc). Usually, your addon will write its own data in its own format, and it should be the only thing that needs to know that.

TLDR: Not all computers treat data / extensions the same. This defeats a layer of security if added.

Kefta commented 4 years ago

Even if you use RegQueryValueEx, or stat as you mentioned, it would really be more work than it's worth, especially if it can't consistently be implemented over multiple systems.

It's a standard Windows function that's implemented on every system already, and can detect custom executable types if an association has been made with a program.

Kefta commented 4 years ago

You shouldn't need more than .dat or .txt, most things in Garry's Mod already have some form of MiME type detection (Bass, Material(), etc). Usually, your addon will write its own data in its own format, and it should be the only thing that needs to know that.

BASS does but GMod does not utilise it - sound files must be .ogg, .wav, or .mp3. util.IsValidModel requires extensions to end with .mdl, and it searches for its hardcoded components with .ani, .vtx, etc. Even if the whitelist isn't removed, the custom extensions are absolutely needed.

XAYRGA commented 4 years ago

You shouldn't need more than .dat or .txt, most things in Garry's Mod already have some form of MiME type detection (Bass, Material(), etc). Usually, your addon will write its own data in its own format, and it should be the only thing that needs to know that.

BASS does but GMod does not utilise it - sound files must be .ogg, .wav, or .mp3. util.IsValidModel requires extensions to end with .mdl, and it searches for its hardcoded components with .ani, .vtx, etc. Even if the whitelist isn't removed, the custom extensions are absolutely needed.

Now this, I can absolutely get behind.

I think possibly an extended whitelist would be a healthy change for the game, but not a blacklist.

Honestly the rule of thumb should be if the game can load it as assets then it should be white listed.

robotboy655 commented 3 years ago

Added .xml, .csv, .jpeg, .mp3, .wav and .ogg file formats to file.Write whitelist

robotboy655 commented 3 years ago

I have no intention of adding the sound format aliases, the game won't be able to recognize them anyway, and they are pretty rare, I for one have never seen any of those.

Similar thing goes for .bsp, .lmp and .properties - they are useless in the data/ folder.

Benn20002 commented 3 years ago

When are we going to get .mdl etc.? Or is there any reason not to add them?

robotboy655 commented 3 years ago

I am not sure they are load-able from data/, the game/engine relies on the model beginning with models/ in a lot of places I imagine.

Benn20002 commented 3 years ago

I just tested it and it seems to work totally fine (visuals/physics) except that it doesnt load materials. Using e:GetMaterials() only shows 1 = ___error even if the model has several materials.

Benn20002 commented 3 years ago

Also util.IsValidModel returns always false when using a model from the data folder.

thegrb93 commented 3 years ago

@robotboy655 Please document all the writable filetypes on https://wiki.facepunch.com/gmod/file.Write . I don't know all of them.

robotboy655 commented 3 years ago

They are documented. New types will be added when the update ships.

Benn20002 commented 3 years ago

Any chance we'll get .mdl and its dependencies? Dont wanna push you guys just wanna know if its worth to wait for or if it wont come anyways.

VaasKahnGrim commented 3 years ago

I am not sure they are load-able from data/, the game/engine relies on the model beginning with models/ in a lot of places I imagine.

mdl files would be useful for storing premade meshes for use with the mesh fnctions. Textures have to be applied manually anyways so the thing about not loading textures from it is not TOO bad

Vurv78 commented 2 years ago

Any chance of this happening anytime soon? Really annoying having to rename everything and in the case of working with special file formats having double extensions like foo.bar.txt (working on a programming language). Especially when there seems to be no harm in the implementation..

Benn20002 commented 1 year ago

I think its very annoying there is no response by the dev(s). Maybe just tell us whats preventing you from implementing it instead of just leaving us in the dark.. I'm sorry to say it like this but I am waiting for about two years now without any response.

ghost commented 1 year ago

I think its very annoying there is no response by the dev(s). Maybe just tell us whats preventing you from implementing it instead of just leaving us in the dark.. I'm sorry to say it like this but I am waiting for about two years now without any response.

facepunch have been completely radio silent on gmod for months now, the silence from them is not exclusive to this issue at all unfortunately :(