Prevent insecure deserialization (CWE-502) in ZookeeperDistributedQueue#48
Open
devin-ai-integration[bot] wants to merge 1 commit into
Open
Conversation
…edQueue 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:
|
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
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.
Fixing insecure deserialization (CWE-502) in
ZookeeperDistributedQueueZookeeperDistributedQueue.deserialize()previously calledObjectInputStream.readObject()on bytes read from Zookeeper with no class filtering. An attacker able to influence data stored in Zookeeper could plant a serialized gadget chain and achieve Remote Code Execution.A Brief Overview
This restricts which classes may be reconstructed during deserialization to an explicit allowlist, so unexpected/gadget classes are rejected before they can be instantiated.
SecureObjectInputStream(anObjectInputStreamsubclass) overridesresolveClass()to reject any class not on an allowlist and overridesresolveProxyClass()to reject all dynamic proxies (a common gadget vector). Array types are unwrapped to their component type before matching; primitive components are always allowed.ZookeeperDistributedQueue.deserialize()now usesSecureObjectInputStreaminstead of a rawObjectInputStream. Serialization is unchanged.getDeserializationAllowlist()/setDeserializationAllowlist(List<String>)for applications that store custom serializable element types. The default covers boxed primitives,String, standard collections (java.util.*),java.time.*,java.math.*, andorg.broadleafcommerce.*.Allowlist pattern syntax: an exact FQCN (
java.lang.Integer) or a package prefix ending in.*(java.util.*, matching the package and sub-packages).Tests
Added
ZookeeperDistributedQueueDeserializationTest(6 tests, all passing) covering: allowlisted scalar/collection round-trips, rejection of a non-allowlisted serializable class (java.io.File) and array thereof, and honoring a custom allowlist.Run:
mvn -pl core/broadleaf-framework -am test -Dtest=ZookeeperDistributedQueueDeserializationTest -Dsurefire.failIfNoSpecifiedTests=falseAdd Labels to the right panel: Security, Severity: critical, Status: ready-for-code-review
Additional context
The configurable allowlist means existing apps that place custom types on the queue may need to register their packages via
setDeserializationAllowlist(...); otherwise behavior is unchanged for the standard types Broadleaf stores.Link to Devin session: https://app.devin.ai/sessions/c14f970fa42d4e58a91bfe0882669e3c
Devin Review