[
https://issues.apache.org/jira/browse/GROOVY-12238?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103527#comment-18103527
]
ASF GitHub Bot commented on GROOVY-12238:
-----------------------------------------
paulk-asert closed pull request #2771: GROOVY-12238: SecureASTCustomizer does
not check constructors, initia…
URL: https://github.com/apache/groovy/pull/2771
> SecureASTCustomizer does not check constructors, initializer blocks or field
> initializers
> -----------------------------------------------------------------------------------------
>
> Key: GROOVY-12238
> URL: https://issues.apache.org/jira/browse/GROOVY-12238
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Problem
> {{SecureASTCustomizer}} visits the script statement block and method bodies
> only. Code which lives outside a method body is never handed to the securing
> visitor, so none of the configured restrictions apply to it - not
> {{disallowedReceivers}}, not the statement or expression allow/deny lists,
> and not any registered {{StatementChecker}} or {{ExpressionChecker}}.
> With {{disallowedReceivers = ['java.lang.System']}} configured, every one of
> the following compiles and runs today, while the same call in the script body
> is correctly rejected:
> {code:groovy}
> class A { A() { System.getProperty('x') } } // constructor
> class B { static { System.getProperty('x') } } // static initializer
> class C { { System.getProperty('x') } } // instance
> initializer
> class D { def f = System.getProperty('x') } // field initializer
> class E { static def f = System.getProperty('x') } // static field
> initializer
> @TupleConstructor(pre={ System.getProperty('x') }) // relocated into the
> class F { String s } // generated
> constructor
> {code}
> The existing filters cannot be reused to reach these, for three separate
> reasons:
> * a static initializer block ends up inside a {{<clinit>}} method, and
> {{<clinit>}} is *synthetic*, so it is excluded both by {{filterMethods}} and
> by the {{!isSynthetic()}} test in the sibling-class loop;
> * instance initializer blocks live in
> {{ClassNode.getObjectInitializerStatements()}}, a separate list which is
> never read;
> * field initializers live in {{FieldNode.getInitialExpression()}}, and the
> backing fields of Groovy properties are themselves *synthetic*, so the
> synthetic flag cannot be used to filter them either.
> h2. Change
> Adds {{SecureASTCustomizer.visitConstructorsAndInitializers(ClassNode,
> GroovyCodeVisitor)}}, called for every class in the module, which applies the
> existing securing visitor to:
> * declared constructors (non-synthetic, with a body)
> * {{getObjectInitializerStatements()}}
> * the statements inside {{<clinit>}}
> * {{FieldNode.getInitialExpression()}}
> The method is {{protected}} so subclasses can adjust it, consistent with
> {{createGroovyCodeVisitor}} and {{filterMethods}}.
> h2. Distinguishing generated code
> This is the part worth reviewing closely.
> Constructors and initializers are not written solely by the author of the
> source being secured. The compiler generates constructors for every script
> class, and AST transformations add constructors, fields and initializer
> statements of their own. Checking those rejects valid programs rather than
> restricting the author.
> A first cut which visited everything broke 4 of the 81 existing customizer
> tests, all on generated code:
> {noformat}
> ConstructorCallExpressions are not allowed: super (context)
> Usage of variables of type [groovy.lang.Binding] is not allowed
> Indirect import checks prevents usage of expression (x2)
> {noformat}
> Those are the script class's generated {{Script()}} and {{Script(Binding)}}
> constructors - and they are *not* marked synthetic, so there is no flag
> available to exclude them. The discriminator used instead is the source
> position: a node is visited only when {{getLineNumber() > 0}}, expressed as
> an {{isFromSource(ASTNode)}} helper. Generated nodes normally carry no source
> position.
> Caveats a reviewer should weigh:
> * This is a heuristic, not a guarantee. There are ~220 {{setSourcePosition}}
> calls in the main source; a transformation which copies a source position
> onto a generated constructor, field initializer or initializer statement will
> have that code checked. {{@Grab}} looks safe by inspection - it injects via
> {{addStaticInitializerStatements}} and {{addObjectInitializerStatements}}
> using {{stmt()}}/{{callX()}} helpers, which do not set positions - but
> {{@Grab}} was not exercised directly.
> * *The heuristic does fire on generated nodes in practice, and when it does
> it errs the safe way.* groovy-contracts positions its generated wrappers
> deliberately: {{LoopInvariantASTTransformation}} calls
> {{wrapped.setSourcePosition(annotation)}} on a try/catch block built by
> {{TryCatchBlockGenerator}}, and {{AnnotationClosureVisitor}} calls
> {{value.setSourcePosition(annotationNode)}} on the {{ClassExpression}} it
> substitutes for the closure. Both are generated nodes carrying a source
> position, so both are visited. That is the correct outcome rather than a
> false positive: the wrapper is generated but the code inside it is the
> contract the user wrote, so checking it checks authored code. The point for
> review is that the failure mode is now observed rather than hypothetical, and
> its direction is to check slightly more than intended, never less. A missed
> check would be the dangerous direction, and the heuristic cannot produce one
> - generated code that carries no position is skipped, and authored code
> always carries one.
> * The {{<clinit>}} method's wrapper {{BlockStatement}} is itself generated
> even when its statements are not, so the filter has to be applied per
> statement rather than to the method body. Applying it at the method level
> leaves static initializer blocks open.
> h2. Authored code relocated into generated members
> A generated member may nonetheless *contain* code the author wrote, because a
> transformation can move it there. {{@TupleConstructor(pre=...)}} and
> {{@MapConstructor(pre=...)}} relocate the supplied closure body into the
> constructor they generate. Dumping the AST at CANONICALIZATION shows what
> happens:
> {noformat}
> @TupleConstructor(pre={ System.getProperty('x') })
> ctor line=-1 synthetic=false
> stmt[0] line=2 java.lang.System.getProperty(x) <- authored, relocated
> here
> stmt[1] line=-1 (this.s = s) <- generated
> {noformat}
> The author's statement keeps the source position it had in the original
> source. So a member with no source position of its own is filtered *statement
> by statement* rather than skipped outright - the same treatment {{<clinit>}}
> already needs. Without this, {{@TupleConstructor(pre=...)}} and
> {{@MapConstructor(pre=...)}} escape the restrictions entirely.
> This also answers a question raised separately on GROOVY-12239, which
> proposed visiting annotation members. Annotation members turn out to be the
> wrong lever: by CANONICALIZATION the closure supplied to an annotation has
> usually been *moved* - into a constructor or a method - where it remains
> reachable by source position without touching annotations at all.
> GROOVY-12239 was closed on that basis.
> h2. Testing
> {{SecureASTCustomizerTest}} goes from 39 to 49 tests:
> ||Test||Purpose||
> |{{testDisallowedReceiverInScriptBody}}|control; unchanged behaviour|
> |{{testDisallowedReceiverInConstructor}}|closed gap|
> |{{testDisallowedReceiverInStaticInitializer}}|closed gap|
> |{{testDisallowedReceiverInObjectInitializer}}|closed gap|
> |{{testDisallowedReceiverInFieldInitializer}}|closed gap|
> |{{testDisallowedReceiverInStaticFieldInitializer}}|closed gap|
> |{{testDisallowedReceiverMovedIntoGeneratedConstructor}}|{{@TupleConstructor(pre=...)}}
> relocation|
> |{{testDisallowedReceiverMovedIntoGeneratedMapConstructor}}|{{@MapConstructor(pre=...)}}
> relocation|
> |{{testGeneratedScriptConstructorsAreNotChecked}}|generated
> {{super(Binding)}} stays exempt|
> |{{testTransformGeneratedConstructorIsNotChecked}}|{{@TupleConstructor}}
> output stays exempt|
> Verified:
> * the five gap tests and the two relocation tests fail against unmodified
> master and pass with the change; the script-body control passes in both
> * the two exemption tests pass in both, by design - they exist to stop a
> later simplification from dropping the source-position check, which is the
> property most likely to regress silently
> * probe scenarios run during development ({{@Singleton}}, {{@Immutable}},
> {{@TupleConstructor}}, {{@Canonical}}, {{@Lazy}}, {{@Delegate}}, traits,
> enums, records, inner classes, user constructors) under both a
> receiver-restriction config and an allow-list config showed results identical
> to baseline
> * full test suite: 16553 tests, no failures
> h3. Corpus measurement
> Rather than rely on hand-written scenarios alone, the change was measured
> against a real corpus: every {{.groovy}} file under {{src/test}} (1632 files;
> 22 using {{@Grab}}/{{@Grapes}} excluded to avoid network resolution), each
> compiled under a {{SecureASTCustomizer}} configured with
> {{disallowedReceivers = ['java.lang.System', 'java.lang.Thread',
> 'java.lang.Runtime', 'java.lang.ProcessBuilder']}} and
> {{groovy.asttest.enable=false}}.
> The change was isolated by reverting {{SecureASTCustomizer.java}} to the
> pre-change version on the same branch, so that nothing else in the branch
> could affect the comparison.
> ||Corpus outcome||Without the change||With the change||
> |Compiled|1558|1554|
> |Rejected|74|*78*|
> *4 files newly rejected, all four genuine, no false positives.* Each is
> authored code sitting in one of the constructs this issue is about:
> ||File||Construct||Code||
> |{{bugs/StaticClosurePropertyBug.groovy}}|static field initializer|{{static
> def out = \{System.out.println(it)\}}}|
> |{{runtime/WriterAppendTest.groovy}}|static initializer
> block|{{defaultEncoding = System.getProperty(...)}}|
> |{{groovy/execute/ExecuteTest_LinuxSolaris.groovy}}|static field
> initializer|{{System.getProperty('os.name')}}|
> |{{groovy/execute/ExecuteTest_Windows.groovy}}|static field
> initializer|{{System.properties['os.name']}}|
> Two things follow. The gap is real rather than theoretical - four files in
> Groovy's own test tree put restricted calls in static field initializers and
> a static block, and the customizer waved them through. And the checks were
> live throughout: 74 rejections already occurred at baseline, so an unchanged
> verdict on the other 1628 files means the change was exercised and produced
> no false positive, not that the restriction never fired.
> What the corpus does *not* cover: it produced no instance-initializer or
> constructor-body hits, and no
> {{@TupleConstructor(pre=...)}}/{{@MapConstructor(pre=...)}} relocation cases.
> Those parts of the change rest on the unit tests above rather than on corpus
> evidence.
> h2. Compatibility
> This is a behavioural change: scripts which compile today under a
> {{SecureASTCustomizer}} will be rejected if their constructors, initializers
> or relocated closure bodies violate the configured restrictions. That is the
> intent, but per
> [COMPATIBILITY.md|https://github.com/apache/groovy/blob/master/COMPATIBILITY.md]
> it is a breaking change and wants a dev@ discussion plus a major version.
> Targeted at 6.0.
> Both {{Limitations}} sections - the user guide and the
> {{SecureASTCustomizer}} javadoc - are updated in the same change, since they
> currently document these gaps as behaviour.
> h2. Out of scope
> * *Annotation members* remain unvisited - see GROOVY-12239 for the audit and
> the reasoning above.
> * *Synthetic methods* remain unvisited. A transformation may relocate
> authored code there too: {{ConditionalInterruptibleASTTransformation}} moves
> its condition into a method created by {{addSyntheticMethod}}, and applying
> the same per-statement filter there would catch {{@ConditionalInterrupt}}.
> That was prototyped and works, but it buys one annotation and "synthetic"
> covers a great deal of compiler-generated code, so it deserves broader
> evidence before shipping. Deliberately deferred.
> * *Constructors still do not count towards {{methodDefinitionAllowed}}*.
> Their bodies are now checked, but declaring a constructor remains permitted.
> Making constructors count is a second, separable breaking change.
> * *{{@ASTTest}} is unreachable on every path*, because its transformation
> moves the closure out of the AST into node metadata and reconstructs it from
> the raw source text.
> h2. Scope note
> {{SecureASTCustomizer}} is a best-effort grammar filter, not a security
> boundary - see THREAT_MODEL.md sections 3, 9 and 11a. This change is
> hardening which removes behaviour that is surprising to a developer following
> the documentation; 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)
