[
https://issues.apache.org/jira/browse/GROOVY-12244?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12244:
-------------------------------
Description:
h2. Summary up front
This is a small, safe and *speculative* change. Measured across 1632 real files
it produces zero false positives - and also zero benefit, because nothing in
that corpus exercises it. The only demonstrated case is hand-written. It is
offered on that basis rather than as something known to be needed; see "Is it
worth doing" below.
Depends on GROOVY-12238, which introduces the two helpers it reuses.
h2. Problem
{{SecureASTCustomizer}} skips synthetic methods when visiting method bodies:
{code:java}
for (MethodNode methodNode : clNode.getMethods()) {
if (!methodNode.isSynthetic() && methodNode.getCode() != null) {
methodNode.getCode().visit(visitor);
}
}
{code}
That is right for compiler-generated members, but a transformation may relocate
code the *user wrote* into a synthetic method.
{{ConditionalInterruptibleASTTransformation}} does exactly this - it lifts the
closure supplied to {{@ConditionalInterrupt}} into a private synthetic method
and injects calls to it at every method start and every loop:
{code:groovy}
type.addSyntheticMethod(conditionMethod, ACC_PRIVATE, ClassHelper.OBJECT_TYPE,
Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, conditionNode.code)
{code}
So with {{disallowedReceivers = ['java.lang.System']}} configured, this
compiles and runs today:
{code:groovy}
import groovy.transform.ConditionalInterrupt
@ConditionalInterrupt({ System.getProperty('x') != null })
class A { def m() { 1 } }
{code}
while the same call written directly in a method body is correctly rejected.
This is the same relocation story as GROOVY-12238, one destination further on.
There, authored code moved into a generated *constructor* escaped the checks;
here it is a synthetic *method*. Dumping the AST at CANONICALIZATION shows the
code arrives with its source position intact, exactly as in the constructor
case:
{noformat}
@ConditionalInterrupt({ System.getProperty('x') != null })
method conditionalTransform...$condition line=-1 synthetic=true codeLine=2
stmt[0] line=2 (java.lang.System.getProperty(x) != null) <- authored,
relocated here
{noformat}
h2. Change
About six lines, reusing {{visitAuthoredStatementsOf}} and {{isFromSource}}
from GROOVY-12238:
{code:java}
for (MethodNode method : clNode.getMethods()) {
if (method.isSynthetic() && !"<clinit>".equals(method.getName()) &&
method.getCode() != null) {
visitAuthoredStatementsOf(method.getCode(), visitor);
}
}
{code}
The per-statement source-position filter is what makes this safe: a synthetic
method's *generated* statements carry no source position and are skipped, while
relocated authored statements carry one and are checked. {{<clinit>}} is
excluded because GROOVY-12238 already handles it.
h2. Evidence
*Exposure is far narrower than "synthetic" suggests.* Nine common constructs
were scanned for synthetic methods containing source-positioned statements -
trait implementations, {{@Delegate}}, records, enums, plain properties,
closures in methods, {{@Immutable}}, {{@Sortable}}, {{@ConditionalInterrupt}}.
Only {{@ConditionalInterrupt}} produced one. Generated accessors, delegate
forwarders, record components, enum machinery and trait bridges all carry
unpositioned statements, so the filter excludes them without needing to
enumerate them. Notably, a trait's method body is *not* copied onto the
implementing class as a source-positioned synthetic method, so there is no
duplicate-checking problem there.
*Corpus: zero false positives, and zero benefit.* Every {{.groovy}} file under
{{src/test}} (1632 files; 22 using {{@Grab}}/{{@Grapes}} excluded) compiled
under a {{SecureASTCustomizer}} with {{disallowedReceivers =
['java.lang.System', 'java.lang.Thread', 'java.lang.Runtime',
'java.lang.ProcessBuilder']}}, on top of GROOVY-12238, with and without this
change:
||Corpus outcome||Without||With||
|Compiled|1554|1554|
|Rejected|78|78|
Byte-identical. The 78 rejections show the checks were live throughout, so the
unchanged verdicts mean the change was exercised and stayed quiet. But no file
in the corpus was newly caught either: the three files using
{{@ConditionalInterrupt}} have conditions that do not touch the restricted
receivers, so the corpus could not demonstrate an upside.
*Hand-written case:* {{@ConditionalInterrupt}} with a disallowed receiver is
permitted without the change and rejected with it. Full test suite passes.
h2. Is it worth doing
Arguments for: it is small, it reuses machinery already being added, it
completes the relocation story rather than leaving one destination uncovered,
and it generalises - it catches *any* transformation that relocates authored
code into a synthetic method, including third-party ones that will never be
reviewed here.
Arguments against: exactly one first-party annotation benefits, and no evidence
exists that anyone combines {{@ConditionalInterrupt}} with
{{SecureASTCustomizer}} in practice.
Someone with a view on whether that combination occurs in the wild should
decide this; the measurements above do not settle it.
h2. Why not change the transform instead
Making the condition method non-synthetic would remove the need for this
change, and looks like a one-word fix. It is not:
* {{isSynthetic()}} is load-bearing in three places in
{{ConditionalInterruptibleASTTransformation}} - the injection-eligibility
check, the traversal check, and a defensive guard on the condition method
itself. Two of them are what stop the condition method being instrumented with
its own interrupt check.
* {{filterMethods}} excludes synthetic methods, so a non-synthetic condition
method would count as a *method definition*. Any class using
{{@ConditionalInterrupt}} would then be rejected under
{{methodDefinitionAllowed = false}} - a new false positive in
{{SecureASTCustomizer}}, created by trying to make code visible to
{{SecureASTCustomizer}}.
More broadly, the transform is not doing anything wrong. It relocates authored
code into the AST with its source position intact, which is the convention
documented in {{ARCHITECTURE.md}} and the user guide; the customizer is simply
not looking there. Fixing the consumer covers every transformation at once,
whereas changing this transformation fixes one and leaves the next one silently
uncovered.
h2. Scope note
{{SecureASTCustomizer}} is a best-effort grammar filter, not a security
boundary - see THREAT_MODEL.md sections 3, 9 and 11a. This is hardening which
removes surprising behaviour; it does not alter that position, and a
demonstrated bypass remains by design rather than a vulnerability.
> SecureASTCustomizer does not check authored code relocated into a synthetic
> method
> ----------------------------------------------------------------------------------
>
> Key: GROOVY-12244
> URL: https://issues.apache.org/jira/browse/GROOVY-12244
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Summary up front
> This is a small, safe and *speculative* change. Measured across 1632 real
> files it produces zero false positives - and also zero benefit, because
> nothing in that corpus exercises it. The only demonstrated case is
> hand-written. It is offered on that basis rather than as something known to
> be needed; see "Is it worth doing" below.
> Depends on GROOVY-12238, which introduces the two helpers it reuses.
> h2. Problem
> {{SecureASTCustomizer}} skips synthetic methods when visiting method bodies:
> {code:java}
> for (MethodNode methodNode : clNode.getMethods()) {
> if (!methodNode.isSynthetic() && methodNode.getCode() != null) {
> methodNode.getCode().visit(visitor);
> }
> }
> {code}
> That is right for compiler-generated members, but a transformation may
> relocate code the *user wrote* into a synthetic method.
> {{ConditionalInterruptibleASTTransformation}} does exactly this - it lifts
> the closure supplied to {{@ConditionalInterrupt}} into a private synthetic
> method and injects calls to it at every method start and every loop:
> {code:groovy}
> type.addSyntheticMethod(conditionMethod, ACC_PRIVATE, ClassHelper.OBJECT_TYPE,
> Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, conditionNode.code)
> {code}
> So with {{disallowedReceivers = ['java.lang.System']}} configured, this
> compiles and runs today:
> {code:groovy}
> import groovy.transform.ConditionalInterrupt
> @ConditionalInterrupt({ System.getProperty('x') != null })
> class A { def m() { 1 } }
> {code}
> while the same call written directly in a method body is correctly rejected.
> This is the same relocation story as GROOVY-12238, one destination further
> on. There, authored code moved into a generated *constructor* escaped the
> checks; here it is a synthetic *method*. Dumping the AST at CANONICALIZATION
> shows the code arrives with its source position intact, exactly as in the
> constructor case:
> {noformat}
> @ConditionalInterrupt({ System.getProperty('x') != null })
> method conditionalTransform...$condition line=-1 synthetic=true codeLine=2
> stmt[0] line=2 (java.lang.System.getProperty(x) != null) <- authored,
> relocated here
> {noformat}
> h2. Change
> About six lines, reusing {{visitAuthoredStatementsOf}} and {{isFromSource}}
> from GROOVY-12238:
> {code:java}
> for (MethodNode method : clNode.getMethods()) {
> if (method.isSynthetic() && !"<clinit>".equals(method.getName()) &&
> method.getCode() != null) {
> visitAuthoredStatementsOf(method.getCode(), visitor);
> }
> }
> {code}
> The per-statement source-position filter is what makes this safe: a synthetic
> method's *generated* statements carry no source position and are skipped,
> while relocated authored statements carry one and are checked. {{<clinit>}}
> is excluded because GROOVY-12238 already handles it.
> h2. Evidence
> *Exposure is far narrower than "synthetic" suggests.* Nine common constructs
> were scanned for synthetic methods containing source-positioned statements -
> trait implementations, {{@Delegate}}, records, enums, plain properties,
> closures in methods, {{@Immutable}}, {{@Sortable}},
> {{@ConditionalInterrupt}}. Only {{@ConditionalInterrupt}} produced one.
> Generated accessors, delegate forwarders, record components, enum machinery
> and trait bridges all carry unpositioned statements, so the filter excludes
> them without needing to enumerate them. Notably, a trait's method body is
> *not* copied onto the implementing class as a source-positioned synthetic
> method, so there is no duplicate-checking problem there.
> *Corpus: zero false positives, and zero benefit.* Every {{.groovy}} file
> under {{src/test}} (1632 files; 22 using {{@Grab}}/{{@Grapes}} excluded)
> compiled under a {{SecureASTCustomizer}} with {{disallowedReceivers =
> ['java.lang.System', 'java.lang.Thread', 'java.lang.Runtime',
> 'java.lang.ProcessBuilder']}}, on top of GROOVY-12238, with and without this
> change:
> ||Corpus outcome||Without||With||
> |Compiled|1554|1554|
> |Rejected|78|78|
> Byte-identical. The 78 rejections show the checks were live throughout, so
> the unchanged verdicts mean the change was exercised and stayed quiet. But no
> file in the corpus was newly caught either: the three files using
> {{@ConditionalInterrupt}} have conditions that do not touch the restricted
> receivers, so the corpus could not demonstrate an upside.
> *Hand-written case:* {{@ConditionalInterrupt}} with a disallowed receiver is
> permitted without the change and rejected with it. Full test suite passes.
> h2. Is it worth doing
> Arguments for: it is small, it reuses machinery already being added, it
> completes the relocation story rather than leaving one destination uncovered,
> and it generalises - it catches *any* transformation that relocates authored
> code into a synthetic method, including third-party ones that will never be
> reviewed here.
> Arguments against: exactly one first-party annotation benefits, and no
> evidence exists that anyone combines {{@ConditionalInterrupt}} with
> {{SecureASTCustomizer}} in practice.
> Someone with a view on whether that combination occurs in the wild should
> decide this; the measurements above do not settle it.
> h2. Why not change the transform instead
> Making the condition method non-synthetic would remove the need for this
> change, and looks like a one-word fix. It is not:
> * {{isSynthetic()}} is load-bearing in three places in
> {{ConditionalInterruptibleASTTransformation}} - the injection-eligibility
> check, the traversal check, and a defensive guard on the condition method
> itself. Two of them are what stop the condition method being instrumented
> with its own interrupt check.
> * {{filterMethods}} excludes synthetic methods, so a non-synthetic condition
> method would count as a *method definition*. Any class using
> {{@ConditionalInterrupt}} would then be rejected under
> {{methodDefinitionAllowed = false}} - a new false positive in
> {{SecureASTCustomizer}}, created by trying to make code visible to
> {{SecureASTCustomizer}}.
> More broadly, the transform is not doing anything wrong. It relocates
> authored code into the AST with its source position intact, which is the
> convention documented in {{ARCHITECTURE.md}} and the user guide; the
> customizer is simply not looking there. Fixing the consumer covers every
> transformation at once, whereas changing this transformation fixes one and
> leaves the next one silently uncovered.
> h2. Scope note
> {{SecureASTCustomizer}} is a best-effort grammar filter, not a security
> boundary - see THREAT_MODEL.md sections 3, 9 and 11a. This is hardening which
> removes surprising behaviour; it does not alter that position, and a
> demonstrated bypass remains by design rather than a vulnerability.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
