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

Harden plugin loading path requirements #1437

Merged

Conversation

peace-maker
Copy link
Member

Restrict loading of plugins to the sourcemod/plugins folder and require the .smx file extension.

Symlinks inside the plugins folder are fine. Apply the same path restriction to extensions as well.

This behavior was abused as part of justCTF 2020 in the PainterHell challenge by cypis. Thank you!

Restrict loading of plugins to the `sourcemod/plugins` folder and require the `.smx` file extension.

Symlinks inside the `plugins` folder are fine.

This behavior was abused as part of justCTF 2020 in the PainterHell challenge by cypis. Thank you!
Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nit in code you didn't touch, but is a crashy bug.

core/logic/PluginSys.cpp Show resolved Hide resolved
@naydef
Copy link
Contributor

naydef commented Mar 7, 2021

I'm afraid forcing plugins to have .smx extension will break almost all Freak Fortress 2 subplugins, because of this line

Working around this is doable i believe, just letting you know about that.

@dvander
Copy link
Member

dvander commented Mar 7, 2021

Can you explain what this code is trying to do by using a ".ff2" extension?

We believe restricting plugin loading to a single extension is a security improvement, since it'll be harder to load maliciously crafted files. If we were to instead make an extension whitelist, it'd be harder to coordinate that list between GSPs and server operators. ".smx" has been our standard for well over a decade now.

@naydef
Copy link
Contributor

naydef commented Mar 7, 2021

Well, it's a fact that Freak Fortress 2 will only load subplugins with .ff2 extension(that's fixable). Plugins identify themselves and other plugins using this_plugin_name variable, populated by GetThisPluginName(), which contains the plugin path with the extension stripped, for ex. addons/sourcemod/plugins/freaks/default_abilities. What this variable will contain now is addons/sourcemod/plugins/freaks/default_abilities.smx. Honestly, modifying Freak Fortress 2 in a way not breaking subplugins after this change is probably doable, but I don't know whether others(subplugins) have relied on the fact that ff2 subplugins have .ff2 extension.

Edit: Probably the price of merging this will not be considerable, anyway. Several plugins and subplugins should be modified and recompiled, but not sure how many.

Edit 2: I believe there's a small misunderstanding here about the real issue, but whatever. Plugin authors relying on plugin extension in a special way will be affected, but this is outside the reach of FF2 maintenance.

Have a nice day!
Naydef

@KyleSanderson
Copy link
Member

Well, it's a fact that Freak Fortress 2 will only load subplugins with .ff2 extension(that's fixable). Plugins identify themselves and other plugins using this_plugin_name variable, populated by GetThisPluginName(), which contains the plugin path with the extension stripped, for ex. addons/sourcemod/plugins/freaks/default_abilities. What this variable will contain now is addons/sourcemod/plugins/freaks/default_abilities.smx. Honestly, modifying Freak Fortress 2 in a way not breaking subplugins after this change is probably doable, but I don't know whether others(subplugins) have relied on the fact that ff2 subplugins have .ff2 extension.

Totally understood. We're presently doing the exact same thing, I use subfolders to break out requirements (but still use the .smx extension, with symbolic links - which still works with this change). The great thing about this is SM already handles this case built-in, so I think the impact to ff2 here is quite small (and this is a huge quality of life improvement where other files are now ignored, and you won't get owned by arbitrary data in other folders). Valve already has a deny-list built into many games, where smx is covered but I don't believe ff2 is so this should help you there as well.

Making changes like this a decade+ later always hurts someone, but in this case it's a rename of the files impacted. While this isn't my change, if you would like I'd be more than happy to help with reviews in ff2 or similar. If you change to a prefix, as an example, you likely won't get hit by other changes if there's glitches (readas: exploits) found in the future. I also believe the path is visible from GetPluginFilename (so in your case you could likely lean on the freaks folder if this is the standard for your distribution of software).

@KyleSanderson KyleSanderson merged commit 6ea1e39 into alliedmodders:master Mar 7, 2021
@peace-maker peace-maker deleted the harden_plugin_loading branch March 7, 2021 23:14
naydef added a commit to 50DKP/FF2-Official that referenced this pull request Mar 8, 2021
Some preliminary changes, while I fully consider the new backward compatibility code.
[WIP]
Batfoxkid added a commit to Batfoxkid/FreakFortressBat that referenced this pull request Mar 9, 2021
Different method from Naydef to try to preserve the fact that plugins aren't loaded in pre-round or when FF2 is disabled by only renaming them to .smx for the duration they are loaded.

See 50DKP/FF2-Official@3bd8c04 for more details on FF2-Official's commit and alliedmodders/sourcemod#1437 about plugin ext changes.
naydef added a commit to 50DKP/FF2-Official that referenced this pull request Mar 10, 2021
The string comparison might be unreliable if there's ".smx" text in the middle of the plugin name...
simmsb added a commit to simmsb/sourcemod that referenced this pull request Jun 27, 2021
…rs#1437)"

This isn't actually gonna be exploited ever lol

This reverts commit 6ea1e39.
naydef added a commit to 50DKP/FF2-Official that referenced this pull request Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants