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

Introduced protections against deserialization attacks #4613

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pixeeai
Copy link

@pixeeai pixeeai commented Dec 1, 2024

Hi, my name is Zach and I'm a developer for Pixee. I wanted to bring some light to this change as it was generated from our automated code security bot.

This change hardens Java deserialization operations against attack. Even a simple operation like an object deserialization is an opportunity to yield control of your system to an attacker. In fact, without specific, non-default protections, any object deserialization call can lead to arbitrary code execution. The JavaDoc now even says:

Deserialization of untrusted data is inherently dangerous and should be avoided.

Let's discuss the attack. In Java, types can customize how they should be deserialized by specifying a readObject() method like this real example from an old version of Spring:

static class MethodInvokeTypeProvider implements TypeProvider {
    private final TypeProvider provider;
    private final String methodName;

    private void readObject(ObjectInputStream inputStream) {
        inputStream.defaultReadObject();
        Method method = ReflectionUtils.findMethod(
                this.provider.getType().getClass(),
                this.methodName
        );
        this.result = ReflectionUtils.invokeMethod(method,this.provider.getType());
    }
}

Reflecting on this code reveals a terrifying conclusion. If an attacker presents this object to be deserialized by your app, the runtime will take a class and a method name from the attacker and then call them. Note that an attacker can provide any serliazed type -- it doesn't have to be the one you're expecting, and it will still deserialize.

Attackers can repurpose the logic of selected types within the Java classpath (called "gadgets") and chain them together to achieve arbitrary remote code execution. There are a limited number of publicly known gadgets that can be used for attack, and our change simply inserts an ObjectInputFilter into the ObjectInputStream to prevent them from being used.

+ import io.github.pixee.security.ObjectInputFilters;
  ObjectInputStream ois = new ObjectInputStream(is);
+ ObjectInputFilters.enableObjectFilterIfUnprotected(ois);
  AcmeObject acme = (AcmeObject)ois.readObject();

This is a tough vulnerability class to understand, but it is deadly serious. It offers the highest impact possible (remote code execution), it's a common vulnerability (it's in the OWASP Top 10), and exploitation is easy enough that automated exploitation is possible. It's best to remove deserialization entirely, but our protections is effective against all known exploitation strategies.

More reading

I have additional improvements ready for this repo! If you want to see them, leave the comment: (after installing for your repo here)

@pixeebot next

... and I will open a new PR right away!

🧚🤖 Powered by Pixeebot

Feedback | Community | Docs | Codemod ID: pixee:java/harden-java-deserialization

Copy link
Member

@xuzifu666 xuzifu666 left a comment

Choose a reason for hiding this comment

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

Please fix the error CI first. @pixeeai

@JingsongLi
Copy link
Contributor

Hi @pixeeai , thanks for the contribution, but I don't think we need to protect serialization because our libs are serializations that run in computing engines such as Flink and Spark, and they have their own security mechanisms.

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.

3 participants