[
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 change whose value is unproven rather than disproven.
Measured across 1632 real files it produced *zero false positives*, with the
checks firing 78 times throughout, so the safety evidence is solid. It produced
*no new catches* on that corpus either - but that measures the corpus, not the
change: Groovy's test tree happens to contain no case combining
{{@ConditionalInterrupt}} with a {{SecureASTCustomizer}} restriction, and a
corpus can only show a benefit for a pattern it actually contains. The
mechanism is demonstrated by a hand-written case, and adding one such file to
the corpus would show the benefit immediately.
So the corpus is strong evidence about safety and weak evidence about need.
Whether this is worth shipping turns on how often that combination occurs in
practice, which no measurement here settles; 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; no new catches, for want of a case to catch.*
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 is known to benefit, and
there is no evidence that anyone combines {{@ConditionalInterrupt}} with
{{SecureASTCustomizer}} in practice.
Note the shape of that second argument: it is an absence of evidence, not
evidence of absence. The corpus used here is Groovy's own test tree, which was
never written to exercise that combination, so it could not have shown a
benefit however useful the change is. Someone with a view on whether the
combination occurs in the wild should decide this; no measurement here settles
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.
was:
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 change whose value is unproven rather than disproven.
> Measured across 1632 real files it produced *zero false positives*, with the
> checks firing 78 times throughout, so the safety evidence is solid. It
> produced *no new catches* on that corpus either - but that measures the
> corpus, not the change: Groovy's test tree happens to contain no case
> combining {{@ConditionalInterrupt}} with a {{SecureASTCustomizer}}
> restriction, and a corpus can only show a benefit for a pattern it actually
> contains. The mechanism is demonstrated by a hand-written case, and adding
> one such file to the corpus would show the benefit immediately.
> So the corpus is strong evidence about safety and weak evidence about need.
> Whether this is worth shipping turns on how often that combination occurs in
> practice, which no measurement here settles; 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; no new catches, for want of a case to catch.*
> 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 is known to benefit,
> and there is no evidence that anyone combines {{@ConditionalInterrupt}} with
> {{SecureASTCustomizer}} in practice.
> Note the shape of that second argument: it is an absence of evidence, not
> evidence of absence. The corpus used here is Groovy's own test tree, which
> was never written to exercise that combination, so it could not have shown a
> benefit however useful the change is. Someone with a view on whether the
> combination occurs in the wild should decide this; no measurement here
> settles 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)
