[
https://issues.apache.org/jira/browse/GROOVY-12283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105859#comment-18105859
]
ASF GitHub Bot commented on GROOVY-12283:
-----------------------------------------
Copilot commented on code in PR #2819:
URL: https://github.com/apache/groovy/pull/2819#discussion_r3811707528
##########
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java:
##########
@@ -1557,6 +1576,44 @@ protected ClassNode getExpressionType(ClassNode
objectExpressionType) {
return objectExpressionType.isArray() ?
getExpressionType(objectExpressionType.getComponentType()) :
objectExpressionType;
}
+ /**
+ * Whether a cast constructs an instance of its type by coercing a
literal operand — a list
+ * or map (invoking a constructor) or a closure (creating a SAM proxy)
— as opposed to
+ * converting a value that already exists. Such a cast is treated like
a constructor call by
+ * the indirect import check (GROOVY-12283).
+ *
+ * @param cast the cast expression
+ * @return {@code true} if the cast materialises a new instance of its
type
+ */
+ private static boolean constructsByCoercion(final CastExpression cast)
{
+ Expression operand = cast.getExpression();
+ return operand instanceof ListExpression
+ || operand instanceof MapExpression
+ || operand instanceof ClosureExpression;
+ }
+
+ /**
+ * Whether a subscript is a named-argument construction such as
+ * {@code Foo[name: 'x', size: 2]} rather than an ordinary index
access. Map entries are not
+ * valid in a real subscript, so their presence uniquely marks the
construction form
+ * (GROOVY-12283).
+ *
+ * @param expression the binary expression
+ * @return {@code true} if the expression constructs by named arguments
+ */
+ private static boolean isNamedArgConstruction(final BinaryExpression
expression) {
+ if (!"[".equals(expression.getOperation().getText())) {
+ return false;
+ }
+ Expression right = expression.getRightExpression();
+ if (right instanceof SpreadMapExpression) {
+ return true;
+ }
+ return right instanceof ListExpression
+ && ((ListExpression) right).getExpressions().stream()
+ .anyMatch(e -> e instanceof MapEntryExpression ||
e instanceof SpreadMapExpression);
Review Comment:
This check runs during AST traversal and may be executed very frequently.
Using a stream here allocates extra objects and can add overhead; a simple
indexed/foreach loop over `getExpressions()` can perform the same check with
less allocation and typically better performance in compiler-phase code.
##########
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java:
##########
@@ -1540,6 +1545,20 @@ protected void assertExpressionAuthorized(final
Expression expression) throws Se
final String typename = expr.getType().getName();
assertImportIsAllowed(typename);
assertStaticImportIsAllowed(expr.getText(), typename);
+ } else if (expression instanceof CastExpression expr &&
constructsByCoercion(expr)) {
+ // GROOVY-12283: a cast whose operand is a list, map
or closure literal
+ // constructs an instance of the cast type (list/map
-> constructor,
+ // closure -> SAM proxy) rather than converting an
existing value, so it
+ // is checked like a constructor call. Covers `(Foo)
[..]` and `[..] as Foo`.
+ ClassNode target = getExpressionType(expr.getType());
// array -> component
+ if (!ClassHelper.isPrimitiveType(target)) { // e.g.
(int[]) [1, 2] has no class to check
+ assertImportIsAllowed(target.getName());
+ }
+ } else if (expression instanceof BinaryExpression expr &&
isNamedArgConstruction(expr)) {
+ // GROOVY-12283: `Foo[name: 'x', ..]` is a
named-argument construction of
+ // Foo, not a subscript (map entries are not valid in
a real subscript). The
+ // receiver type is dynamic here, so the class is
named by its source text.
+
assertImportIsAllowed(expr.getLeftExpression().getText());
}
} catch (SecurityException e) {
throw new SecurityException("Indirect import checks
prevents usage of expression", e);
Review Comment:
The exception message has a grammatical error (“checks prevents”) and is
very generic. Since this code path will now trigger for additional expression
kinds (casts/subscripts), it would be more actionable to fix the wording (e.g.,
“checks prevent”) and include some minimal context (such as the offending
expression text or its class) to help users identify what was blocked.
##########
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java:
##########
@@ -1557,6 +1576,44 @@ protected ClassNode getExpressionType(ClassNode
objectExpressionType) {
return objectExpressionType.isArray() ?
getExpressionType(objectExpressionType.getComponentType()) :
objectExpressionType;
}
+ /**
+ * Whether a cast constructs an instance of its type by coercing a
literal operand — a list
+ * or map (invoking a constructor) or a closure (creating a SAM proxy)
— as opposed to
+ * converting a value that already exists. Such a cast is treated like
a constructor call by
+ * the indirect import check (GROOVY-12283).
+ *
+ * @param cast the cast expression
+ * @return {@code true} if the cast materialises a new instance of its
type
+ */
+ private static boolean constructsByCoercion(final CastExpression cast)
{
+ Expression operand = cast.getExpression();
+ return operand instanceof ListExpression
+ || operand instanceof MapExpression
+ || operand instanceof ClosureExpression;
+ }
+
+ /**
+ * Whether a subscript is a named-argument construction such as
+ * {@code Foo[name: 'x', size: 2]} rather than an ordinary index
access. Map entries are not
+ * valid in a real subscript, so their presence uniquely marks the
construction form
+ * (GROOVY-12283).
+ *
+ * @param expression the binary expression
+ * @return {@code true} if the expression constructs by named arguments
+ */
+ private static boolean isNamedArgConstruction(final BinaryExpression
expression) {
+ if (!"[".equals(expression.getOperation().getText())) {
+ return false;
+ }
+ Expression right = expression.getRightExpression();
+ if (right instanceof SpreadMapExpression) {
+ return true;
+ }
Review Comment:
`SpreadMapExpression` is now explicitly treated as named-argument
construction, but the added tests only cover map-entry style (`Foo[a: 1]`) and
not the spread-map forms (e.g., `Foo[*: someMap]` or mixtures like `Foo[a: 1,
*: someMap]`). Adding a regression test for the spread-map variant would ensure
this branch stays correct and prevents future regressions.
> SecureASTCustomizer: apply import rules to construction-coercion casts and
> subscripts
> -------------------------------------------------------------------------------------
>
> Key: GROOVY-12283
> URL: https://issues.apache.org/jira/browse/GROOVY-12283
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> h4. Summary
> When {{indirectImportCheckEnabled}} is on, {{SecureASTCustomizer}} checks the
> type of {{new Foo(...)}} against the import rules but not the type of a
> *construction by coercion*, so an instance of an import-forbidden class can
> still be built:
> * cast coercion: {{(Foo) [a, b]}}, {{(Foo) [x: 1, y: 2]}}, {{(Foo) { .. }}},
> and the {{as}} form {{[a, b] as Foo}} — a {{CastExpression}} whose operand is
> a list, map or closure literal
> * named-arg subscript: {{Foo[x: 1, y: 2]}} — a {{BinaryExpression}}
> (subscript)
> Sibling to GROOVY-12279, which fixes the method-pointer arm of the same
> indirect-import block; this closes the construction-coercion arms so the
> whitelist behaves the same across equivalent construction syntaxes.
> h4. Framing (read first)
> This is an *improvement to a hardening aid, not a security fix*.
> {{SecureASTCustomizer}}'s own javadoc calls it "a hardening aid rather than a
> security boundary" and states "a report that merely demonstrates a bypass is
> by design, not a vulnerability." Not a disclosure, not a CVE. It is unrelated
> to GROOVY-10355 — these coercion forms are long-standing and independent of
> that parser change.
> h4. Reproduction (verified, 6.0-SNAPSHOT)
> Sandbox whitelisting only {{java.lang.String}}, {{indirectImportCheckEnabled
> = true}}:
> * {{new java.io.File('/etc/passwd')}} → blocked at compile time.
> * {{(java.io.File) ['/etc/passwd']}} → *allowed*; constructs a {{File}} for
> {{/etc/passwd}}.
> * {{['/etc/passwd'] as java.io.File}} → *allowed* (same node as the cast
> form).
> * {{Foo[a: '1', b: '2']}} (class with a Map constructor) → *allowed*.
> h4. Cause
> The indirect-import block in
> {{SecuringCodeVisitor.assertExpressionAuthorized}} inspects
> {{ConstructorCallExpression}}, {{MethodCallExpression}},
> {{StaticMethodCallExpression}} and {{MethodPointerExpression}} (the last
> fixed by GROOVY-12279). A {{CastExpression}} and a subscript
> {{BinaryExpression}} are not among them, so the target type name never
> reaches {{assertImportIsAllowed}}. {{visitCastExpression}} does call
> {{assertExpressionAuthorized}}, but that only tests whether
> {{CastExpression}} as a node class is allow/deny-listed — never the cast's
> target type.
> h4. Why not simply "check all cast types"
> Cast types are excluded on purpose — the javadoc groups them with class
> literals, {{instanceof}}, property access and catch types: places where a
> type name appears but nothing executes on it. That is correct for {{(String)
> obj}}, {{(int) n}}, {{(Foo) bar()}} — checked conversions of a value that
> already exists. Checking every cast type would reverse a sound decision and
> over-block ordinary downcasts.
> The threat is the sub-case where "nothing executes" is false. A cast whose
> operand is a *list, map or closure literal* materialises a new instance of
> the cast type (list/map → constructor, closure → SAM proxy); it is a
> construction, not a conversion. That sub-case is structurally identifiable by
> operand shape and is exactly the slice to check.
> h4. Proposed change
> Extend the indirect-import block, guarded by {{isIndirectImportCheckEnabled}}:
> * {{CastExpression}} whose operand is a {{ListExpression}}, {{MapExpression}}
> or {{ClosureExpression}} → {{assertImportIsAllowed}} on the cast target type
> (unwrapping array component types via the existing {{getExpressionType}}
> helper; primitive component types have no name to check and are skipped).
> Covers both the {{(Foo) [..]}} and {{[..] as Foo}} spellings, which share the
> node.
> * construction-coercion subscript {{BinaryExpression}} ({{Foo[x: 1, ..]}},
> i.e. {{[}} operator with a map-entry / list right side) → check the receiver
> (left) type the same way.
> All other casts stay unexamined, so inert conversions are unaffected. Update
> the "cast ... types are not examined" javadoc line to record the
> literal-operand exception.
> h4. Boundary (state in the fix)
> The residual is the non-literal coercion — {{(Foo) someVar}} / {{someVar as
> Foo}} — where an overridden {{asType}} could construct at runtime. That is
> statically invisible and stays uncovered, consistent with the hardening-aid
> posture. The slice catches every statically obvious construction, a strict
> improvement over the current all-or-nothing exclusion.
> h4. Tests
> Add cases to {{SecureASTCustomizerTest}} for both coercion forms (cast
> list/map/closure and {{as}}; subscript), allowed and denied, with the check
> on and off; plus a negative test that an inert cast ({{(String) x}}) to a
> non-whitelisted type is still permitted, so the slice boundary is pinned.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
