fix: mitigate insecure deserialization in ZookeeperDistributedQueue (CWE-502)#17
Open
devin-ai-integration[bot] wants to merge 2 commits intodevelop-7.0.xfrom
Open
Conversation
…CWE-502) Replace unfiltered ObjectInputStream.readObject() with a filtering ObjectInputStream that validates class names against an allowlist before deserialization. This prevents Remote Code Execution (RCE) via crafted payloads stored in Zookeeper. The allowlist permits only Broadleaf Commerce classes, standard Java types (java.lang, java.util, java.math, java.time), and primitive arrays. Subclasses can override getAllowedDeserializationPrefixes() to customize the allowlist. Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
Author
🤖 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:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…n deserialization filter Address Devin Review findings: - Add org.apache.solr. to the allowlist since IncrementalUpdateCommand contains SolrInputDocument fields from that package. - Unwrap JVM array type descriptors (e.g. [Ljava.lang.String;) to their component class name before checking against allowed prefixes. - Remove now-unnecessary explicit primitive array prefixes since primitive type descriptors are handled by the single-char check after stripping. Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
ZookeeperDistributedQueue.deserialize()used a rawObjectInputStream.readObject()on data read from Zookeeper with no class filtering. This is a well-known deserialization vulnerability (CWE-502) that can lead to Remote Code Execution if an attacker can influence data stored in Zookeeper nodes.Labels: Security, critical, ready-for-code-review
Changes
The
deserialize()method now uses a filteringObjectInputStreamthat overridesresolveClass()to validate each class name against a prefix-based allowlist before permitting deserialization. Classes not matching any allowed prefix are rejected with anInvalidClassException.The default allowlist (
DEFAULT_ALLOWED_DESERIALIZATION_PREFIXES) permits:org.broadleafcommerce.*— all Broadleaf domain types (e.g.SolrUpdateCommand)org.apache.solr.*— Solr types required byIncrementalUpdateCommand(which containsSolrInputDocumentfields)java.lang.*,java.math.*,java.util.*,java.time.*— standard JDK types (coversIntegerused for config storage)JVM array type descriptors are handled generically in
resolveClass():[markers are stripped to handle multi-dimensional arrays (e.g.[[I)L...;) are unwrapped to extract the component class name (e.g.[Ljava.lang.String;→java.lang.String)B,C,I,J,S,D,F,Z) are always permittedThe filter is extensible: subclasses can override
getAllowedDeserializationPrefixes()to widen or narrow the allowlist. ThecreateFilteredObjectInputStream()factory method is alsoprotectedfor further customization.Human review checklist
org.broadleafcommerce.andorg.apache.solr.are broad prefixes. A narrower scope (e.g.org.broadleafcommerce.core.search.service.solr.indexer.) would be more restrictive but risks breaking extensibility. Worth a judgement call.resolveClasslogic handles all JVM type descriptor edge cases correctly — particularly nested object arrays like[[[Ljava.util.List;.Link to Devin session: https://app.devin.ai/sessions/7573a5bd991a427aacde1f410f0903af
Requested by: @Colhodm