Conversation
…-list ObjectInputFilter (CWE-502) 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:
|
| protected static final String DEFAULT_ALLOWED_CLASSES = | ||
| "maxdepth=32;maxrefs=10000;maxbytes=1048576;maxarray=10000;" | ||
| + "java.lang.*;java.lang.Enum;java.math.*;java.time.*;java.util.*;java.util.concurrent.atomic.*;" | ||
| + "org.broadleafcommerce.**"; |
There was a problem hiding this comment.
🔴 Search index update messages can no longer be read from the shared queue, halting index updates
Queue messages containing Solr document objects are now rejected when read back (ois.setObjectInputFilter(getDeserializationFilter()) at core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/util/queue/ZookeeperDistributedQueue.java:857) because the allow-list only permits standard Java and Broadleaf types, so incremental search index updates fail in distributed setups.
Impact: In a clustered/SolrCloud deployment, incremental catalog index updates queued for processing fail with an error instead of being applied, so search results go stale.
Allow-list omits org.apache.solr.common classes carried by queued update commands
The distributed queue is used by DefaultSolrIndexQueueProvider.createDistributedQueue (core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/DefaultSolrIndexQueueProvider.java:147-149) for SolrUpdateCommand entries. One concrete entry type is IncrementalUpdateCommand, which holds a List<org.apache.solr.common.SolrInputDocument> (core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/IncrementalUpdateCommand.java:30), produced by AbstractSolrIndexUpdateServiceImpl.updateIndex (.../AbstractSolrIndexUpdateServiceImpl.java:270-273) and offered onto the queue at .../AbstractSolrIndexUpdateServiceImpl.java:157.
DEFAULT_ALLOWED_CLASSES (lines 92-95) permits only java.lang.*, java.math.*, java.time.*, java.util.*, java.util.concurrent.atomic.* and org.broadleafcommerce.**, then rejects everything else with !*. SolrInputDocument/SolrInputField are in org.apache.solr.common, so reading such an entry throws InvalidClassException, which is converted into a DistributedQueueException at lines 859-863 and the message aborts the take.
The fix is to include the Solr classes actually used by framework queue payloads (e.g. org.apache.solr.common.*) in the default pattern.
| protected static final String DEFAULT_ALLOWED_CLASSES = | |
| "maxdepth=32;maxrefs=10000;maxbytes=1048576;maxarray=10000;" | |
| + "java.lang.*;java.lang.Enum;java.math.*;java.time.*;java.util.*;java.util.concurrent.atomic.*;" | |
| + "org.broadleafcommerce.**"; | |
| protected static final String DEFAULT_ALLOWED_CLASSES = | |
| "maxdepth=32;maxrefs=10000;maxbytes=1048576;maxarray=10000;" | |
| + "java.lang.*;java.lang.Enum;java.math.*;java.time.*;java.util.*;java.util.concurrent.atomic.*;" | |
| + "org.apache.solr.common.*;org.broadleafcommerce.**"; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| protected static final String DEFAULT_ALLOWED_CLASSES = | ||
| "maxdepth=32;maxrefs=10000;maxbytes=1048576;maxarray=10000;" | ||
| + "java.lang.*;java.lang.Enum;java.math.*;java.time.*;java.util.*;java.util.concurrent.atomic.*;" | ||
| + "org.broadleafcommerce.**"; |
There was a problem hiding this comment.
🔍 Resource limits may reject large legitimate index batches
maxrefs=10000 and maxarray=10000 count object references/array lengths across the whole stream, not just the payload's top-level collection. A single IncrementalUpdateCommand carrying a few thousand SolrInputDocuments (each with several fields and string values) can easily exceed 10,000 references, resulting in an InvalidClassException that the code reports as a disallowed type, which is misleading. Also, maxbytes=1048576 is essentially at the Zookeeper transport ceiling that serialize merely warns about at 1,000,000 bytes, so entries just under the ZK limit could be rejected on read after being written successfully.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| protected static String buildDeserializationFilterPattern() { | ||
| final StringBuilder sb = new StringBuilder(DEFAULT_ALLOWED_CLASSES); | ||
| final String additional = System.getProperty(ADDITIONAL_ALLOWED_CLASSES_PROPERTY); | ||
| if (additional != null && !additional.trim().isEmpty()) { | ||
| sb.append(';').append(additional.trim()); | ||
| } | ||
|
|
||
| return sb.append(";!*").toString(); | ||
| } |
There was a problem hiding this comment.
🟨 Deserialization allow-list can be widened to arbitrary classes via a system property
The deserialization filter appends whatever is set in the broadleaf.zookeeper.queue.deserialization.allowedClasses system property before the trailing !* (core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/util/queue/ZookeeperDistributedQueue.java:907-915). A value such as * or ** silently disables the entire protection, and the property is also read only once and then cached in getDeserializationFilter() (lines 893-905), so the effective policy depends on process startup ordering. This is an intentional escape hatch, but it is worth documenting/validating so an over-broad pattern (e.g. **) does not reintroduce the gadget-chain exposure this PR fixes.
Was this helpful? React with 👍 or 👎 to provide feedback.
A Brief Overview
ZookeeperDistributedQueue.deserialize()calledObjectInputStream.readObject()on bytes read straight from Zookeeper znodes with no type filtering (CWE-502). Anyone able to write to the queue's/elementsor/configsznodes (the queue defaults toZooDefs.Ids.OPEN_ACL_UNSAFEwhen no ACLs are supplied) could plant a gadget payload and get code execution in every node that drains the queue.The stream is now constrained by a JDK
ObjectInputFilter(Java 17 baseline) built from an allow list, with the trailing!*rejecting everything else, plus depth/refs/bytes/array limits:A rejected class surfaces as
InvalidClassException, which is wrapped in aDistributedQueueExceptionwhose message points at the escape hatch.Escape hatches for applications queueing their own entry types, neither of which weakens the default:
-Dbroadleaf.zookeeper.queue.deserialization.allowedClasses=com.mycompany.model.**(semicolon-delimited, appended before the!*)getDeserializationFilter()/buildDeserializationFilterPattern()in a subclassNote this hardens the parse step only; it is not a substitute for locking down the znode ACLs.
Testing
ZookeeperDistributedQueueDeserializationFilterTestcovers an allowed entry (ArrayList<String>,Integer— the type used for themaxCapacityconfig node), rejection of a non-allow-listed type, and the system-property extension.mvn -pl core/broadleaf-framework test -Dtest=ZookeeperDistributedQueueDeserializationFilterTest→ 3/3 green.Add Labels to the right panel: Bug, Severity: critical, Status: ready-for-code-review
Link to Devin session: https://app.devin.ai/sessions/52debaaa56134cd9b90c3d98609b840a
Requested by: @Colhodm
Devin Review
fc368d4