[
https://issues.apache.org/jira/browse/GROOVY-12283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105825#comment-18105825
]
ASF GitHub Bot commented on GROOVY-12283:
-----------------------------------------
paulk-asert opened a new pull request, #2819:
URL: https://github.com/apache/groovy/pull/2819
…ubscripts
The indirect import check inspected constructor, method, static-method and
method-pointer expressions, so a class forbidden by the import rules could
still be built through a construction that is neither a constructor call nor a
method call: a cast whose operand is a list, map or closure literal ((Foo)
[..], [..] as Foo, (Runnable) { }), and a named-argument subscript (Foo[a: 1]).
Each builds an instance of the named type.
The check is extended to both. A cast constructs when its operand is a list,
map or closure literal, as opposed to converting a value that already exists;
its target type is checked like a constructor call (array component unwrapped,
primitive components skipped as they name no class). A subscript constructs
when its arguments are map entries, which are not valid in an ordinary
subscript, so their presence marks the form unambiguously; the receiver type is
dynamic at this phase, so the class is named by its source text.
Plain converting casts ((String) x, (int) n) and positional subscripts stay
unexamined. The residual is the non-literal coercion ((Foo) var, var as Foo),
where an overridden asType could construct at runtime; that is statically
invisible and out of scope, consistent with this customizer being a hardening
aid rather than a security boundary.
> 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)
