Copilot commented on code in PR #8104:
URL: https://github.com/apache/incubator-seata/pull/8104#discussion_r3254920587
##########
saga/seata-saga-engine/src/main/java/org/apache/seata/saga/engine/pcext/handlers/ScriptTaskStateHandler.java:
##########
@@ -49,6 +53,18 @@ public class ScriptTaskStateHandler implements StateHandler,
InterceptableStateH
private static final Logger LOGGER =
LoggerFactory.getLogger(ScriptTaskStateHandler.class);
+ private static final Set<String> ALLOWED_SCRIPT_TYPES = new
HashSet<>(Arrays.asList("groovy", "js", "javascript"));
+
+ private static final Pattern DANGEROUS_PATTERN = Pattern.compile("(?i)"
+ + "Runtime|ProcessBuilder|\\.execute\\s*\\("
+ + "|System\\s*\\.\\s*(exit|getRuntime|setSecurityManager)"
+ + "|Class\\s*\\.\\s*forName|ClassLoader"
+ + "|java\\.lang\\.reflect"
+ + "|Thread\\s*\\.|\\.getClass\\s*\\("
+ + "|java\\.io\\.File|java\\.net\\."
+ + "|javax\\.script\\.ScriptEngine"
+ + "|GroovyShell|GroovyClassLoader");
Review Comment:
This blacklist does not actually prevent dangerous Groovy scripts; Groovy
default-imports java.io.*, so a script such as new File('/tmp/secret').text
does file I/O without matching java.io.File. If scripts are untrusted, enforce
sandboxing with the script engine/compiler configuration or an allowlisted API
surface instead of scanning for a few substrings.
##########
saga/seata-saga-spring/src/main/java/org/apache/seata/saga/engine/expression/spel/SpringELExpression.java:
##########
@@ -25,18 +27,34 @@
public class SpringELExpression implements ELExpression {
private org.springframework.expression.Expression expression;
+ private EvaluationContext evaluationContext;
public SpringELExpression(org.springframework.expression.Expression
expression) {
this.expression = expression;
}
+ public SpringELExpression(
+ org.springframework.expression.Expression expression,
EvaluationContext evaluationContext) {
+ this.expression = expression;
+ this.evaluationContext = evaluationContext;
+ }
+
@Override
public Object getValue(Object elContext) {
+ if (evaluationContext instanceof StandardEvaluationContext) {
+ ((StandardEvaluationContext)
evaluationContext).setRootObject(elContext);
+ return expression.getValue(evaluationContext);
Review Comment:
This mutates a single EvaluationContext stored on the SpringELExpression
before each evaluation. These expressions are cached on state definitions (for
example choice evaluators) and can be evaluated concurrently by different state
machine instances, so one thread can replace the root object while another is
still evaluating and produce results against the wrong context. Create a fresh
evaluation context per getValue/setValue call or otherwise avoid sharing
mutable root state across evaluations.
##########
saga/seata-saga-spring/src/main/java/org/apache/seata/saga/engine/expression/spel/SpringELExpressionFactory.java:
##########
@@ -43,9 +46,22 @@ public SpringELExpressionFactory(ApplicationContext
applicationContext) {
@Override
public Expression createExpression(String expression) {
org.springframework.expression.Expression defaultExpression =
parser.parseExpression(expression);
- EvaluationContext evaluationContext = ((SpelExpression)
defaultExpression).getEvaluationContext();
- ((StandardEvaluationContext) evaluationContext).setBeanResolver(new
AppContextBeanResolver());
- return new SpringELExpression(defaultExpression);
+ StandardEvaluationContext evaluationContext =
createRestrictedEvaluationContext();
+ return new SpringELExpression(defaultExpression, evaluationContext);
+ }
+
+ private StandardEvaluationContext createRestrictedEvaluationContext() {
+ StandardEvaluationContext context = new StandardEvaluationContext();
+ context.setTypeLocator(new DenyAllTypeLocator());
+ context.setBeanResolver(new AppContextBeanResolver());
Review Comment:
Blocking only the TypeLocator still leaves the restricted context with
StandardEvaluationContext's default method resolvers, so an expression can
reach java.lang.Class through the root object (for example via
getClass()/forName()) and then invoke dangerous APIs without using T(...). Use
a SimpleEvaluationContext or remove/replace method/constructor resolvers so
reflection and arbitrary method invocation are not available if this is
intended to sandbox Saga SpringEL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
