Paul King created GROOVY-12283:
----------------------------------
Summary: 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
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)
