[
https://issues.apache.org/jira/browse/GROOVY-12238?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12238:
-------------------------------
Description:
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.
was:
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@ 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. 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.
> Cover additional SecureASTCustomizer cases
> ------------------------------------------
>
> 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)
