JinwooHwang commented on PR #7941:
URL: https://github.com/apache/geode/pull/7941#issuecomment-3618863613
Hi @sboorlagadda,
Thank you for taking the time to review this PR and propose an alternative
architectural approach. I genuinely appreciate your thoughtful consideration of
how this could integrate with Geode's existing infrastructure.
I'd like to better understand the security coverage of your proposed Thread
Context ClassLoader approach. You mentioned two key benefits:
> "Closes the vulnerability immediately"
> "Uses ALL existing Geode security filters"
I want to make sure I understand this correctly. My current implementation
explicitly immediately blocks:
- **26 specific gadget classes** (InvokerTransformer, ChainedTransformer,
TemplatesImpl, ObjectFactory, MethodClosure, JndiRefForwardingDataSource, etc.)
- **10 dangerous package patterns**
(org.apache.commons.collections.functors.*,
org.springframework.beans.factory.*, java.rmi.*, etc.)
Could you help me understand how your proposed approach would block these
same classes? Specifically:
1. **Which existing Geode filter(s) block these gadget chains?**
I traced through the code and found that `serializationFilter` defaults
to `NullStreamSerialFilter` (line 294 in InternalDataSerializer.java). Looking
at the implementation:
```java
// NullStreamSerialFilter.java
public class NullStreamSerialFilter implements StreamSerialFilter {
@Override
public void setFilterOn(ObjectInputStream objectInputStream) {
// Do nothing, this is the case where we don't filter.
}
}
```
This is explicitly documented as "Implementation of StreamSerialFilter
that does nothing" with the comment "Do nothing, this is the case where we
don't filter." So by default, no filtering is applied.
2. **Does sanctioned-geode-core-serializables.txt contain gadget blocking
entries?**
I examined this file (485 lines) and found it contains only Apache Geode
internal classes (exceptions, cache operations, admin/management, PDX
serialization).
This appears to be a whitelist of Geode-internal classes for cluster
operations, not a security blocklist. It contains 485 Geode classes but zero
gadget chain blocking entries.
Regarding your statement that this "closes the vulnerability immediately" -
I want to understand the mechanism. The vulnerability exists because malicious
gadget chains (like InvokerTransformer) can be deserialized without any
validation. For the vulnerability to be closed:
- **These specific classes must be blocked** during deserialization, OR
- **An allowlist must reject** classes not explicitly permitted
Could you clarify which of these mechanisms the TCCL approach uses? From my
code analysis, I see that `BlobHelper.deserializeBlob()` calls through to
`InternalDataSerializer` which applies `serializationFilter` (currently
`NullStreamSerialFilter` by default). This appears to provide no blocking of
gadget chains.
I may be misunderstanding how Geode's existing filter infrastructure works.
If there's existing gadget chain protection in Geode that I'm missing, I'd be
very grateful if you could point me to the specific code or configuration that
provides this protection.
I've invested considerable effort creating 28 unit tests in this PR covering
gadget blocking, resource limits, and various configuration scenarios - all
demonstrating that SafeDeserializationFilter successfully blocks known attack
chains. Could you share examples or point me to existing tests that demonstrate
how TCCL prevents classes like InvokerTransformer from being deserialized and
executed? That would really help me understand the protection mechanism you're
describing.
My goal is to understand whether the Thread Context ClassLoader approach
provides equivalent security coverage to the explicit
SafeDeserializationFilter, or if additional work would be needed to add gadget
chain protection to Geode's core infrastructure.
**Specific concern:** If we adopt the TCCL approach without gadget blocking,
an attacker could still send a malicious `InvokerTransformer` object in a
session attribute. The TCCL would successfully load the class (improving
compatibility), but the gadget chain would execute, achieving RCE. Is there a
mechanism I'm missing that prevents this?
I'm completely open to integrating with Geode's existing infrastructure if
it can provide the same security guarantees. I just want to make sure we don't
inadvertently leave the vulnerability open while we work on architectural
improvements.
Thank you again for your guidance and expertise on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
