[
https://issues.apache.org/jira/browse/GROOVY-12109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18092028#comment-18092028
]
ASF GitHub Bot commented on GROOVY-12109:
-----------------------------------------
codecov-commenter commented on PR #2634:
URL: https://github.com/apache/groovy/pull/2634#issuecomment-4817379222
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2634?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `89.65517%` with `3 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 68.5103%. Comparing base
([`568bf98`](https://app.codecov.io/gh/apache/groovy/commit/568bf980dfd617c3e98d62b51ce38d74e9ae3490?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`facf4d5`](https://app.codecov.io/gh/apache/groovy/commit/facf4d5bc32e51c8ed1de331545179c0844440d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2634?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[src/main/java/groovy/lang/Closure.java](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FClosure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9DbG9zdXJlLmphdmE=)
| 89.6552% | [1 Missing and 2 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2634 +/- ##
==================================================
- Coverage 68.5180% 68.5103% -0.0077%
- Complexity 33597 33599 +2
==================================================
Files 1519 1519
Lines 127384 127413 +29
Branches 23123 23130 +7
==================================================
+ Hits 87281 87291 +10
- Misses 32355 32365 +10
- Partials 7748 7757 +9
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2634?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...ava/org/codehaus/groovy/runtime/MethodClosure.java](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FMethodClosure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvTWV0aG9kQ2xvc3VyZS5qYXZh)
| `80.7692% <ø> (ø)` | |
|
[src/main/java/groovy/lang/Closure.java](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FClosure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9DbG9zdXJlLmphdmE=)
| `79.2857% <89.6552%> (+1.1981%)` | :arrow_up: |
... and [6 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2634/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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
> 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
