Java Deserialization vulnerability in GWT-RPC

[Medo42]

I just stumbled upon these discussions while looking for information on how the "enhanced classes" mechanism works:

https://groups.google.com/forum/#!msg/google-web-toolkit/j36D9-11JF4/OZwNQgvSAgAJ https://groups.google.com/forum/#!topic/google-web-toolkit-contributors/Jmo5qFoaw2M

In short, enhanced fields are serialized using Java Object serialization, and when the client sends back that serialized data, the server will deserialize it. Since there does not appear to be any safeguard (like a signature) to ensure that it has not been tampered with on the client-side, the client could inject any data it wants and the server will attempt to deserialize it.

Deserializing untrusted input is considered a security vulnerability, see e.g. https://owasp.org/www-community/vulnerabilities/Deserialization_of_untrusted_data

Unfortunately, I was not able to find any clue that these discussions actually led to improvements in the GWT RPC implementation, and a brief search in the current master branch shows that the relevant code (https://github.com/gwtproject/gwt/blob/master/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java#L804) seems to be unchanged since 2013.

Further, I'd argue that the implementation in GWT-RPC suffers from an additional vulnerability, since it allows an attacker to override the value of any field declared on the enhanced class, even transient and static fields. I think it is safe to say that application authors will not expect the possiblity that static or transient fields might be affected by deserialization, so this provides additional attack surface even in the absence of a gadget that would allow exploiting the Java Object Deserialization.

Edit to make a response prominent:

Mitigation for this has been released in GWT 2.11.0 and is backported to a GWT 2.10.1 release - this will disable this feature by default, showing warnings when compiling, and refusing to start the servlet unless the author explicitly opts-in to re-enabling it. This allows projects running those versions to be confident that they are not using this feature and thus unaffected by this bug.

The issue https://github.com/gwtproject/gwt/issues/9880 is filed to fix this by signing payloads when sent from the server. Additionally, https://github.com/gwtproject/gwt/issues/9881 is filed to disable attempting to serialize such payloads even if a class is annotated with JPA/JDO annotations. At present, both issues are waiting for someone to take them up, either by developing them, or to sponsor someone else to complete them.

To opt-in to allowing enhanced classes:

If a project decides that this risk is acceptable (due to appropriate access controls, input sanitation, or other limits that ensure this cannot be exploited), the Java system property gwt.enhancedClasses.enabled can be set to true on the server JVM to allow these RPC services to be used. This is generally discouraged, and will still result in a warning during compilation of the client, and during startup of the RemoteService implementation.

[tbroyer]

Unfortunately, I was not able to find any clue that these discussions actually led to improvements in the GWT RPC implementation

That's right. GWT-RPC has been basically unmaintained for the last 5 years.

The takeaway from the discussions you linked to is "don't use enhanced classes".

You may want to have a look at https://github.com/Vertispan/gwt-rpc, which is a "modern" reimplementation of GWT-RPC, and doesn't seem to suffer from this vulnerability.

[Medo42]

In this case, IMO the documentation should be updated with a warning in the relevant places. Looking at http://www.gwtproject.org/doc/latest/DevGuideServerCommunication.html gives the impression that GWT-RPC is the default and recommended approach, and there is no indication in the section "Serializing Enhanced Classes" either that their use is unsafe and should be avoided, neither in the Security section http://www.gwtproject.org/doc/latest/DevGuideSecurity.html.

[niloc132]

I missed this when it happened (newborn in the house) - and indeed our gwt-rpc fork is not vulnerable to this issue, we removed enhanced classes entirely.

A blog post was recently published about this, highlighting the issue. The core of the issue is correct, though the mitigations/conclusions section missed some nuance. Using GWT does not make an application vulnerable, but instead it is required that all of the following to be true:

• An application to be using GWT-RPC (other supported serialization mechanisms are unaffected),
• Explicit configuration in .gwt.xml files, adding one or more <extends-configuration-property> tags for the rpc.enhancedClasses property (without this, the application's services are safe),
• At least one of the rpc.enhancedClasses classes reachable from one of the RemoteService interfaces (unused classes will not contribute to this bug), and
• At least one of those DTO types decorated with JDO/JPA annotations (if no type is annotated, the bug cannot happen)

If any of the above is not met, the serialization policy file will not have @ClientFields - as the blog post correctly indicates, it is possible to check for that declaration and if missing, the application isn't vulnerable.

I'll propose a PR for 2.11 to warn both at compile time and fail at runtime, with an option to re-enable the feature (but with a warning). If someone is interested in contributing a way to safeguard that only the server can write these payloads, we could consider that instead, and I'll file a separate issue for that (ideally by signing payloads, and verifying the signature when reading them back again).

[Medo42]

It's been a while, but this does not quite match my memory of the investigation. I concur with points 1, 3 and 4, but we certainly did not explicitly enable the enhanced-classes feature in the .gwt.xml files, and there was no way to disable it which would have made the service safe.

I remember reading through all the relevant code back then and not finding any hook or condition I could use to prevent the problematic code from running. I eventually had to settle for a solution which scans all RPC data before deserializing and denying any byte sequence that could represent a deserialized object except the exact sequence that occurs when there are no extra fields. I'm quite sure if there had been a setting that would have prevented the affected code from running, I would have found it - though it's of course possible such a setting has been added since.

[Medo42]

Also even though this matches what you said, I think it's worth restating that this also affects you if you are not using enhanced classes at all. All Hibernate / JPA entity classes reachable from a RemoteService trigger the issue, whether enhanced or not, because GWT knows that they may be enhanced.

[Medo42]

I think the confusion about point 2 comes from this line:

https://github.com/gwtproject/gwt/blob/f439c5e544915b850587ee4b6e91600d7d4c49f4/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java#L870

As far as I can tell, tic.maybeEnhanced() is true if the class is annotated @Entity or @PersistenceCapable. enhancedClasses is the list of classes configured using rpc.enhancedClasses. The || means that rpc.enhancedClasses does not function as an allow list, but as a list of classes additionally considered enhanced.

[niloc132]

@Medo42 thanks for your reply - you caught my mistake - I was halfway through a walkthrough of code to show my work, but as I found the ||, your reply appeared, too soon for me to correct myself.

Fortunately, the implementation was extra aggressive to ensure that my understanding of the details wouldn't trip up the fix. Instead:

• First, it warns while compiling if a type is marked as enhanced (with the name of the type marked in this way, so a developer can fix that type as needed
• Then, when running the server, logs an error and will not return a usable SerializationPolicy if any type in that policy is marked as needing enhancement.

In this way, we should be sure that developers at build time can see the error, whether or not they actually test the service in question, and when a service no longer works, they can see in the logs that it has been disabled for this reason. Note that it is possible to override this with a new system property (e.g. -Drpc.enhancedClasses.enabled=true), but that will still result in a warning at runtime on the server.

[Medo42]

@niloc132 This sounds good from the safety side, but unless I overlooked something your fix does not change how the serialization policy is generated - if that's right, classes annotated with @Entity or @PersistenceCapable would still be marked with @ClientFields, cause warnings and make the serialization policy unloadable by default.

My project still sends @Entity-annotated classes (detached Hibernate entities) to the client and back, but we never used Hibernate bytecode enhancements. In other words, the fix does not actually help my project. We'd still be forced to change the code to not use @Entity classes with RPC or keep using our existing safety filter and add the new system property.

So what I suggest is to also stop automatically marking @Entity classes with @ClientFields, or at least offer a way to prevent that, in addition to the changes you have implemented. This would allow me to opt out of the mechanism I never needed and also have the assurance that if @ClientFields made it into a SerializationPolicy due to a mistake on my part, the new system property would still prevent the vulnerable code from running.

[niloc132]

@Medo42 not using the bytecode enhancements does not make your project safe, that's part of the problem here - a malicious client could add client fields even if the server didn't write them. I'm glad you have a safer filter, and I'm filing a separate issue to propose an outline of a general solution to this issue, but the fix has to also cover users who are accidentally using this feature, or deliberately using it with no safeguards in place. For the same reason, I am reticent to outright remove the @ClientFields, as there may be projects deliberately using this.

I would like to move swiftly to get a "quick" fix out, and then we can follow up with another point release that includes a safer solution?

From a discussion off-list, we will probably release 2.11.0 soon, and also backport this to 2.10.1 - if we find a follow-up, we should also backport it. Note that we cannot backport further than 2.10, as only Google can release com.google.gwt jars.

Does this seem reasonable as a way forward?

[Medo42]

@niloc132 I think there is a misunderstanding. Yes, whether or not I use bytecode enhancements has no bearing on whether my GWT service will be safe or not, all else being equal.

However, if I don't use bytecode enhancement, I don't need my @Entity classes to be marked with @ClientFields in the serialization policy. And if no classes are marked @ClientFields in the serialization policy, the application is not vulnerable, as you yourself wrote here.

The problem is that I have no easy way to influence how the serialization policy is generated and opt out of the @ClientFields, but as far as I can tell it would be easy to add - create a new configuration property for the .gwt.xml files (perhaps rpc.enhancedJpaClasses.enabled), give it whatever default you think is best, and at https://github.com/gwtproject/gwt/blob/f439c5e544915b850587ee4b6e91600d7d4c49f4/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java#L870 change the condition to

if ((enhancedJpaClasses && tic.maybeEnhanced())
          || (enhancedClasses != null && enhancedClasses.contains(type.getQualifiedSourceName())))

Your proposed fix alone would make affected projects aware of the problem (which is great!) and stop new projects from making the same mistake, but existing projects would still have to actually fix the problem, e.g. by not using @Entity classes in RPC anymore, unless they just accept it by setting the new system property.

However, I believe many projects just like mine don't use Hibernate bytecode enhancement and so would not actually need to change their code, if you just gave them a way to opt out of having @Entity classes automatically marked as @ClientFields. Releasing a new GWT version which forces them to acknowledge and address the problem without giving them this easy way to fix it at the same time would, in my estimation, cause a lot of unnecessary work.

[selcukatay]

Hi everyone,

I wanted to share a quick solution I've implemented and get some feedback on it.

I have changed following line

https://github.com/gwtproject/gwt/blob/7f76f0f2b5b1b028d2d870782048e44402637fb0/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java#L803

to use my new LookAheadObjectInputStream that extends ObjectInputStream. LookAheadObjectInputStream validates the class names against the SerializationPolicy to check if they are allowed to deserialize.

ObjectInputStream ois = new LookAheadObjectInputStream(baos, serializationPolicy,classLoader);

Here is the LookAheadObjectInputStream

package com.google.gwt.user.server.rpc.impl;

import com.google.gwt.user.client.rpc.SerializationException;
import com.google.gwt.user.server.rpc.SerializationPolicy;

import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.io.ObjectStreamClass;
import java.util.logging.Level;
import java.util.logging.Logger;

public class LookAheadObjectInputStream extends ObjectInputStream {

    private static final Logger log = Logger.getLogger(LookAheadObjectInputStream.class.getCanonicalName());
    private final SerializationPolicy serializationPolicy;
    private final ClassLoader classLoader;

    public LookAheadObjectInputStream(InputStream inputStream, SerializationPolicy serializationPolicy, ClassLoader classLoader)
            throws IOException {
        super(inputStream);
        this.serializationPolicy = serializationPolicy;
        this.classLoader = classLoader;
    }

    /**
     * This method is overridden to validate the class being deserialized.
     *
     * @param desc an instance of class <code>ObjectStreamClass</code>
     * @return
     * @throws IOException
     * @throws ClassNotFoundException
     */
    @Override
    protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException,
            ClassNotFoundException {
        try {
            serializationPolicy.validateDeserialize(Class.forName(desc.getName(),false,classLoader));
        } catch (SerializationException e) {
            log.log(Level.SEVERE, null, e);
            throw new RuntimeException(e.getMessage(), e);
        }

        return super.resolveClass(desc);
    }
}

I would appreciate any feedback on this approach. Specifically, I'm interested in knowing:

Are there any potential issues with this implementation? Is there a more efficient or better way to achieve this validation? Thank you for your time and insights!

ps: for a quick testing, you can place your own copy of ServerSerializationStreamReader.java and LookAheadObjectInputStream.java into your code base under the package com.google.gwt.user.server.rpc.impl Since your code base higher priority, the class loader would loads them instead of the gwt library so you don't have to compile whole gwt

[niloc132]

@selcukatay it sounds like you are describing option 3 at https://github.com/gwtproject/gwt/issues/9880, which is a perfectly valid way to solve this. I have used this in the past patching over similar issues in different serialization schemes.

Two quick code remarks.

Class.forName(desc.getName()).

You probably want to change this to the forName overload that does not initialize the class: Class.forName(desc.getName(), false, classloader). Otherwise, you are giving a user control to initialize classes, though not instantiate them. This is what GWT itself does in ServerSerializationStreamReader.deserialize before validation (slightly earlier in the same file as you quoted): https://github.com/gwtproject/gwt/blob/89d06d61e2e7ee3007d4cbeabe60e0f28da18467/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java#L623

serializationPolicy.validateSerialize(...)

You almost certainly mean to use validateDeserialize(...) here, as you are checking if a type may be deserialized.

---

Two immediate concerns with the approach:

• I have not used this feature before, but calling SerializationPolicy.validateSerialize() does not make sense to be as a valid way to defend against this. The serialization policy only contains the types that the GWT client can deserialize, but by definition, the clientFields/enhancedClasses feature is for types that cannot be deserialized by the client. Otherwise, they would just be regular fields.
• Be very very certain that your implementation can never not have "higher priority" in the classloader, or your fix will silently stop working and application will become vulnerable again. Unless you are testing by trying to pass bad payloads, you won't know if this fix ever stops working (contrast with a fix where you are preventing a bug - if the bug comes back right away you know something changed in how the classpath is set up).

If this works for you, that's great - if I'm mistaken and this solves some significant portion of projects out there (>50% for example), we could think about merging it as an easy fix. That would indicate that your enhanced classes end up just having a few extra fields added in bytecode, not that some ORM type for lazy loading or working with cursors are added at runtime.

Making the list of allowed classes configurable would fully implement option 3, but would require too that RPC.decodeRequest take another argument, or that SerializationPolicyProvider/SerializationPolicy know what types could be loaded via ObjectInputStream. Either looks like a breaking change to me, so as a general fix for this and #9880, I'd still lean towards the signing/verifying approach discussed there.

[selcukatay]

You've raised some excellent points:

• Class Initialization:I'll update my code to use the Class.forName(desc.getName(), false, classLoader) overload to avoid unnecessary class initialization. I'm glad you pointed it out.

• Switching from validateSerialize to validateDeserialize makes perfect sense.

Higher Priority Concern:

Your point about the class loader priority is well taken. I'll make sure it works as intended always. If it doesn't work stably, I will have to build by GWT fork.

I believe most JPA projects almost always have two-way serialization for all JPA entities. So I hope this helps someone.

Thank you again for your valuable input.

[niloc132]

This was mitigated in 2.10.1 and 2.11 by https://github.com/gwtproject/gwt/pull/9879.

Follow-up issues to let users re-enable this feature safely: https://github.com/gwtproject/gwt/issues/9880 is filed to fix this by signing payloads when sent from the server. https://github.com/gwtproject/gwt/issues/9881 is filed to disable attempting to serialize such payloads even if a class is annotated with JPA/JDO annotations.

Note that there seems to be no interest in this - I've discussed this issue with several users who were affected by this, but so far they have all found that they don't actually need this feature. As it is a liability, we may want to explore outright removing this, if there continues to be no development in one of the above tickets.