Harden ZookeeperDistributedQueue against insecure deserialization (CWE-502)#42
Conversation
…n (CWE-502) Restrict ObjectInputStream.readObject() in deserialize() to an allow list of permitted classes via AllowListObjectInputStream, rejecting non-allow-listed classes and dynamic proxies. Adds unit tests covering allowed and blocked types. Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| public static final Set<String> DEFAULT_DESERIALIZATION_ALLOW_LIST = Collections.unmodifiableSet(Set.of( | ||
| "java.lang.", | ||
| "java.util.", | ||
| "java.time.", | ||
| "java.math.", | ||
| "org.apache.solr.common.", | ||
| "org.broadleafcommerce." | ||
| )); |
There was a problem hiding this comment.
🚩 Broad java.util. prefix permits large surface area of JDK classes*
The java.util. prefix in the allow list permits ALL classes in java.util and its subpackages (e.g., java.util.concurrent.*, java.util.logging.*). While gadget chains typically require non-JDK sink classes that ARE blocked, some known deserialization gadgets leverage java.util.PriorityQueue or java.util.Comparator implementations as intermediate steps. In this deployment context (Zookeeper data), the risk is mitigated because an attacker would need to control ZK data AND find a gadget chain using only the allowed prefixes. Nonetheless, narrowing java.util. to specific classes (e.g., java.util.ArrayList, java.util.LinkedHashMap, java.util.CollSer) would reduce the attack surface.
Was this helpful? React with 👍 or 👎 to provide feedback.
A Brief Overview
ZookeeperDistributedQueue.deserialize()calledObjectInputStream.readObject()directly on bytes read from Zookeeper with no type filtering. Since queue/config data in Zookeeper must be treated as untrusted, this is an insecure deserialization vulnerability (CWE-502) that can lead to RCE via gadget chains.This PR restricts deserialization to an explicit allow list of classes, rejecting everything else.
What changed
deserialize()now builds the stream via a new overridablecreateObjectInputStream(InputStream)which returns anAllowListObjectInputStream.AllowListObjectInputStream extends ObjectInputStreamand overrides:resolveClass(...): unwraps array/primitive types and throwsInvalidClassExceptionfor any class not on the allow list.resolveProxyClass(...): always rejects dynamic proxy deserialization.DEFAULT_DESERIALIZATION_ALLOW_LIST) covers exactly the types actually placed on this queue:java.lang.,java.util.,java.time.,java.math.org.apache.solr.common.(SolrInputDocument/SolrInputField) andorg.broadleafcommerce.(SolrUpdateCommandsubtypes); config payloads areInteger.Serializabletypes on the queue can overridegetDeserializationAllowList()(e.g. return a superset of the default) instead of weakening the base list.Pseudocode of the core check:
Tests
Added
ZookeeperDistributedQueueDeserializationTest(4 tests, all passing):Integer,String,ArrayList) round-trip,String[]) round-trip,IncrementalUpdateCommandcontaining aSolrInputDocument) round-trips,Serializableclass (java.io.File) is rejected withInvalidClassException.mvn -pl core/broadleaf-framework -am test -Dtest=ZookeeperDistributedQueueDeserializationTest→ Tests run: 4, Failures: 0, Errors: 0.Labels
Additional context
No behavior change for legitimate queue traffic; only the set of resolvable classes during deserialization is constrained.
Link to Devin session: https://app.devin.ai/sessions/a5ed15631bc24c738f47afca59e7e5b3
Requested by: @Colhodm
Devin Review
083f957