-
Notifications
You must be signed in to change notification settings - Fork 1
Restricting ZookeeperDistributedQueue deserialization to an allow list of classes #100
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,29 @@ 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. "com.mycompany.**") | ||
| * to be accepted when deserializing queue entries. Patterns configured here are appended to the built-in allow list. | ||
| */ | ||
| public static final String ADDITIONAL_ALLOWED_CLASSES_PROPERTY = "broadleaf.zookeeper.queue.deserialization.allowedClasses"; | ||
|
|
||
| /** | ||
| * Classes that queue entries are allowed to reference during deserialization. Everything else is rejected in order to | ||
| * prevent remote code execution via untrusted data in Zookeeper (CWE-502). | ||
| */ | ||
| protected static final String DEFAULT_ALLOWED_CLASSES = | ||
| "java.lang.Boolean;java.lang.Byte;java.lang.Character;java.lang.Double;java.lang.Enum;java.lang.Float;" | ||
| + "java.lang.Integer;java.lang.Long;java.lang.Number;java.lang.Object;java.lang.Short;java.lang.String;" | ||
| + "java.math.BigDecimal;java.math.BigInteger;java.time.*;java.util.Date;" | ||
| + "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.UUID;java.util.Map$Entry;" | ||
| + "org.broadleafcommerce.**"; | ||
|
|
||
| /** | ||
| * Resource consumption limits applied on top of the class allow list to guard against deserialization bombs. | ||
| */ | ||
| protected static final String DESERIALIZATION_LIMITS = "maxdepth=32;maxrefs=2000;maxbytes=1048576;maxarray=10000"; | ||
|
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. 🟡 Larger queued messages are rejected by overly tight size limits The resource limits applied when reading queue messages ( maxrefs/maxbytes vs. the queue's documented 1MB payload allowance
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"; | ||
|
|
||
|
|
@@ -86,6 +111,7 @@ public class ZookeeperDistributedQueue<T extends Serializable> implements Distri | |
| private final int requestedMaxQueueCapacity; | ||
| private final DistributedLock queueAccessLock; | ||
| private final DistributedLock configLock; | ||
| private volatile ObjectInputFilter deserializationFilter; | ||
| private int capacity; | ||
|
|
||
| /** | ||
|
|
@@ -822,7 +848,55 @@ public void process(WatchedEvent event) { | |
| } | ||
|
|
||
| /** | ||
| * Mechanism to convert a byte array to an object. Default implementation uses {@link ObjectInputStream}. | ||
| * The {@link ObjectInputFilter} that restricts which classes may be deserialized from the queue. Additional | ||
| * application classes can be allowed via the {@link #ADDITIONAL_ALLOWED_CLASSES_PROPERTY} system property, or by | ||
| * overriding {@link #createDeserializationFilter()}. | ||
| * | ||
| * @return the filter applied to every {@link ObjectInputStream} created by {@link #deserialize(byte[])} | ||
| */ | ||
| protected ObjectInputFilter getDeserializationFilter() { | ||
| ObjectInputFilter filter = deserializationFilter; | ||
| if (filter == null) { | ||
| synchronized (this) { | ||
| filter = deserializationFilter; | ||
| if (filter == null) { | ||
| filter = createDeserializationFilter(); | ||
| deserializationFilter = filter; | ||
| } | ||
| } | ||
| } | ||
| return filter; | ||
| } | ||
|
|
||
| /** | ||
| * Creates the {@link ObjectInputFilter} returned by {@link #getDeserializationFilter()}. | ||
| * | ||
| * @return the class allow list filter | ||
| */ | ||
| protected ObjectInputFilter createDeserializationFilter() { | ||
| return ObjectInputFilter.Config.createFilter(buildDeserializationFilterPattern()); | ||
| } | ||
|
|
||
| /** | ||
| * Assembles the {@link ObjectInputFilter} pattern from the resource limits, the built-in allow list, and any classes | ||
| * configured via the {@link #ADDITIONAL_ALLOWED_CLASSES_PROPERTY} system property. | ||
| * | ||
| * @return the filter pattern, which rejects everything that is not explicitly allowed | ||
| */ | ||
| protected static String buildDeserializationFilterPattern() { | ||
| StringBuilder pattern = new StringBuilder(DESERIALIZATION_LIMITS).append(';').append(DEFAULT_ALLOWED_CLASSES); | ||
| String additionalAllowedClasses = System.getProperty(ADDITIONAL_ALLOWED_CLASSES_PROPERTY); | ||
| if (additionalAllowedClasses != null && !additionalAllowedClasses.trim().isEmpty()) { | ||
| pattern.append(';').append(additionalAllowedClasses.trim()); | ||
| } | ||
|
|
||
| //Reject anything that was not explicitly allowed above. | ||
| return pattern.append(";!*").toString(); | ||
| } | ||
|
Comment on lines
+886
to
+895
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. 🟨 Allow list permits any org.broadleafcommerce class plus arbitrary user-supplied patterns The deserialization allow list ends with the broad wildcard Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||
|
|
||
| /** | ||
| * Mechanism to convert a byte array to an object. Default implementation uses {@link ObjectInputStream}, restricted | ||
| * to the classes permitted by {@link #createDeserializationFilter()}. | ||
| * | ||
| * @param bytes | ||
| * @return | ||
|
|
@@ -832,7 +906,12 @@ protected Object deserialize(byte[] bytes) { | |
| ObjectInputStream ois = null; | ||
| try { | ||
| ois = new ObjectInputStream(bais); | ||
| ois.setObjectInputFilter(getDeserializationFilter()); | ||
|
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. 🔍 setObjectInputFilter can throw IllegalStateException when a JVM filter factory is configured
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||
| return ois.readObject(); | ||
| } catch (InvalidClassException e) { | ||
| throw new DistributedQueueException("An element from the Zookeeper queue referenced a class that is not allowed to be " | ||
| + "deserialized. Allowed classes 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 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /*- | ||
| * #%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 static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.fail; | ||
|
|
||
| import org.junit.After; | ||
| import org.junit.Test; | ||
|
|
||
| import java.io.ByteArrayInputStream; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.io.InvalidClassException; | ||
| import java.io.ObjectInputFilter; | ||
| import java.io.ObjectInputStream; | ||
| import java.io.ObjectOutputStream; | ||
| import java.io.Serializable; | ||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
|
|
||
| /** | ||
| * Verifies that queue entries are deserialized through a class allow list rather than an unrestricted | ||
| * {@link ObjectInputStream}. | ||
| */ | ||
| public class ZookeeperDistributedQueueDeserializationTest { | ||
|
|
||
| @After | ||
| public void tearDown() { | ||
| System.clearProperty(ZookeeperDistributedQueue.ADDITIONAL_ALLOWED_CLASSES_PROPERTY); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAllowedTypesAreDeserialized() throws Exception { | ||
| ArrayList<String> list = new ArrayList<>(); | ||
| list.add("first"); | ||
| list.add("second"); | ||
| assertEquals(list, readWithFilter(serialize(list))); | ||
|
|
||
| HashMap<String, Integer> map = new HashMap<>(); | ||
| map.put("count", 2); | ||
| assertEquals(map, readWithFilter(serialize(map))); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDisallowedTypeIsRejected() throws Exception { | ||
| assertRejected(serialize(new File("/tmp/some-file"))); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDisallowedTypeNestedInAllowedCollectionIsRejected() throws Exception { | ||
| ArrayList<Serializable> list = new ArrayList<>(); | ||
| list.add(new File("/tmp/some-file")); | ||
| assertRejected(serialize(list)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAdditionalAllowedClassesProperty() throws Exception { | ||
| byte[] payload = serialize(new File("/tmp/some-file")); | ||
| assertRejected(payload); | ||
|
|
||
| System.setProperty(ZookeeperDistributedQueue.ADDITIONAL_ALLOWED_CLASSES_PROPERTY, "java.io.File"); | ||
| assertEquals(new File("/tmp/some-file"), readWithFilter(payload)); | ||
| } | ||
|
|
||
| private void assertRejected(byte[] payload) throws Exception { | ||
| try { | ||
| readWithFilter(payload); | ||
| fail("Expected the deserialization filter to reject the payload."); | ||
| } catch (InvalidClassException e) { | ||
| //Expected | ||
| } | ||
| } | ||
|
|
||
| private byte[] serialize(Serializable object) throws IOException { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| try (ObjectOutputStream oos = new ObjectOutputStream(baos)) { | ||
| oos.writeObject(object); | ||
| } | ||
| return baos.toByteArray(); | ||
| } | ||
|
|
||
| private Object readWithFilter(byte[] bytes) throws Exception { | ||
| ObjectInputFilter filter = ObjectInputFilter.Config | ||
| .createFilter(ZookeeperDistributedQueue.buildDeserializationFilterPattern()); | ||
| try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(bytes))) { | ||
| ois.setObjectInputFilter(filter); | ||
| return ois.readObject(); | ||
| } | ||
| } | ||
| } |
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.
🔴 Distributed Solr index updates stop working because the queue rejects its own messages
The list of types the queue is permitted to read back (
DEFAULT_ALLOWED_CLASSESatcore/broadleaf-framework/src/main/java/org/broadleafcommerce/core/util/queue/ZookeeperDistributedQueue.java:92-98) leaves out the Solr document types that the framework itself places on this queue, so every attempt to read a queued index-update message fails with an error.Impact: In clustered deployments, incremental Solr index updates can no longer be queued or processed, so search index changes stop being applied.
Allow list omits org.apache.solr.common types carried by IncrementalUpdateCommand
The only production user of this queue is
DefaultSolrIndexQueueProvider.createDistributedQueue(core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/DefaultSolrIndexQueueProvider.java:148), which storesSolrUpdateCommandinstances.IncrementalUpdateCommandholds aList<SolrInputDocument>(core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/IncrementalUpdateCommand.java:30), i.e.org.apache.solr.common.SolrInputDocument/SolrInputField, which are not matched by any pattern in the allow list and are therefore rejected by the trailing!*.Because
AbstractSolrIndexUpdateServiceImpl.scheduleCommandcallscommandQueue.contains(...)(core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/search/service/solr/indexer/AbstractSolrIndexUpdateServiceImpl.java:156), which reads and deserializes existing entries, both writing and consuming break once any incremental command is on the queue:deserializeconverts the filter'sInvalidClassExceptioninto aDistributedQueueException.Also note that immutable/wrapper collection implementations commonly produced by
List.of(...),Arrays.asList(...)andCollections.unmodifiable*(java.util.ImmutableCollections$*,java.util.Arrays$ArrayList,java.util.Collections$*) are likewise absent from the allow list.Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Playground