Hide modding-related types from TypeHelper.FindType #67
Conversation
|
there should be a nml config option to disable this feature, maybe "unsafe_types" |
Implemented now as a |
|
ideally this harmony patch would not run in LocalWorlds, namely userspace. a potential way to implement this would be to check what thread the FindType() method is being called on and compare that to a list of whitelisted localworld threads |
| { | ||
| // an assembly was in neither neosAssemblies nor modAssemblies | ||
| // this implies someone late-loaded an assembly after NML, and it was later used in-game | ||
| // this is super weird, and probably shouldn't ever happen... but if it does, I want to know about it. |
There was a problem hiding this comment.
wont any plugin loaded after nml is loaded cause this?
There was a problem hiding this comment.
No, plugins are loaded before NML starts executing.
There was a problem hiding this comment.
maybe we could add another config option to allow late loaded types thru our filter.
There was a problem hiding this comment.
That's doable. I'm just not sure who'd be late loading types at all, hence the warning log.
| private static HashSet<Assembly> GetNeosAssemblies(HashSet<Assembly> initialAssemblies) | ||
| { | ||
| initialAssemblies.Remove(Assembly.GetExecutingAssembly()); | ||
| initialAssemblies.Remove(typeof(Harmony).Assembly); |
There was a problem hiding this comment.
should probably add a comment here clarifying why a specific case is needed.
afak in theory, if nml is the only thing a user is using that loads harmony, and the user followed the install instructions of placing harmony in nml_libs, initialAssemblies shouldn't contain harmony.
There was a problem hiding this comment.
Comment added in f9c6f22. The install instructions weren't always to use nml_libs, and I don't want to punish users who followed the old steps.
Offhand I think all the other config options have false as the default behavior of nml. I think the name should be adjusted to follow that for consistency as hiding types should be the default. |
Do we know for a fact that threads aren't in a pool that handles multiple worlds? |
There is another config that defaults to true. /doc/modloader_config.md The reason I named this |
AllowUnsafeTypeCheck, AllowUnsafeModTypeChecks or similar could work but just figured I'd mention it. |
There was a problem hiding this comment.
While this seems okay, it should be noted that fully hiding anything in Neos from remote clients is hard to do properly. Even with this, I'm pretty sure there is still at least a possibility of a theoretical timing based detection approach, given a dedicated enough attacker.
It turns out #67 did not cover all cases. In this PR, `WorkerManager.IsValidGenericType` and `WorkerManager.GetType` are now both handled. I've made the code a little more modular as well now that three distinct methods are being hooked in very similar ways.
This fixes #64.
The core idea in this PR is right as NML is spinning up, I record all currently loaded assemblies, which I call
initialAssemblies. Harmony may or may not be loaded yet depending on if the user has it in nml_mods or not. SoinitialAssemblies- NML - Harmony isneosAssemblies. Any assembly not inneosAssemblieswill be hidden from Neos's main string-to-Type conversion function,TypeHelper.FindType().As an additional safety check, I calculate assemblies that belong to mods as well. If a type belongs to neither
neosAssembliesormodAssemblies, that's an indication that a type was loaded very late (after NML) but it's being used in game. That's pretty weird, so I have a warn log for it.