[
https://issues.apache.org/jira/browse/GROOVY-12244?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King resolved GROOVY-12244.
--------------------------------
Fix Version/s: 6.0.0-beta-2
Resolution: Fixed
> 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
> Fix For: 6.0.0-beta-2
>
>
> h2. Summary up front
> This is about *consistency of treatment*, not about catching more code.
> Someone who configures {{disallowedReceivers = ['java.lang.System']}} holds a
> single expectation: System calls are rejected in the source they compile.
> After GROOVY-12238 that expectation holds everywhere - method bodies,
> constructor bodies, static and instance initializers, field initializers,
> closures relocated into a generated constructor by
> {{@TupleConstructor(pre=...)}}, groovy-contracts conditions inlined into loop
> bodies - with exactly one exception: {{@ConditionalInterrupt}}.
> Nothing visible to the user explains the exception. It exists because that
> transformation relocates its closure into a *synthetic* method while the
> others relocate into constructors, ordinary methods or generated classes, and
> the customizer skips synthetic methods. That is an implementation detail of
> one transformation leaking into observable behaviour, and no user could be
> expected to predict it.
> Measured across 1632 real files the change produces *zero false positives*,
> with the checks firing 78 times throughout. It produced no new catches on
> that corpus, because Groovy's test tree happens to contain no case combining
> {{@ConditionalInterrupt}} with a restriction - but the case for the change
> does not rest on catching more code, it rests on the rule being uniform.
> 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}
> Set against the other transformations which relocate a user-supplied closure,
> {{@ConditionalInterrupt}} is the only one left uncovered:
> ||Transformation||Where the authored closure lands||Checked after
> GROOVY-12238?||
> |{{@TupleConstructor(pre/post)}}, {{@MapConstructor(pre/post)}}|generated
> constructor|yes|
> |{{@AutoImplement(code)}}|generated method|yes|
> |groovy-contracts {{@Requires}}/{{@Ensures}}/{{@Invariant}}|generated closure
> class added to the module|yes|
> |groovy-contracts loop {{@Invariant}}/{{@Decreases}}|inlined into the loop
> body|yes|
> |*{{@ConditionalInterrupt}}*|*synthetic method*|*no*|
> |{{@ASTTest}}|nowhere in the AST - rebuilt from raw source text|no, and
> unreachable by any AST-level check; it has its own off-switch,
> {{groovy.asttest.enable}}|
> Each destination was chosen for a good reason - {{@ConditionalInterrupt}}
> injects at every method start and every loop, so one method called many times
> is cheaper than inlining the condition dozens of times per class. The point
> is not that the transformation is wrong, but that its choice is invisible to
> whoever configured the customizer.
> 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
> The case for it is consistency. A user configuring the customizer has no way
> to know that one transformation relocates into a synthetic method while its
> neighbours relocate into constructors, generated methods, generated classes
> or the loop body itself, and that the customizer looks in every one of those
> but the first. Uniform treatment is what makes the configuration
> comprehensible; an exception that cannot be predicted from the source is the
> kind of surprise THREAT_MODEL.md section 3 identifies as worth acting on,
> whether or not it is common.
> It also generalises. The fix catches *any* transformation that relocates
> authored code into a synthetic method, including third-party ones that will
> never be reviewed here, which is the property that a per-transformation fix
> would not have.
> The case against is that one first-party annotation is known to benefit, and
> there is no evidence that anyone combines {{@ConditionalInterrupt}} with
> {{SecureASTCustomizer}} today.
> That objection is worth weighing carefully, because it is an absence of
> evidence rather than evidence of absence: the corpus is Groovy's own test
> tree, never written to exercise that combination, so it could not have shown
> a benefit however useful the change is. But the consistency argument does not
> depend on the answer. Even at zero current users, the rule "restrictions
> apply to the code you wrote" either holds uniformly or it does not, and today
> it does not.
> 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)
