Skip to content

Restricting the classes that can be deserialized from Zookeeper queues - #108

Open
Colhodm wants to merge 1 commit into
develop-7.0.xfrom
devin/1787784057-zk-queue-deser-filter
Open

Colhodm wants to merge 1 commit into
develop-7.0.xfrom
devin/1787784057-zk-queue-deser-filter

Conversation

@Colhodm

@Colhodm Colhodm commented Aug 26, 2026

Copy link
Copy Markdown

A Brief Overview

ZookeeperDistributedQueue.deserialize() called ObjectInputStream.readObject() on bytes read out of Zookeeper with no class filtering (CWE-502). Any party able to write to the queue's znodes (or the ZK ensemble itself, since the queue defaults to OPEN_ACL_UNSAFE) could get arbitrary classes instantiated on every node reading the queue — the standard path to RCE via a gadget chain on the classpath.

Reads now go through an ObjectInputFilter:

protected Object deserialize(byte[] bytes) { return deserialize(bytes, createDeserializationFilter()); }
protected Object deserialize(byte[] bytes, ObjectInputFilter filter)   // ois.setObjectInputFilter(filter)
protected boolean isDeserializationAllowed(Class<?> clazz)             // package allow/deny list
  • Default entry filter allows org.broadleafcommerce.*, java.lang/util/math/time.*, always rejects java.lang.reflect.* and java.lang.invoke.*, and everything not matched is rejected before the class is loaded. Array classes are checked by base component type; primitives pass.
  • Stream limits (depth 32, refs/array length 10k, 1MB — Zookeeper's own transport cap) reject deserialization bombs.
  • The configs/maxCapacity read, whose type is known up front, uses createTypeRestrictedDeserializationFilter(Integer.class). Because String is encoded as TC_STRING and never reaches the filter as a class, the type-restricted filter also validates the resulting object's exact class after the read.
  • Deployments that queue entry types outside the default packages stay supported by overriding isDeserializationAllowed(Class) or createDeserializationFilter(); a rejected class is logged with the queue path and that hint.

New ZookeeperDistributedQueueDeserializationTest covers allowed round trips, rejection of a disallowed type (java.io.File), the Integer-only filter accepting Integer while rejecting String, and reflection types being denied.

Labels: Security, Severity: critical, Status: ready-for-code-review

Additional context

mvn -pl core/broadleaf-framework -am -DskipTests compile and the new test class pass locally; the wider module suite was not run.

Devin-Org: engineering

Link to Devin session: https://app.devin.ai/sessions/ec83e007b8154be2a9766e428715a5ed
Requested by: @Colhodm


Devin Review

Status Commit
🟢 Reviewed ea288f4
Devin Review (Staging)

Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@staging-devin-ai-integration staging-devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Devin Review (Staging)
Debug

Playground

Comment on lines +87 to +93
protected static final List<String> ALLOWED_DESERIALIZATION_PACKAGES = Collections.unmodifiableList(Arrays.asList(
"org.broadleafcommerce.",
"java.lang.",
"java.util.",
"java.math.",
"java.time."
));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Distributed Solr incremental updates fail to deserialize

The allow-list in ALLOWED_DESERIALIZATION_PACKAGES omits org.apache.solr, so reading an IncrementalUpdateCommand off the queue rejects its nested SolrInputDocument entries. Incremental Solr index updates over a Zookeeper-backed queue fail with a DistributedQueueException.

Prompt for agents
The deserialization allow-list restricts nested classes to org.broadleafcommerce, java.lang, java.util, java.math, and java.time. However, IncrementalUpdateCommand (placed on this queue via AbstractSolrIndexUpdateServiceImpl.updateIndex through DefaultSolrIndexQueueProvider.createDistributedQueue) holds a List<SolrInputDocument>, and SolrInputDocument/SolrInputField live in org.apache.solr.common. When a node reads such a command back, the ObjectInputFilter rejects org.apache.solr.common.* and deserialize() throws DistributedQueueException, breaking distributed incremental Solr indexing. Consider adding the org.apache.solr package prefix to the allowed packages (and verifying the concrete value types stored inside SolrInputField such as dates/numbers are also covered), or have the Solr queue subclass override isDeserializationAllowed to permit the Solr command types it actually enqueues.
Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Comment on lines +87 to +93
protected static final List<String> ALLOWED_DESERIALIZATION_PACKAGES = Collections.unmodifiableList(Arrays.asList(
"org.broadleafcommerce.",
"java.lang.",
"java.util.",
"java.math.",
"java.time."
));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Commercial com.broadleafcommerce packages not in allow-list

ALLOWED_DESERIALIZATION_PACKAGES covers org.broadleafcommerce but not com.broadleafcommerce, used by commercial modules. Entry or nested-field types from those packages are rejected unless a subclass overrides isDeserializationAllowed.

Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant