[
https://issues.apache.org/jira/browse/GROOVY-12416?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12416:
-------------------------------
Description:
With {{setIndirectImportCheckEnabled(true)}}, {{SecureASTCustomizer}} checks
the receiver type of a method call or method pointer against the import rules.
When that receiver has a primitive type, the check uses the primitive type name
({{int}}, {{long}}, {{boolean}}, {{double}}, ...). Such a name can never match
a class in an allow list, so the call is rejected even when the corresponding
wrapper type is allowed:
{code:java}
SecureASTCustomizer secure = new SecureASTCustomizer();
secure.setAllowedImports(List.of("java.lang.Object", "java.lang.Integer",
"java.lang.Long",
"java.lang.Boolean", "java.lang.Double"));
secure.setAllowedStarImports(List.of());
secure.setIndirectImportCheckEnabled(true);
secure.setClosuresAllowed(true);
CompilerConfiguration config = new CompilerConfiguration();
config.addCompilationCustomizers(secure);
new GroovyShell(config).parse("3.times { }");
// SecurityException: Indirect import checks prevent usage of expression:
Importing [int] is not allowed
{code}
Results on master (7.0.0-SNAPSHOT), with the wrapper types allowed:
||Expression||Result||
|{{3.times { }{}}}|rejected: {{Importing [int] is not allowed}}|
|{{3L.times { }}}|rejected: {{Importing [long] is not allowed}}|
|{{true.and(false)}}|rejected: {{Importing [boolean] is not allowed}}|
|{{1.5d.abs()}}|rejected: {{Importing [double] is not allowed}}|
|{{int[] a = [1]; a.size()}}|rejected: {{Importing [int] is not allowed}}
(array receivers are reduced to their component type)|
|{{3.×}}|rejected: {{Importing [int] is not allowed}}|
|{{int n = 3; n.times { }}}|compiles (checked as {{java.lang.Integer}})|
|{{Integer n = 3; n.times { }}}|compiles|
The same value is therefore judged under a different name depending on whether
it is written as a literal or held in a typed local variable. The behaviour is
long-standing: 2.5.6, 2.5.14, 4.0.28 and 5.0.4 behave the same way.
The {{MethodCallExpression}} and {{MethodPointerExpression}} branches of
{{SecuringCodeVisitor}} pass the receiver type name straight to
{{assertImportIsAllowed}}. The construction-coercion cast branch added in
GROOVY-12283 already guards against primitives with
{{ClassHelper.isPrimitiveType}}.
*Proposed behaviour:* check a primitive receiver as its wrapper type
({{ClassHelper.getWrapper}}), so that {{3.times { }}} is checked as
{{java.lang.Integer}}, consistent with a typed local. Simply skipping
primitives would be looser: a policy that does not allow {{java.lang.Integer}}
would then still accept calls on integer literals.
was:
With {{setIndirectImportCheckEnabled(true)}}, {{SecureASTCustomizer}} checks
the receiver type of a method call or method pointer against the import rules.
When that receiver has a primitive type, the check uses the primitive type name
({{int}}, {{long}}, {{boolean}}, {{double}}, ...). Such a name can never match
a class in an allow list, so the call is rejected even when the corresponding
wrapper type is allowed:
{code:java}
SecureASTCustomizer secure = new SecureASTCustomizer();
secure.setAllowedImports(List.of("java.lang.Object", "java.lang.Integer",
"java.lang.Long",
"java.lang.Boolean", "java.lang.Double"));
secure.setAllowedStarImports(List.of());
secure.setIndirectImportCheckEnabled(true);
secure.setClosuresAllowed(true);
CompilerConfiguration config = new CompilerConfiguration();
config.addCompilationCustomizers(secure);
new GroovyShell(config).parse("3.times { }");
// SecurityException: Indirect import checks prevent usage of expression:
Importing [int] is not allowed
{code}
Results on master (7.0.0-SNAPSHOT), with the wrapper types allowed:
||Expression||Result||
|{{3.times \{ \}}}|rejected: {{Importing [int] is not allowed}}|
|{{3L.times { }}}|rejected: {{Importing [long] is not allowed}}|
|{{true.and(false)}}|rejected: {{Importing [boolean] is not allowed}}|
|{{1.5d.abs()}}|rejected: {{Importing [double] is not allowed}}|
|{{int[] a = [1]; a.size()}}|rejected: {{Importing [int] is not allowed}}
(array receivers are reduced to their component type)|
|{{3.×}}|rejected: {{Importing [int] is not allowed}}|
|{{int n = 3; n.times { }}}|compiles (checked as {{java.lang.Integer}})|
|{{Integer n = 3; n.times { }}}|compiles|
The same value is therefore judged under a different name depending on whether
it is written as a literal or held in a typed local variable. The behaviour is
long-standing: 2.5.6, 2.5.14, 4.0.28 and 5.0.4 behave the same way.
The {{MethodCallExpression}} and {{MethodPointerExpression}} branches of
{{SecuringCodeVisitor}} pass the receiver type name straight to
{{assertImportIsAllowed}}. The construction-coercion cast branch added in
GROOVY-12283 already guards against primitives with
{{ClassHelper.isPrimitiveType}}.
*Proposed behaviour:* check a primitive receiver as its wrapper type
({{ClassHelper.getWrapper}}), so that {{3.times { }}} is checked as
{{java.lang.Integer}}, consistent with a typed local. Simply skipping
primitives would be looser: a policy that does not allow {{java.lang.Integer}}
would then still accept calls on integer literals.
> SecureASTCustomizer: indirect import check does not box primitive receiver
> types
> --------------------------------------------------------------------------------
>
> Key: GROOVY-12416
> URL: https://issues.apache.org/jira/browse/GROOVY-12416
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> With {{setIndirectImportCheckEnabled(true)}}, {{SecureASTCustomizer}} checks
> the receiver type of a method call or method pointer against the import
> rules. When that receiver has a primitive type, the check uses the primitive
> type name ({{int}}, {{long}}, {{boolean}}, {{double}}, ...). Such a name can
> never match a class in an allow list, so the call is rejected even when the
> corresponding wrapper type is allowed:
> {code:java}
> SecureASTCustomizer secure = new SecureASTCustomizer();
> secure.setAllowedImports(List.of("java.lang.Object", "java.lang.Integer",
> "java.lang.Long",
> "java.lang.Boolean", "java.lang.Double"));
> secure.setAllowedStarImports(List.of());
> secure.setIndirectImportCheckEnabled(true);
> secure.setClosuresAllowed(true);
> CompilerConfiguration config = new CompilerConfiguration();
> config.addCompilationCustomizers(secure);
> new GroovyShell(config).parse("3.times { }");
> // SecurityException: Indirect import checks prevent usage of expression:
> Importing [int] is not allowed
> {code}
> Results on master (7.0.0-SNAPSHOT), with the wrapper types allowed:
> ||Expression||Result||
> |{{3.times { }{}}}|rejected: {{Importing [int] is not allowed}}|
> |{{3L.times { }}}|rejected: {{Importing [long] is not allowed}}|
> |{{true.and(false)}}|rejected: {{Importing [boolean] is not allowed}}|
> |{{1.5d.abs()}}|rejected: {{Importing [double] is not allowed}}|
> |{{int[] a = [1]; a.size()}}|rejected: {{Importing [int] is not allowed}}
> (array receivers are reduced to their component type)|
> |{{3.×}}|rejected: {{Importing [int] is not allowed}}|
> |{{int n = 3; n.times { }}}|compiles (checked as {{java.lang.Integer}})|
> |{{Integer n = 3; n.times { }}}|compiles|
> The same value is therefore judged under a different name depending on
> whether it is written as a literal or held in a typed local variable. The
> behaviour is long-standing: 2.5.6, 2.5.14, 4.0.28 and 5.0.4 behave the same
> way.
> The {{MethodCallExpression}} and {{MethodPointerExpression}} branches of
> {{SecuringCodeVisitor}} pass the receiver type name straight to
> {{assertImportIsAllowed}}. The construction-coercion cast branch added in
> GROOVY-12283 already guards against primitives with
> {{ClassHelper.isPrimitiveType}}.
> *Proposed behaviour:* check a primitive receiver as its wrapper type
> ({{ClassHelper.getWrapper}}), so that {{3.times { }}} is checked as
> {{java.lang.Integer}}, consistent with a typed local. Simply skipping
> primitives would be looser: a policy that does not allow
> {{java.lang.Integer}} would then still accept calls on integer literals.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
