[
https://issues.apache.org/jira/browse/GROOVY-12238?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18102752#comment-18102752
]
ASF GitHub Bot commented on GROOVY-12238:
-----------------------------------------
paulk-asert opened a new pull request, #2771:
URL: https://github.com/apache/groovy/pull/2771
…lizer blocks or field initializers
SecureASTCustomizer visited the script statement block and method bodies
only, so code outside a method body escaped every configured restriction:
disallowedReceivers, the statement and expression allow/deny lists, and any
registered StatementChecker or ExpressionChecker. With disallowedReceivers =
['java.lang.System'], a call in a constructor, a static or instance initializer
block, or a field initializer all compiled and ran, while the same call in the
script body was correctly rejected.
The existing filters could not reach these. A static initializer ends up in
<clinit>, which is synthetic and so excluded by filterMethods; instance
initializers live in a separate getObjectInitializerStatements() list; and
field initializers hang off FieldNode, whose property backing fields are
themselves synthetic.
Add visitConstructorsAndInitializers(), applying the securing visitor to
declared constructors, object initializer statements, the statements inside
<clinit>, and field initial expressions.
Only nodes carrying a source position are visited. Constructors and
initializers are not written solely by the author of the secured source: every
script class has generated constructors, and AST transformations add their own.
Visiting those rejects valid programs
> 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}}.
> Four constructs are affected: constructor bodies, static initializer blocks,
> instance (object) initializer blocks, and field initializer expressions
> (static and instance).
> 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
> {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.
> Two 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
> would have that code checked. Nothing found in testing does so, and {{@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. If the
> heuristic does misfire, the failure mode is a false rejection of a valid
> program, not a silent hole.
> * The {{<clinit>}} method's wrapper {{BlockStatement}} is itself synthetic
> 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. Testing
> {{SecureASTCustomizerTest}} goes from 39 to 47 tests:
> ||Test||Purpose||
> |{{testDisallowedReceiverInScriptBody}}|control; unchanged behaviour|
> |{{testDisallowedReceiverInConstructor}}|closed gap|
> |{{testDisallowedReceiverInStaticInitializer}}|closed gap|
> |{{testDisallowedReceiverInObjectInitializer}}|closed gap|
> |{{testDisallowedReceiverInFieldInitializer}}|closed gap|
> |{{testDisallowedReceiverInStaticFieldInitializer}}|closed gap|
> |{{testGeneratedScriptConstructorsAreNotChecked}}|generated
> {{super(Binding)}} stays exempt|
> |{{testTransformGeneratedConstructorIsNotChecked}}|{{@TupleConstructor}}
> output stays exempt|
> Verified:
> * the five gap 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
> * 27 further scenarios probed during development ({{@Singleton}},
> {{@Immutable}}, {{@TupleConstructor}}, {{@Canonical}}, {{@Lazy}},
> {{@Delegate}}, traits, enums, records, inner classes) under both a
> receiver-restriction config and an allow-list config showed results
> byte-identical to baseline
> * full test suite: 16551 tests, no failures
> h2. Compatibility
> This is a behavioural change: scripts which compile today under a
> {{SecureASTCustomizer}} will be rejected if their constructors or
> initializers 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@ decision 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. Higher false-positive risk, and it
> does not help the {{@ASTTest}} case that prompted this investigation: that
> transformation moves its closure out of the AST into node metadata and
> reconstructs it from raw source text, so no AST-level filter can see it at
> any phase.
> * *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.
> h2. Note on scope
> {{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)
