[
https://issues.apache.org/jira/browse/GROOVY-12239?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King closed GROOVY-12239.
------------------------------
Resolution: Won't Fix
> Opt-in for SecureASTCustomizer annotation member processing
> -----------------------------------------------------------
>
> Key: GROOVY-12239
> URL: https://issues.apache.org/jira/browse/GROOVY-12239
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> h2. Proposal
> Annotation members are the one category of source-written code that
> {{SecureASTCustomizer}} still does not check, after GROOVY-12238 closed
> constructors, initializer blocks and field initializers. Some annotations
> take a closure as a member, and that closure is code the author of the
> secured source wrote.
> The proposal was an opt-in flag, defaulting to {{false}} so nothing changes
> for existing users:
> {code:java}
> customizer.setAnnotationMembersProcessed(true);
> {code}
> with a walk over the annotations of the class, its fields, its methods and
> constructors and their parameters, visiting each member expression that
> carries a source position (reusing the {{isFromSource}} discriminator added
> by GROOVY-12238).
> This issue records an audit of what that flag would actually buy. *The
> conclusion is that it buys very little, and the recommendation is not to ship
> it.*
> h2. Audit: annotations with closure-valued members
> Groovy declares 57 AST-transform annotations. 15 have at least one
> closure-valued member:
> {{ASTTest}}, {{AutoImplement}}, {{ClosureParams}}, {{ConditionalInterrupt}},
> {{Decreases}}, {{DelegatesTo}}, {{Ensures}}, {{ExpectedToFail}},
> {{Invariant}}, {{MapConstructor}}, {{Modifies}}, {{Option}}, {{Requires}},
> {{ThrowsIf}}, {{TupleConstructor}}
> Grouped by where the closure actually ends up by the time the customizer runs
> at CANONICALIZATION:
> ||Fate of the closure||Annotations||Checked today?||Would the flag help?||
> |Erased - the member is blanked after the transform consumes it|{{ASTTest}},
> {{TupleConstructor(pre/post)}}, {{MapConstructor(pre/post)}},
> {{AutoImplement(code)}}|no|*no* - nothing remains in the AST to visit|
> |Inlined into ordinary method bodies|{{Requires}}, {{Ensures}}, {{Invariant}}
> (and presumably {{Decreases}}, {{Modifies}}, {{ThrowsIf}} - same subproject,
> not measured)|*yes, already*|no - redundant|
> |Class literal, not executable code|{{ClosureParams}}, {{DelegatesTo}} (and
> {{Option(convert)}}, {{ExpectedToFail}} - not measured)|n/a|no|
> |Moved into a *synthetic* method|{{ConditionalInterrupt}}|no|*yes*|
> h2. Empirical results
> Measured by compiling each case under a customizer with {{disallowedReceivers
> = ['java.lang.System', 'java.lang.Runtime']}}, with the flag off and on. The
> payload in each closure member uses a disallowed receiver.
> ||Case||flag off||flag on||
> |{{@ConditionalInterrupt}}|OK|*BLOCKED*|
> |{{@Requires}}|BLOCKED|BLOCKED|
> |{{@Ensures}}|BLOCKED|BLOCKED|
> |{{@Invariant}}|BLOCKED|BLOCKED|
> |{{@AutoImplement(code=...)}}|BLOCKED|BLOCKED|
> |{{@TupleConstructor(pre=...)}}|OK|OK|
> |{{@MapConstructor(pre=...)}}|OK|OK|
> |{{@ASTTest}}|OK|OK|
> |{{@ClosureParams}}, {{@DelegatesTo}}|OK|OK|
> |controls: plain class, {{@Canonical}}, script body|unchanged|unchanged|
> Full test suite with the flag *defaulting to on*: 16551 tests, 0 failures.
> This result is not informative on its own - only 3 test files in the
> repository configure a {{SecureASTCustomizer}} at all, so the flag is inert
> everywhere else. It does establish that the walk itself introduces no
> incidental breakage.
> h2. Why annotation members turn out to be a poor lever
> Two structural reasons, both worth recording independently of this issue.
> *The closure-erasure idiom is general, not an {{@ASTTest}} quirk.* Four
> transforms read their closure member, use it, then replace the member with an
> empty closure:
> {code:java}
> anno.setMember("pre", new ClosureExpression(Parameter.EMPTY_ARRAY,
> EmptyStatement.INSTANCE));
> {code}
> {{TupleConstructorASTTransformation}}, {{MapConstructorASTTransformation}},
> {{AutoImplementASTTransformation}} and {{ASTTestTransformation}} all do this,
> deliberately, so that later phases - notably static type checking - do not
> trip over the expression. {{ASTTestTransformation}} says so in a comment:
> _"convert value into node metadata so that the expression doesn't mix up with
> other AST xforms like STC"_. No AST-level filter can see those closures at
> any phase.
> *Where the closure survives, it is usually already checked.* groovy-contracts
> inlines its conditions into ordinary method bodies, which the customizer
> already visits, so {{@Requires}}, {{@Ensures}} and {{@Invariant}} are blocked
> with the flag off.
> h2. The one case that differs is not really an annotation problem
> {{ConditionalInterruptibleASTTransformation}} moves the condition into a
> *synthetic* method:
> {code:groovy}
> type.addSyntheticMethod(conditionMethod, ACC_PRIVATE, ClassHelper.OBJECT_TYPE,
> Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, conditionNode.code)
> {code}
> and {{SecureASTCustomizer}} skips synthetic methods when visiting method
> bodies. The annotation-member route happens to catch this because the member
> also survives on the annotation, but the underlying gap is the
> synthetic-method skip. A fix there would generalise, though it carries its
> own false-positive exposure, since "synthetic" covers a great deal of
> compiler-generated code that should not be checked.
> h2. Recommendation
> Do not ship the flag. A new public property on {{SecureASTCustomizer}} which
> changes behaviour for exactly one annotation, and whose real cause lies
> elsewhere, is not worth the API surface on a class that already has many
> knobs.
> Instead, extend the {{Limitations}} sections in the user guide and the
> {{SecureASTCustomizer}} javadoc to explain *why* annotation members are
> largely unreachable, rather than the current bare statement that they are not
> visited. That is more useful to a reader deciding whether the customizer fits
> their needs.
> If the synthetic-method gap is judged worth pursuing, it should be a separate
> issue scoped to that question.
> h2. Relationship to GROOVY-12238
> The implementation prototyped here builds on GROOVY-12238, reusing its
> {{isFromSource(ASTNode)}} helper. If GROOVY-12238 does not land, this would
> need its own copy of that three-line discriminator.
> h2. Scope note
> {{SecureASTCustomizer}} is a best-effort grammar filter, not a security
> boundary - see THREAT_MODEL.md sections 3, 9 and 11a. Neither this proposal
> nor its rejection changes that position.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
