-
Notifications
You must be signed in to change notification settings - Fork 382
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
Java Deserialization vulnerability in GWT-RPC #9709
Comments
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. |
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. |
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:
If any of the above is not met, the serialization policy file will not have 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). |
Logs warnings at compile time, indicating which classes need to be cleaned up to remove this feature. Mitigation for gwtproject#9709
Logs warnings at compile time, indicating which classes need to be cleaned up to remove this feature. Mitigation for gwtproject#9709
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. |
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. |
I think the confusion about point 2 comes from this line:
As far as I can tell, |
@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 Fortunately, the implementation was extra aggressive to ensure that my understanding of the details wouldn't trip up the fix. Instead:
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. |
@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 My project still sends So what I suggest is to also stop automatically marking |
@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 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 Does this seem reasonable as a way forward? |
@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 The problem is that I have no easy way to influence how the serialization policy is generated and opt out of the
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 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 |
Logs warnings at compile time, indicating which classes need to be cleaned up to remove this feature. Mitigation for #9709
Logs warnings at compile time, indicating which classes need to be cleaned up to remove this feature. Mitigation for #9709
Hi everyone, I wanted to share a quick solution I've implemented and get some feedback on it. I have changed following line gwt/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java Line 803 in 7f76f0f
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? 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 |
@selcukatay it sounds like you are describing option 3 at #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.
You probably want to change this to the forName overload that does not initialize the class: gwt/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java Line 623 in 89d06d6
You almost certainly mean to use Two immediate concerns with the approach:
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 |
You've raised some excellent points:
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. |
This was mitigated in 2.10.1 and 2.11 by #9879. Follow-up issues to let users re-enable this feature safely: 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. |
Hi folks, I'm movig from 2.9 to 2.11. In my project I have a lot of detached @Entities used in RPC communication. I just want to disable the Warn message when I complie it, because there are a lot of them. I tried to enable/disable gwt.enhancedClasses.enabled system property, but it keeps showing up. I realize that in the code the warnnig is always generated. I don't be sure if I need the enhanced classes functionality enabled or disabled, but I just want to continue using my Entities in RPC and avoid the warnning. Thank you. |
The system property only applies to the server, and it only stops the server from emitting an error and exiting. In the server, it logs only once, when that service's serialization policy file is first loaded. At compile time it logs per-bean, under the assumption that the developer should work to fix each vulnerable location. We can investigate warning only once per service instead of once per bean, but I wouldn't want to disable it further than that - this is a serious vulnerability, and work should instead be put into either mitigating it within your project (disabling enhanced classes, stop mixing jpa in dtos, etc) or in the GWT project itself by fixing 9880 and/or 9881 so that this is no longer a security issue at all. |
I use Beanlib Serialization to move entities between server and client, then I supouse I don't need Enhanced Classes feature, I can disable it, but I don't want to get the warning messages. In compilation time, the time increase a lot due to the warning. |
Can you give me a reference on what you mean by "Beanlib Serialization to move entities between server and client"? If you aren't using GWT-RPC at all (i.e. calling GWT.create(...) with a RemoteService subtype, and mentioning these annotated beans in that subtype) then you wouldn't be seeing these warnings. If someone were interested in implementing something like what @Medo42 proposed in #9709 (comment) (where we have an annotation, config property, compiler flag that would outright disable enhanced classes), that might fit what you seem to be describing - but I would want to be very certain that this is what you actually want before suggesting that this could be worth some development time. |
BeanLib (hibernate) is a tool that let me serializate Entites in GWT-RPC communication. Then yes, I use GWT-RPC. In the client, I really have a Entity Clone, but the original Entity Class Name is referenced by GWT Service. Then, I need my entities java package to be referenced by GWT Module At this point, I recompiled gtw-user 11 and removed the warning, but in order for it to work on the server, and not exclude @Entities from the serialization policy, I had to enable the enhanced classes feature (-Dgwt.enhancedClasses.enabled=true), but I'm not sure I really need to have it enabled (taking the risk), given the use of BeanLib. Thanks! |
@gerson-samaniego Another sanity check for whether you'd actually need the GWT-RPC "enhanced classes" feature is whether you are actually using Hibernate bytecode enhancement. Note that it is disabled by default. |
If the class is annotated with jpa/jdo annotations, fields that GWT doesn't support will be serialized using plain java.io.ObjectOutputStream. We don't currently support a feature to stop sending other fields (even if they don't exist or don't matter), that could be covered as part of #9881. |
Correct. I was just trying to make sure whether @gerson-samaniego's situation is the one that would benefit from #9881. I'm a bit confused by your wording though ("that could be covered as part of #9881") - Isn't giving users the choice to disable sending of extra fields via ObjectOutputStream / ObjectInputStream the main point of #9881 rather than an optional part? |
You're correct, that is the point, though at least for me the ticket is more about the "how" of implementing that - what would be the most concise/accurate/consistent/unsurprising way to expose this feature. |
I'll second @gerson-samaniego proposition to optionally suppress the #9709 warnings during GWT compilation. For every GWT service compiled, a warning for every entity class is reported, even if the service doesn't reference them. |
I can't confirm this - can you file a bug specifically about this behavior, with steps to reproduce so we can track this and fix it? Here's what I did to try to reproduce this: Start with the "dynatable" sample, and add the following class in the "client" package: package com.google.gwt.sample.dynatable.client;
@javax.persistence.Entity
public class MyEntity {
} Ensure that this class is used in the client (and can be compiled by the client) by referencing it from public class DynaTable implements EntryPoint {
public void onModuleLoad() {
new MyEntity();
// Find the slot for the calendar widget.
... Add the persistence api jar to the project classpath, something like
If that jar was missing, the build output will be something like this, indicating the missing jar
If the jar is correctly present, so it could potentially introduce this bug, we instead see this log, correctly missing the warning:
If instead the annotation is placed on a bean that is actually reachable from the RemoteService type (like Person.java), we instead correctly see this:
I suspect that in your case, the annotated type is actually reachable, although your project may not actually de/serialize the type under normal operation. In this case, the warning is not only correct but important to take note of: your application is likely to be vulnerable! Just because the type isn't sent under normal circumstances doesn't meant that the serialization policy isn't configured to forbid it being sent - if you are seeing the warning, that indicates that your annotated type was reachable and so can be deserialized, so your server could be forced to deserialize a dangerous payload. Take a look at your generated rpc policy file (.gwt.rpc in the same directory as your generated JS) to confirm that the type is or is not present to confirm this. If your server can start and application can apparently run without the flag that allows it to run unsafely, you may still be safe - check the build log carefully in that case to confirm that the RPC interface being compiled is actually used. If it isn't, you should look into deleting that dead code, which will itself fix your warnings. This doesn't diminish the suggest of suppressing duplicate entries, but I'm still against entirely suppressing the warnings. If someone would like to work on that, that's up to them. If someone has the time/resources/inclination to focus on this, I would encourage them to look more closely at fixing the root issue rather than preventing the warning aimed at discouraging the bad behavior. |
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 #9880 is filed to fix this by signing payloads when sent from the server. Additionally, #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 totrue
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.The text was updated successfully, but these errors were encountered: