[
https://issues.apache.org/jira/browse/GROOVY-12239?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12239:
-------------------------------
Description:
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.
> 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)
