[
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}}.
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 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 misfires, the failure mode is a false rejection of a valid program,
not a silent hole.
* 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
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.
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@ 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.
> 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
> 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 misfires, the failure mode is a false rejection of a valid program,
> not a silent hole.
> * 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
> 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)
