Repository navigation
Conversation
…tInputFilter allowlist (CWE-502) Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
…and cap array length Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
…so report serialClass) Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
…ytes, don't retain bulk payloads Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
…city config with a fixed filter Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
…decoder, fall back for legacy custom-format config Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
… never pass Java-stream config to the deserialize override Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
…to null before writing Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Brief Overview
ZookeeperDistributedQueue.deserialize()calledObjectInputStream.readObject()on bytes read from ZooKeeper with no class filtering. Anyone who can write to the queue znodes could plant a gadget chain and get RCE in every node that polls the queue (CWE-502, Critical).What changed (
core/broadleaf-framework/.../util/queue/ZookeeperDistributedQueue.java)deserialize()now installs a per-streamObjectInputFilter: exact allowlisted class names, ending in!*. The allowlist covers JDK value/collection types (includingList.ofimmutable collections andjava.time.*), theSolrUpdateCommandtypes, andSolrDocumentBase/SolrInputDocument/SolrInputField. It also setsmaxdepth,maxarray(1,000,000, ZooKeeper's 1MB znode cap),maxrefsandmaxbytes. Subclasses can extend the allowlist with overridable hooks.readQueueInternal()deletes a znode only when the filter actually returned REJECTED, so a poisoned head entry can't block the queue. Blockingtake/pollgo back to waiting when every scanned entry was discarded. The rejection is caught inside theGenericOperation, so it isn't retried.writeToQueue()round-trips each entry throughdeserialize()before writing. Entries the filter rejects throwIllegalArgumentException, null entries throwNullPointerException, and entries that come back null are rejected (a polled null looks like an empty queue). The exact validated bytes are re-serialized and written. A bulkaddAllis validated up front, so one bad entry rejects the whole batch, but payloads are not held in memory.maxCapacityconfig is written and read with a final, paired encoder/decoder behind an Integer-only filter. The legacy custom-format fallback todeserialize()only runs for bytes that are not a Java serialization stream.Additional context
ZookeeperDistributedQueueDeserializationTest(24 tests) plus anorg.example.gadget.Gadgettest class that sits outside the allowlist. Local run:mvn -B -q -pl core/broadleaf-framework -am test -Dtest=ZookeeperDistributedQueueDeserializationTest→Tests run: 24, Failures: 0, Errors: 0, Skipped: 0. TheInvalidClassException: filter status: REJECTEDstack traces in the log are the expected rejections being logged.develop-7.0.x). Only one of the two should land.Devin-Org: engineering
Link to Devin session: https://devin-gtm.devinenterprise.com/sessions/e1959b30293d487bb73c2011e4de60a3
Open in Devin Desktop: https://devin-gtm.devinenterprise.com/desktop/session/e1959b30293d487bb73c2011e4de60a3?variant=devin
Requested by: @Colhodm
Devin Review