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

What about potentially vulnerable classes that *aren't* in the config? #18

Open
SuperStormer opened this issue Jul 30, 2023 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested

Comments

@SuperStormer
Copy link

Forgive me if I misread the code, but

if (patchModule.getClassesToPatch().contains(className)) {
suggests that OIS is only redirected to the filtered version in classes that are covered by one of the patchModules in the config. Wouldn't this leave open the possibility that a vulnerable class was missed and remains unpatched?

@SuperStormer
Copy link
Author

possibly solved by #15 ?

@Einhornyordle
Copy link
Collaborator

Yes. Only known exploits are fixed by the mod, there might still be a ton of affected mods out there we haven't discovered yet and all the exploits that come with them won't be fixed by our mod. As we stated multiple times, we got rushed to provide this fix early, we were just in the process of establishing contact to curseforge and modrinth so we would have two massive mod databases to scan for exploits but now we have to use what we got and this is it.

Still, the exploits that we could fix with this mod are way better than being completely unprotected.

@dogboy21
Copy link
Owner

As already mentioned, our patch only applies to the known vulnerable classes in the config file so we're able to provide a valid deserialization allowlist for all cases instead of possibly breaking the mods behavior when we outright block all attempts of deserialization.
I will take a look at #15 (once I've got some spare time) which could be a valid option for those willing to take the risk of potentially breaking mods.

@dogboy21 dogboy21 added enhancement New feature or request help wanted Extra attention is needed question Further information is requested labels Jul 30, 2023
@mikehearn
Copy link

Why not use the serialization filtering feature? That avoids the need to rewrite bytecode and it applies to the whole process:

https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputFilter.html

See "Serialization Filters" here:

https://docs.oracle.com/en/java/javase/17/core/serialization-filtering1.html#GUID-8296D8E8-2B93-4B9A-856E-0A65AF9B8C66

@dogboy21
Copy link
Owner

The problem with ObjectInputFilter is that it's only available in newer Java versions (>9 and >8u121 according to this blog article).
And while I totally know that 8u121 was released in January 2017, we are also aware that there are still Launchers shipping a version as old as 8u51, which would completely break a fix depending on that feature.

Then we also noticed that many mods use OIS internally to persist some state (so not in the direct network path) which we would also completely break with that approach probably. I know that our approach with just fixing the classes that we know of definitely isn't perfect, but I think it's a good compromise between security and literally breaking any mod in existance.

@HeatherComputer
Copy link
Collaborator

Just to say, I plan to work on a good way to do this.
It'll need configuring ofc - but how exactly do you reckon we have good config whilst maintaining compatability? It'll need to be more in-depth than a simple target all classes boolean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants