Harden the java serialization fallback bridge with a JEP-290 serial filter

[L1nq0]

Closes #9073

Upgrade note: clusters running topology.fall.back.on.java.serialization=true get a new default serial filter with this change: known gadget packages are rejected and streams are capped at 10 MB (maxbytes=10485760). Payloads that legitimately cross the fallback bridge with denied classes or larger sizes will start failing; set topology.fall.back.on.java.serialization.filter to an empty value to restore the previous unfiltered behavior.

What this adds

A new config key, topology.fall.back.on.java.serialization.filter, holding a JEP-290 filter pattern for the java serialization fallback bridge. DefaultKryoFactory parses the pattern once at kryo construction, so an invalid pattern fails worker setup with the config key in the error. SerializableSerializer installs it via setObjectInputFilter whenever it deserializes. The filter is topology-scoped and also covers programmatic construction such as local mode, which a JVM-wide jdk.serialFilter in worker.childopts cannot reach.

conf/defaults.yaml ships a default pattern: a deny-list of well-known gadget namespaces (commons-collections 3/4 functors and comparators, beanutils, xalan external and JDK-internal, rowset, c3p0, groovy closures) plus maxbytes=10485760. Sites that legitimately exchange denied classes or bigger payloads can override or clear the key per topology.

This differs from the option (2) lean in the issue thread: I ended up shipping the deny-list as the default rather than an empty value. The fallback is documented as something to keep disabled in production, but clusters still run it, and the cost of the default is a narrow, overridable deny-list while the cost of an empty default is that the dangerous path stays unconstrained for every deployment that never discovers the knob. One line in conf/defaults.yaml flips this back to opt-in; the code is identical either way. Happy to drop the default if you'd rather keep (2) pure.

Two details:

• Wildcard depth follows JEP-290: pkg.* covers direct package members, pkg.** also covers subpackages. The tests exercise both depths with a fixture in a subpackage that the single-level form would allow.
• !com.sun.org.apache.rowset.internal.* is a defensive entry: current JDKs don't carry that namespace, the real rowset gadget is covered by !com.sun.rowset.*.

Tests

8 cases in SerializableSerializerFilterTest, all running end to end through KryoValuesSerializer/KryoValuesDeserializer: reject/allow round-trips, subpackage coverage, the maxbytes limit (many small arrays, so the filter re-invokes mid-stream), unset-key no-op, fail-fast on invalid patterns, and a defaults.yaml consistency check.

Docs: the mitigation is described in docs/SECURITY.md (Serialization Security) and the new key in docs/Serialization.md.

[rzo1]

Thanks for this — and for the thorough writeup and the issue thread beforehand. I built the branch and ran the new tests locally on JDK 25: 8/8 pass, and CI is green.

The mechanism is right, and I want to say that up front: topology-scoped rather than JVM-wide, parsed once at kryo construction so a bad pattern fails fast, installed per ObjectInputStream, null/empty preserving the old behaviour, and the no-arg SerializableSerializer constructor kept for compatibility. That is the shape I would have asked for.

My objections are about the default, not the code.

1. Please don't ship the deny-list as the conf/defaults.yaml default

This is a behavioural break in a patch release for every cluster running topology.fall.back.on.java.serialization: true — new class rejections and a 10 MB cap on a path that has no cap today. You flagged this yourself in the description; I'd like to take you up on the offer and keep option (2) pure.

Ship the mechanism with an empty default, and put the recommended pattern in docs/SECURITY.md as a copy-paste block operators can adopt deliberately. Point 2 is the real argument for this.

2. As a security default, the deny-list is materially incomplete

Missing entry points that are on the JDK or the Storm worker classpath, so reachable on a stock cluster:

• javax.management.BadAttributeValueExpException — the trigger for a large share of published chains
• sun.reflect.annotation.AnnotationInvocationHandler
• com.sun.jndi.**, java.rmi.**
• clojure.** — Storm ships Clojure in the worker classpath
• org.apache.commons.fileupload.**, bsh.**, org.python.**, org.jboss.**

A twelve-entry list that stops the names we happened to think of, shipped as a default, buys false confidence: an operator reads "hardened" and stops looking. JEP-290's own guidance is allow-list first. That is a fine thing to document as a starting point for someone who has consciously enabled the fallback and knows their payload classes; it is not a good thing to enable silently on their behalf. If a deny-list does ship as the default it needs a much wider set plus an explicit "not exhaustive" note.

3. The filter only exists in DefaultKryoFactory, but defaults.yaml and the docs present it as unconditional

SerializationFactory.getKryo() (SerializationFactory.java:56) instantiates whatever topology.kryo.factory names. A custom IKryoFactory — a documented extension point — silently gets no filter even with the key set.

Either install it in SerializationFactory.getKryo after the kryoFactory.getKryo(conf) call, or state the DefaultKryoFactory-only scope in the Config javadoc and both doc pages.

4. Validate at submit time, not at worker start

@IsString only checks the type, so a bad pattern currently fails every worker at kryo construction and turns into a supervisor restart loop, with nothing surfaced at submission. A ConfigValidation validator that calls ObjectInputFilter.Config.createFilter would let nimbus reject it on submit — much better than discovering it per-worker.

Also RuntimeException → IllegalArgumentException in DefaultKryoFactory.getJavaSerializationFilter; the message itself is good.

5. Doc scoping is too broad

topology.fall.back.on.java.serialization: true also loosens DefaultStateSerializer — see docs/State-checkpointing.md:230, where it is documented as the escape hatch for state that predates kryo registration. That path builds its own new Kryo(...) rather than going through DefaultKryoFactory, so the filter never reaches it. (Lower risk, since unregistered classes there go through FieldSerializer and no readObject runs — but the new prose reads as if the whole config switch is now filtered.) Please scope the wording in Serialization.md and SECURITY.md to the tuple fallback bridge.

Smaller things

• Test fixtures squat third-party package names (org.apache.commons.collections.functors, ...comparators, com.mchange.v2.c3p0.impl) under storm-client/test/jvm. I checked and none of those artifacts is on storm-client's test classpath today, so there's no split package yet — but it's a trap for whoever adds one. Suggestion: keep one end-to-end round-trip on a Storm-owned class with a topology-scoped pattern (which the !java.util.PriorityQueue tests already do nicely), and cover the shipped defaults by implementing ObjectInputFilter.FilterInfo in the test and calling checkInput directly. Same coverage, no fake packages.
• KryoSerializableDefault.setJavaSerializationFilter is a public setter for a security control. A constructor parameter would be harder to get wrong.
• testDefaultFilterEnforcesMaxBytesLimit allocates ~11 MB per run. It's fine — the whole class runs in 0.26s — just noting it in case it ever moves somewhere hotter.

Merge order

Worth landing this after #9076. On master today this PR alone turns a gadget payload into a worker kill: the filter's InvalidClassException propagates out of SerializableSerializer.read and up through DeserializingConnectionCallback.recv() into Utils.handleUncaughtException. With #9076 in first, InvalidClassException extends IOException, so a filtered payload is dropped and counted instead — which is the behaviour this PR's documentation implies but doesn't yet get on its own.

[L1nq0]

Thanks for the thorough review, and for saying up front that the mechanism is the shape you'd have asked for. Taking the points in order:

1. Empty default: agreed, keeping option (2) pure. The defaults.yaml entry goes away, the Config javadoc keeps documenting the key, and the recommended pattern moves into docs/SECURITY.md as a copy-paste block operators adopt deliberately.

2. Deny-list completeness: agreed on the false-confidence read; a twelve-entry list of the names we happened to think of is not a security boundary. The SECURITY.md block will fold in the entries you listed (BadAttributeValueExpException, AnnotationInvocationHandler, com.sun.jndi, java.rmi, clojure, commons-fileupload, bsh, org.python, org.jboss) alongside the original set, carry an explicit not-exhaustive note, and be worded per JEP-290's allow-list-first guidance: a starting point for someone who has consciously enabled the fallback and knows their payload classes, not a boundary.

3. Factory scope: I'd rather close the gap than footnote it, so the plan is to move the installation into SerializationFactory.getKryo(), right after the kryoFactory.getKryo(conf) call, so a custom IKryoFactory gets the filter instead of silently ignoring the key; the Config javadoc will state where the filter applies. If that turns out to fight the extension point in some way I'm not seeing yet, I'll say so on this thread and fall back to documenting the DefaultKryoFactory-only scope explicitly.

4. Submit-time validation: agreed, a nimbus rejection at submit is strictly better than a supervisor restart loop. I'll add a ConfigValidation validator that runs the pattern through ObjectInputFilter.Config.createFilter, and getJavaSerializationFilter will throw IllegalArgumentException instead of RuntimeException.

5. Doc scoping: agreed. The prose in Serialization.md and SECURITY.md will name the tuple fallback bridge specifically and state that DefaultStateSerializer, the escape hatch for pre-kryo state, is not covered by this filter.

Smaller things, all accepted: the test fixtures stop squatting third-party package names; coverage keeps the end-to-end round-trips on Storm-owned classes with topology-scoped patterns, and checks the shipped pattern by implementing ObjectInputFilter.FilterInfo in the test and calling checkInput directly. setJavaSerializationFilter becomes a constructor parameter instead of a public setter. The maxbytes allocation stays as is, noted.

Merge order: agreed, and thanks for laying out the interaction. Once #9076 lands I'll rebase this on it, so what reviewers see is the composed behaviour: a filtered payload dropped and counted rather than killing the worker.

[reiabreu]

Hey folks, I'll try to provide some feedback over the weekend. Thank you