Conversation
…-list ObjectInputFilter 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 List<String> DEFAULT_ALLOWED_CLASS_PATTERNS = Collections.unmodifiableList(Arrays.asList( | ||
| //array patterns match on the component type, so this covers the Object[] backing collections | ||
| "java.lang.Object", | ||
| "java.lang.Boolean", | ||
| "java.lang.Byte", | ||
| "java.lang.Character", | ||
| "java.lang.Double", | ||
| "java.lang.Float", | ||
| "java.lang.Integer", | ||
| "java.lang.Long", | ||
| "java.lang.Short", | ||
| "java.lang.Number", | ||
| "java.lang.String", | ||
| "java.lang.Enum", | ||
| "java.math.BigDecimal", | ||
| "java.math.BigInteger", | ||
| "java.util.Date", | ||
| "java.sql.Date", | ||
| "java.sql.Timestamp", | ||
| "java.time.*", | ||
| "java.util.ArrayList", | ||
| "java.util.LinkedList", | ||
| "java.util.HashMap", | ||
| "java.util.LinkedHashMap", | ||
| "java.util.TreeMap", | ||
| "java.util.HashSet", | ||
| "java.util.LinkedHashSet", | ||
| "java.util.TreeSet", | ||
| "java.util.Arrays$ArrayList", | ||
| "java.util.Collections$*", | ||
| "org.broadleafcommerce.**")); |
There was a problem hiding this comment.
🔴 Search index update messages can no longer be read back, breaking distributed Solr indexing
Search index update messages are blocked from being read (ois.setObjectInputFilter(deserializationFilter.getFilter()) at core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/util/queue/ZookeeperDistributedQueue.java:849) because the documents they carry are not on the allow list, so incremental index updates stop working in clustered setups.
Impact: In a SolrCloud/Zookeeper deployment, incremental catalog index updates fail with an error instead of being applied, so the search index goes stale.
Allow list omits the Solr document classes carried by queued commands
The only in-tree user of this queue is DefaultSolrIndexQueueProvider.createDistributedQueue (core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/DefaultSolrIndexQueueProvider.java:142), which stores SolrUpdateCommand instances. AbstractSolrIndexUpdateServiceImpl.updateIndex enqueues an IncrementalUpdateCommand (core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/AbstractSolrIndexUpdateServiceImpl.java:272-274), whose fields are List<SolrInputDocument> (core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/IncrementalUpdateCommand.java:30).
org.apache.solr.common.SolrInputDocument / SolrInputField are not covered by DEFAULT_ALLOWED_CLASS_PATTERNS (only org.broadleafcommerce.**, JDK value types and collections), so ObjectInputFilter rejects them with an InvalidClassException, which deserialize converts into a DistributedQueueException. Every poll/take/contains that encounters such an entry now fails.
Adding org.apache.solr.common.SolrInputDocument, org.apache.solr.common.SolrInputField (and any value types used inside document fields) to the defaults would restore the existing behavior.
Prompt for agents
The new allow-list in DistributedQueueDeserializationFilter.DEFAULT_ALLOWED_CLASS_PATTERNS does not include the Solr classes that the only in-tree queue user actually places on the Zookeeper queue. DefaultSolrIndexQueueProvider.createDistributedQueue builds a ZookeeperDistributedQueue of SolrUpdateCommand, and AbstractSolrIndexUpdateServiceImpl.updateIndex enqueues IncrementalUpdateCommand, which holds a List<org.apache.solr.common.SolrInputDocument> (each containing SolrInputField). With the filter in place, reading those entries back throws InvalidClassException, which deserialize() turns into a DistributedQueueException, so incremental index updates (and even contains() checks used before offering new commands) fail in any Zookeeper-backed deployment. Consider adding the Solr common document/field types (and any value types stored in document fields, e.g. dates/numbers/collections already covered) to the defaults, or having DefaultSolrIndexQueueProvider register them on the created queue via getDeserializationFilter().addAllowedClassPatterns(...).
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| } catch (InvalidClassException e) { | ||
| LOG.error("Rejected an element from the Zookeeper queue because its type is not allowed for deserialization.", e); | ||
| throw new DistributedQueueException("Rejected an element from the Zookeeper queue because its type is not allowed for deserialization.", e); |
There was a problem hiding this comment.
🔴 A single unreadable queue entry permanently blocks all further queue processing
An entry whose type is not allowed is refused before it is deleted from storage (throw new DistributedQueueException(...) at core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/util/queue/ZookeeperDistributedQueue.java:853), so it stays at the head of the queue and every subsequent read of the queue fails forever.
Impact: One bad or unsupported message wedges the queue permanently, so all later messages are never processed until an operator manually deletes the stored entry.
Rejection propagates out of readQueueInternal without removing the entry
In readQueueInternal each entry is read inside a retryable operation that deserializes first and only deletes afterwards. The surrounding catch only swallows NoNodeException; any other RuntimeException (including the new DistributedQueueException for rejected classes) is rethrown, aborting the whole read. Since entries are processed in sorted (FIFO) order, the offending entry is re-read on every subsequent poll, take, peek, contains, drainTo, and even the contains check performed before writing new commands, so the queue can never make progress.
A more resilient behavior would be to log and skip (and optionally delete or move aside) an entry that fails the filter rather than failing the entire read.
Was this helpful? React with 👍 or 👎 to provide feedback.
A Brief Overview
ZookeeperDistributedQueue.deserialize()calledObjectInputStream.readObject()on bytes fetched from Zookeeper with no class filtering (CWE-502), so anyone able to write to the queue znodes could drive gadget-chain RCE in every node reading the queue.Every stream read from Zookeeper (queue entries and the
maxCapacityconfig node) now runs through a JDKObjectInputFilterthat rejects any class not on an allow list, before the class is resolved or instantiated:The filter lives in the new
DistributedQueueDeserializationFilter, built frommaxdepth=32;maxrefs=10000;maxbytes=1048576;maxarray=10000+ allowed patterns +!*(reject everything else). Defaults cover JDK value types, common collections andorg.broadleafcommerce.**— which is what the only in-tree user (DefaultSolrIndexQueueProvider) puts on the queue. Rejections surface as aDistributedQueueExceptionand are logged.Behavior note for subclasses/integrations: a queue carrying custom element types outside those packages must register them, otherwise reads now fail closed:
DistributedQueueDeserializationFilterTestcovers allowed value types, rejection of an unregistered type (top-level and nested inside an allowed collection), and registration.Labels: Security, critical, ready-for-code-review
Devin-Org: engineering
Link to Devin session: https://app.devin.ai/sessions/33644c5a15db45ca9116c185a877029a
Requested by: @Colhodm
Devin Review
6bd4074