-
Notifications
You must be signed in to change notification settings - Fork 1
Restrict ZookeeperDistributedQueue deserialization with an allow-list ObjectInputFilter #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop-7.0.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,8 @@ | |
| import java.io.ByteArrayInputStream; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.IOException; | ||
| import java.io.InvalidClassException; | ||
| import java.io.ObjectInputFilter; | ||
| import java.io.ObjectInputStream; | ||
| import java.io.ObjectOutputStream; | ||
| import java.io.Serializable; | ||
|
|
@@ -76,6 +78,22 @@ public class ZookeeperDistributedQueue<T extends Serializable> implements Distri | |
| public static final String QUEUE_LOCKS_FOLDER = "/locks"; | ||
| public static final String QUEUE_CONFIGS_FOLDER = "/configs"; | ||
| public static final int DEFAULT_MAX_QUEUE_SIZE = 500; | ||
|
|
||
| /** | ||
| * System property that allows additional semicolon-delimited {@link ObjectInputFilter} patterns | ||
| * (e.g. {@code com.mycompany.queue.*;com.mycompany.model.**}) to be allowed when deserializing queue entries. | ||
| */ | ||
| public static final String ADDITIONAL_ALLOWED_CLASSES_PROPERTY = "broadleaf.zookeeper.queue.deserialization.allowedClasses"; | ||
|
|
||
| /** | ||
| * Baseline set of classes that a queue entry may be composed of, along with resource limits. Anything not | ||
| * explicitly allowed is rejected by the trailing {@code !*} 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.**"; | ||
|
Comment on lines
+92
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Resource limits may reject large legitimate index batches
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||
|
|
||
| private static final Log LOG = LogFactory.getLog(ZookeeperDistributedQueue.class); | ||
| private static final String QUEUE_ENTRY_NAME = "dz-queue-entry"; | ||
|
|
||
|
|
@@ -87,6 +105,8 @@ public class ZookeeperDistributedQueue<T extends Serializable> implements Distri | |
| private final DistributedLock queueAccessLock; | ||
| private final DistributedLock configLock; | ||
| private int capacity; | ||
| private final Object DESERIALIZATION_FILTER_MONITOR = new Object(); | ||
| private volatile ObjectInputFilter deserializationFilter; | ||
|
|
||
| /** | ||
| * Constructs a folder structure in Zookeeper for managing a queue and queue state.. The argument, queuePath, should start with a forward slash ('/') and should not | ||
|
|
@@ -822,7 +842,9 @@ public void process(WatchedEvent event) { | |
| } | ||
|
|
||
| /** | ||
| * Mechanism to convert a byte array to an object. Default implementation uses {@link ObjectInputStream}. | ||
| * Mechanism to convert a byte array to an object. Default implementation uses {@link ObjectInputStream}, restricted | ||
| * by the {@link ObjectInputFilter} returned by {@link #getDeserializationFilter()} so that data read from Zookeeper | ||
| * cannot be used to instantiate arbitrary classes. | ||
| * | ||
| * @param bytes | ||
| * @return | ||
|
|
@@ -832,7 +854,13 @@ protected Object deserialize(byte[] bytes) { | |
| ObjectInputStream ois = null; | ||
| try { | ||
| ois = new ObjectInputStream(bais); | ||
| ois.setObjectInputFilter(getDeserializationFilter()); | ||
| return ois.readObject(); | ||
| } catch (InvalidClassException e) { | ||
| throw new DistributedQueueException( | ||
| "An element from the Zookeeper queue was rejected because its type is not allowed by the " | ||
| + "deserialization filter. Allowed types can be extended with the '" | ||
| + ADDITIONAL_ALLOWED_CLASSES_PROPERTY + "' system property.", e); | ||
| } catch (IOException | ClassNotFoundException e) { | ||
| throw new DistributedQueueException("Unable to deserialze an element from the Zookeeper queue.", e); | ||
| } finally { | ||
|
|
@@ -856,6 +884,36 @@ protected Object deserialize(byte[] bytes) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Allow list based filter applied to every object read from Zookeeper. Subclasses may override this, or the | ||
| * '{@value #ADDITIONAL_ALLOWED_CLASSES_PROPERTY}' system property may be used to allow additional queue entry types. | ||
| * | ||
| * @return | ||
| */ | ||
| protected ObjectInputFilter getDeserializationFilter() { | ||
| ObjectInputFilter filter = deserializationFilter; | ||
| if (filter == null) { | ||
| synchronized (DESERIALIZATION_FILTER_MONITOR) { | ||
| filter = deserializationFilter; | ||
| if (filter == null) { | ||
| filter = ObjectInputFilter.Config.createFilter(buildDeserializationFilterPattern()); | ||
| deserializationFilter = filter; | ||
| } | ||
| } | ||
| } | ||
| return filter; | ||
| } | ||
|
|
||
| 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(); | ||
| } | ||
|
Comment on lines
+907
to
+915
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟨 Deserialization allow-list can be widened to arbitrary classes via a system property The deserialization filter appends whatever is set in the Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||
|
|
||
| /** | ||
| * Mechanism to convert an object to a byte array. Default implementation uses {@link ObjectOutputStream}. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /*- | ||
| * #%L | ||
| * BroadleafCommerce Framework | ||
| * %% | ||
| * Copyright (C) 2009 - 2026 Broadleaf Commerce | ||
| * %% | ||
| * Licensed under the Broadleaf Fair Use License Agreement, Version 1.0 | ||
| * (the "Fair Use License" located at http://license.broadleafcommerce.org/fair_use_license-1.0.txt) | ||
| * unless the restrictions on use therein are violated and require payment to Broadleaf in which case | ||
| * the Broadleaf End User License Agreement (EULA), Version 1.1 | ||
| * (the "Commercial License" located at http://license.broadleafcommerce.org/commercial_license-1.1.txt) | ||
| * shall apply. | ||
| * | ||
| * Alternatively, the Commercial License may be replaced with a mutually agreed upon license (the "Custom License") | ||
| * between you and Broadleaf Commerce. You may not use this file except in compliance with the applicable license. | ||
| * #L% | ||
| */ | ||
| package org.broadleafcommerce.core.util.queue; | ||
|
|
||
| import junit.framework.TestCase; | ||
|
|
||
| import java.io.ByteArrayInputStream; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.File; | ||
| import java.io.InvalidClassException; | ||
| import java.io.ObjectInputFilter; | ||
| import java.io.ObjectInputStream; | ||
| import java.io.ObjectOutputStream; | ||
| import java.io.Serializable; | ||
| import java.util.ArrayList; | ||
|
|
||
| public class ZookeeperDistributedQueueDeserializationFilterTest extends TestCase { | ||
|
|
||
| public void testAllowedTypesAreDeserialized() throws Exception { | ||
| final ArrayList<String> payload = new ArrayList<>(); | ||
| payload.add("some-queue-entry"); | ||
|
|
||
| assertEquals(payload, readFiltered(serialize(payload))); | ||
| assertEquals(Integer.valueOf(500), readFiltered(serialize(Integer.valueOf(500)))); | ||
| } | ||
|
|
||
| public void testDisallowedTypesAreRejected() throws Exception { | ||
| final byte[] bytes = serialize(new File("/tmp/not-a-queue-entry")); | ||
|
|
||
| try { | ||
| readFiltered(bytes); | ||
| fail("Expected the deserialization filter to reject java.io.File"); | ||
| } catch (InvalidClassException e) { | ||
| assertTrue(e.getMessage().contains("filter status: REJECTED")); | ||
| } | ||
| } | ||
|
|
||
| public void testAdditionalAllowedClassesProperty() throws Exception { | ||
| System.setProperty(ZookeeperDistributedQueue.ADDITIONAL_ALLOWED_CLASSES_PROPERTY, "java.io.File"); | ||
| try { | ||
| assertEquals(new File("/tmp/allowed"), readFiltered(serialize(new File("/tmp/allowed")))); | ||
| } finally { | ||
| System.clearProperty(ZookeeperDistributedQueue.ADDITIONAL_ALLOWED_CLASSES_PROPERTY); | ||
| } | ||
| } | ||
|
|
||
| private Object readFiltered(byte[] bytes) throws Exception { | ||
| try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(bytes))) { | ||
| ois.setObjectInputFilter(ObjectInputFilter.Config.createFilter( | ||
| ZookeeperDistributedQueue.buildDeserializationFilterPattern())); | ||
| return ois.readObject(); | ||
| } | ||
| } | ||
|
|
||
| private byte[] serialize(Serializable obj) throws Exception { | ||
| final ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| try (ObjectOutputStream oos = new ObjectOutputStream(baos)) { | ||
| oos.writeObject(obj); | ||
| } | ||
| return baos.toByteArray(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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())atcore/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) forSolrUpdateCommandentries. One concrete entry type isIncrementalUpdateCommand, which holds aList<org.apache.solr.common.SolrInputDocument>(core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/IncrementalUpdateCommand.java:30), produced byAbstractSolrIndexUpdateServiceImpl.updateIndex(.../AbstractSolrIndexUpdateServiceImpl.java:270-273) and offered onto the queue at.../AbstractSolrIndexUpdateServiceImpl.java:157.DEFAULT_ALLOWED_CLASSES(lines 92-95) permits onlyjava.lang.*,java.math.*,java.time.*,java.util.*,java.util.concurrent.atomic.*andorg.broadleafcommerce.**, then rejects everything else with!*.SolrInputDocument/SolrInputFieldare inorg.apache.solr.common, so reading such an entry throwsInvalidClassException, which is converted into aDistributedQueueExceptionat 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.Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Playground