BetaZavr / CustomNPCs_1.12.2-Unofficial

CustomNPCsUn
9 stars 3 forks source link

Remote Code Execution Concerns. #6

Open somehussar opened 3 months ago

somehussar commented 3 months ago

Concerns

I'm writing this post in good faith that this is simply an oversight or perhaps lack of knowledge on how to implement this more securely. I will be only talking about the client-side of things, as server owners are entirely responsible of entrusting wrong people developing scripts for them and it would be the server owner's fault if they allowed a script to essentially delete all server files.

While it is possible you may have meant that those security concerns should affect only the server, in your README and CurseForge page for the mod, you do mention you are aware of this issue: (quote from the README)

... But also for the client. The coder is also given the opportunity to process packets, reflection, work with files, etc. Therefore, pay close attention to who you give the right to develop scripts on your projects!

While the possibility to run scripts on the client is amazing in theory, the way the mod currently implements this is not ideal and violates many safety concerns.

My main squalm are the reflections and file access. Like I mentioned in the beginning, I'd like to remind you I'm talking about the client-side ONLY, as server-side script execution is entirely on the server owner's shoulders.

I do understand entirely the interest of allowing seasoned scripters the ability to use reflections and Java classes, however that completely breaks the informal "trust policy" modders and mod users have. I believe that as modders we should strive to cut-down on security problems that mods we create would be the root problem of.

As it stands, if you happened to join a server whose owner decided to include malicious scripts for the client, you leave a good chunk of your system compromised, solely because you might've missclicked or the server itself was backdoored to spread malicious code to users.

Before writing this, I have looked over the code as well as double-checked this security concern by testing (and successfully managing to execute scripts that could be malicious if misused) in a secure environment

Possible solutions.

Nashorn defines the idea of a ClassFilter, although it is not unique to it at all. LatvianDev's port of Rhino seems to also support a system similar to this.

Simply hard-coding one, even if it may allow all classes to be loaded through the usage of Java.type, already stops the ability to use reflections entirely. But you should most likely go one step further. The class filter should, at the minimum, stop access to Java's native IO & Networking classes, but it would be probably ideal if it blocked ALL classes from being accessed through Java.type.

This ensures that only objects, and the objects returned by methods of those objects provided during engine creation can be used on the Client's Script Environment.

Other notes

  1. On top of what was previously said, the objects passed in to the engine during creation (API objects for example) that are supposed to be wrappers for Minecraft objects should be heavily encapsulated (at the very least to a point). The scripts themselves should never have to interface with the raw Minecraft/Forge class, that interaction should be handled by wrapper classes.
  2. I do understand that providing wrappers for all Minecraft classes would take a lot of effort, for that reason I believe the NpcAPI or ScriptContainer class should be given a static Map<String, Object> engineObjects object, that defines a way to extend scripting capabilities through other mods that simply register new API object (The same way you access the current global CNPC API, or even the CNPC mod instance in your version)
somehussar commented 3 months ago

Explanation regarding this quote:

I do understand entirely the interest of allowing seasoned scripters the ability to use reflections and Java classes, however that completely breaks the informal "trust policy" modders and mod users have. I believe that as modders we should strive to cut-down on security problems that mods we create would be the root problem of.

This point was meant to indicate that while the client has the control over what mod they downloads and install (more knowledgable users can even decompile and look through the code for malware), they do not get a chance to look at the scripts sent over before they get executed

BetaZavr commented 3 months ago

Thank you After discussing this issue on the forums, I come to the same conclusion. Now I'll just remove the questionable elements of the mod, in the future I'll look for safe ways to implement the wishes of users. 1 - Not only Java.type but also Class.forName and other things will be analyzed in the code. 2 - Also added user consent to receive and run client scripts 3 - I will make a ban on encryption of client scripts 4 - Notification to the player about the presence of such scripts and their placement. So that people with skills can analyze these scripts