[jira] [Updated] (GROOVY-12109) Harden Closure deserialization against owner/delegate reference cycle

["Paul King (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/GROOVY-12109?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Paul King updated GROOVY-12109:
-------------------------------
    Description: 
h2. Reject Closure owner/delegate reference cycles during deserialization

{panel:title=Status|borderColor=#d97706|titleBGColor=#fef3c7}
*Proposed — pending team review.* Patch + tests implemented and green locally; decision to ship (and exception wording) not yet ratified.
{panel}

h3. Summary

A hand-crafted serialized {{CurriedClosure}} whose {{owner}}/{{delegate}} point at itself triggers a {{StackOverflowError}} when deserialized-and-invoked (reported as a DoS; PoC uses _ysoserial_ + the JDK {{AnnotationInvocationHandler}} gadget).

* *Is it a CVE?* (x) No — exploitation requires an app that deserializes _untrusted_ data, which is itself the root vulnerability. Consistent with team consensus.
* *Harden anyway?* (/) Yes — as defense-in-depth, _if_ legitimate closure serialization is preserved. This change does that.

h3. Root 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

# Confirm "not a CVE" back to the reporter, with thanks; ship as defense-in-depth, not a security release.
# 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
>
> h2. Reject Closure owner/delegate reference cycles during deserialization
> {panel:title=Status|borderColor=#d97706|titleBGColor=#fef3c7}
> *Proposed — pending team review.* Patch + tests implemented and green locally; decision to ship (and exception wording) not yet ratified.
> {panel}
> h3. Summary
> A hand-crafted serialized {{CurriedClosure}} whose {{owner}}/{{delegate}} point at itself triggers a {{StackOverflowError}} when deserialized-and-invoked (reported as a DoS; PoC uses _ysoserial_ + the JDK {{AnnotationInvocationHandler}} gadget).
> * *Is it a CVE?* (x) No — exploitation requires an app that deserializes _untrusted_ data, which is itself the root vulnerability. Consistent with team consensus.
> * *Harden anyway?* (/) Yes — as defense-in-depth, _if_ legitimate closure serialization is preserved. This change does that.
> h3. Root 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
> # Confirm "not a CVE" back to the reporter, with thanks; ship as defense-in-depth, not a security release.
> # 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.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)