[
https://issues.apache.org/jira/browse/GROOVY-12109?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12109:
-------------------------------
Description:
A closure whose {{owner}}/{{delegate}}/{{thisObject}} references form a cycle
serializes successfully but recurses without bound when the closure is later
invoked.
Add a defensive cycle check when deserializing the built-in serializable
closure types, rejecting such graphs with an {{InvalidObjectException}}.
Legitimate closure serialization (including curried and composed closures) is
unaffected.
was:
h2. Reject Closure owner/delegate reference cycles during deserialization
h3. Cause (confirmed against groovy-5.0.6)
A cyclic {{owner}}/{{delegate}} graph recurses forever through the MOP:
{{CurriedClosure.call -> getOwner().call -> (owner is self) -> call -> ...}}.
Or this similar pure-Groovy shape {{cl.delegate = cl; cl()}}. The graph
serializes and deserializes cleanly; the blow-up is on first *invocation*. The
only attacker-controlled way to build the cycle is a forged stream, so it is
rejected there.
h3. Options ruled out (break tested behaviour)
|| Option || Why it fails ||
| Remove {{Serializable}} from {{Closure}} | Breaks all closure serialization +
real users |
| Make {{owner}}/{{delegate}}/{{thisObject}} {{transient}} | Fields come back
{{null}}; {{Groovy11272.testSerialVersionUID_2/3}} serialize _without_
{{dehydrate()}} and require the round-trip to stay callable |
h3. Fix
A {{readResolve}} hook on {{groovy.lang.Closure}} runs an iterative grey/black
DFS over the raw {{owner}}/{{delegate}}/{{thisObject}} fields (Closure-valued
links only) and throws {{InvalidObjectException}} on any cycle.
Key design points:
* *{{readResolve}}, not {{readObject}}* — a custom {{readObject}} interposes a
core-loaded frame during field reads, shifting
{{ObjectInputStream.latestUserDefinedLoader()}} off the script's
{{GroovyClassLoader}} and breaking deserialization of captured generated types
(caught by {{IntersectionClosureLiteralTest}}). {{readResolve}} runs after
fields are read, so class resolution is untouched.
* *Walk raw fields, not getters* — {{CurriedClosure.getDelegate()}} calls
{{getOwner().getDelegate()}}, which would itself StackOverflow on the malicious
graph.
* *Grey/black DFS, not plain "visited"* — a curried closure's {{owner}} and
{{delegate}} share the same wrapped clone (a diamond, not a cycle); naive
detection false-positives on every curried closure.
* {{MethodClosure}} already hard-blocks deserialization (CVE-2015-3253); its
{{readResolve}} is only widened {{private}} -> {{protected}} to override the
new hook (behaviour unchanged).
h3. Validation
|| Graph || Detected? || Want ||
| self-cycle ({{owner = this}}) — the PoC | (/) yes | yes |
| A<->B two-closure cycle | (/) yes | yes |
| legit simple closure | (/) no | no |
| legit curried closure (shared clone) | (/) no | no |
* New {{groovy.lang.ClosureSerializationCycleTest}} (4 tests): self-cycle
rejected, A<->B cycle rejected, legit closure + legit curried closure
round-trip.
* Regression: previously-failing {{IntersectionClosureLiteralTest}} now passes;
broad closure/serialization sweep all green.
h3. Residual risk (by design)
Does *not* prevent self-inflicted pure-Groovy recursion ({{cl.delegate = cl;
cl()}}) — not a vulnerability, and scoped out deliberately. Hardening targets
only the deserialization vector.
h3. Change inventory
|| File || Change ||
| {{groovy/lang/Closure.java}} | {{readResolve}} cycle check + private
{{Marker}} sentinel; 3 new imports |
| {{runtime/MethodClosure.java}} | {{readResolve}} {{private}} -> {{protected}}
(behaviour unchanged) |
| {{groovy/lang/ClosureSerializationCycleTest.groovy}} (NEW) | 4 tests |
h3. Recommendation
# Adopt the cycle-rejecting {{readResolve}} guard over a self-reference-only
check (closes A<->B at no extra cost).
# Decide the user-facing {{InvalidObjectException}} wording and whether a
changelog note is warranted.
# Run a full CI {{:test}} before merge — this path now runs on _every_ closure
deserialization.
> Harden Closure deserialization against owner/delegate reference cycle
> ---------------------------------------------------------------------
>
> Key: GROOVY-12109
> URL: https://issues.apache.org/jira/browse/GROOVY-12109
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> A closure whose {{owner}}/{{delegate}}/{{thisObject}} references form a
> cycle serializes successfully but recurses without bound when the closure is
> later invoked.
>
>
>
>
>
> Add a defensive cycle check when deserializing the built-in serializable
> closure types, rejecting such graphs with an {{InvalidObjectException}}.
> Legitimate closure serialization (including curried and composed closures) is
> unaffected.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
