Implement endpoints with potential tainted code execution#5
Implement endpoints with potential tainted code execution#5kyle-semgrep wants to merge 1 commit intomainfrom
Conversation
Added multiple endpoints demonstrating potential security vulnerabilities related to expression evaluation in Spring.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 18. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public String bad1(@RequestParam String input) { | ||
| ExpressionParser expressionParser = new SpelExpressionParser(); | ||
| // ruleid: spring-tainted-code-execution | ||
| Expression expression = expressionParser.parseExpression(input).getValue(); | ||
| String result = (String) expression.getValue(); |
There was a problem hiding this comment.
Method bad1 declares a String return type but falls through without returning anything, and the same pattern repeats in bad3 and ok1, so the class will not compile (missing return statement) and none of these endpoints can be exposed.
Useful? React with 👍 / 👎.
| @GetMapping("/api/bad1") | ||
| @ResponseBody | ||
| public String bad2(@RequestParam String input) { |
There was a problem hiding this comment.
The second handler is also annotated with @GetMapping("/api/bad1") even though bad1 already uses that path above, which will cause Spring to reject the application context with an ambiguous mapping error before it starts.
Useful? React with 👍 / 👎.
| Expression expression = expressionParser.parseExpression(input).getValue(); | ||
| String result = (String) expression.getValue(); |
There was a problem hiding this comment.
In bad1, Expression expression = expressionParser.parseExpression(input).getValue(); calls getValue() immediately and stores its result (an Object) into an Expression, which is a type mismatch that prevents compilation; the expression needs to be parsed first and evaluated separately.
Useful? React with 👍 / 👎.
| public String bad2(@RequestParam String input) { | ||
| if (expression == null) { | ||
| return null; | ||
| } | ||
| FacesContext context = getFacesContext(); |
There was a problem hiding this comment.
The null check and final return in bad2 reference expression, but the method never declares or receives that variable, so it will not compile and the endpoint cannot be registered.
Useful? React with 👍 / 👎.
| @SuppressWarnings("unchecked") // Forced cast on T | ||
| @Override | ||
| public <T> T evaluateExpressionGet(FacesContext context, String expression, Class<? extends T> expectedType) | ||
| throws ELException { |
There was a problem hiding this comment.
The evaluateExpressionGet method is annotated with @Override even though Test does not extend any type defining that signature and helper methods like getExpressionFactory() are undefined, so the file will not compile until the class implements the appropriate interface or the annotation and missing methods are fixed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ExpressionParser expressionParser = new SpelExpressionParser(); | ||
| // ruleid: spring-tainted-code-execution | ||
| Expression expression = expressionParser.parseExpression(input).getValue(); | ||
| String result = (String) expression.getValue(); |
There was a problem hiding this comment.
In bad1 the variable expression is initialized with expressionParser.parseExpression(input).getValue(), but getValue() returns an Object, not a Expression, so this statement will not compile and the method cannot be built or run. Consider capturing the Expression from parseExpression and evaluating it separately.
Useful? React with 👍 / 👎.
| String result = (String) expression.getValue(); | ||
| System.out.println(result); | ||
| } |
There was a problem hiding this comment.
bad1 is declared to return String but falls through to the closing brace without a return value on the success path, which causes a compilation error and prevents the controller from building.
Useful? React with 👍 / 👎.
| if (expression == null) { | ||
| return null; |
There was a problem hiding this comment.
bad2 checks and later dereferences expression, but that variable is never declared or assigned in the method, so the class will not compile and the endpoint cannot start.
Useful? React with 👍 / 👎.
| public String bad3(Model model, @RequestParam String input) { | ||
| ScriptEngineManager mgr = new ScriptEngineManager(); | ||
| ScriptEngine engine = mgr.getEngineByName("JavaScript"); | ||
| String foo = "40+"+input; | ||
| // ruleid: spring-tainted-code-execution | ||
| System.out.println(engine.eval(foo)); | ||
| } |
There was a problem hiding this comment.
bad3 is declared to return String yet exits after printing the evaluated script without any return statement, producing a compilation error that blocks the application from building.
Useful? React with 👍 / 👎.
| public String ok1(Model model, @RequestParam String input) { | ||
| ScriptEngineManager mgr = new ScriptEngineManager(); | ||
| ScriptEngine engine = mgr.getEngineByName("JavaScript"); | ||
| String foo = "40+3"; | ||
| // ok: spring-tainted-code-execution | ||
| System.out.println(engine.eval(foo)); |
There was a problem hiding this comment.
ok1 also declares a String return type but falls through without returning, so the compiler will reject this controller and the route cannot be used.
Useful? React with 👍 / 👎.
| @GetMapping("/api/bad1") | ||
| @ResponseBody | ||
| public String bad1(@RequestParam String input) { |
There was a problem hiding this comment.
Two methods (bad1 and bad2) are both annotated with @GetMapping("/api/bad1"), which will cause Spring to fail startup with an ambiguous mapping error once compilation issues are fixed.
Useful? React with 👍 / 👎.
![semgrep-code-blocking-test[bot]](https://avatars.githubusercontent.com/u/208699581?s=60&v=4){kind=small}
| ScriptEngine engine = mgr.getEngineByName("JavaScript"); | ||
| String foo = "40+"+input; | ||
| // ruleid: spring-tainted-code-execution | ||
| System.out.println(engine.eval(foo)); |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
User data flows into a script engine or another means of dynamic code evaluation. This is unsafe and could lead to code injection or arbitrary code execution as a result. Avoid inputting user data into code execution or use proper sandboxing if user code evaluation is necessary.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>vuln15.java</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L42 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 42] input</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L42 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 42] input</a>"]
v3["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L45 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 45] foo</a>"]
end
v2 --> v3
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L47 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 47] foo</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
✨ Commit Assistant fix suggestion
| System.out.println(engine.eval(foo)); | |
| // Sanitize input: Only allow numeric input to prevent code injection | |
| if (!input.matches("\\d+")) { | |
| throw new IllegalArgumentException("Invalid input: Only numeric values are allowed."); | |
| } | |
| int safeInput = Integer.parseInt(input); | |
| int result = 40 + safeInput; | |
| System.out.println(result); |
View step-by-step instructions
- Do not pass the user input directly into a script engine or dynamic code execution method.
- Validate or sanitize the
inputvalue to ensure it contains only safe data (for example, only allow numbers if that's expected):if (!input.matches("\\d+")) { throw new IllegalArgumentException("Invalid input"); }. - Replace the line
String foo = "40+"+input;with logic that only uses safe, validated data. For example:int safeInput = Integer.parseInt(input); String foo = "40+" + safeInput;. - Consider avoiding
engine.eval(foo)completely. Instead, if you only want to calculate40 + input, use standard Java math:int result = 40 + safeInput;. - Only use a scripting engine if absolutely required, and never with user-supplied code. If sandboxing is needed, use a library that restricts the code execution capabilities of the engine.
Allowing user input to be executed as code is highly dangerous and can allow attackers to run arbitrary commands on the server. Use only strict, whitelisted data and avoid dynamic code execution wherever possible.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by spring-tainted-code-execution.
You can view more details about this finding in the Semgrep AppSec Platform.
| expressionString = expressionString.substring(expressionString.indexOf("[") + 1, expressionString.indexOf("]")); | ||
| expressionString = "#{" + expressionString + "}"; | ||
| // ruleid: spring-tainted-code-execution | ||
| ValueExpression dynaVE = factory.createValueExpression(elContext, expressionString, String.class); |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
User data flows into a script engine or another means of dynamic code evaluation. This is unsafe and could lead to code injection or arbitrary code execution as a result. Avoid inputting user data into code execution or use proper sandboxing if user code evaluation is necessary.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>vuln15.java</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L18 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 18] input</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L18 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 18] input</a>"]
v3["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L24 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 24] expressionString</a>"]
end
v2 --> v3
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L34 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 34] expressionString</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
✨ Commit Assistant fix suggestion
| ValueExpression dynaVE = factory.createValueExpression(elContext, expressionString, String.class); | |
| // Validate and restrict user input before using in expressionString to prevent code injection | |
| // Only allow simple variable/property names (alphanumeric and underscores), or throw exception if invalid | |
| if (!expressionString.matches("#\\{[a-zA-Z0-9_\\.]+\\}")) { | |
| throw new IllegalArgumentException("Invalid expression syntax."); | |
| } | |
| ValueExpression dynaVE = factory.createValueExpression(elContext, expressionString, String.class); |
View step-by-step instructions
- Avoid passing user-controlled input directly to the expression evaluation engine (
factory.createValueExpression) to prevent code injection. - If user input is necessary for the expression, strictly validate and whitelist acceptable patterns of input before usage. For example, only allow simple variable names or specific safe values:
if (!input.matches("[a-zA-Z0-9_]+")) { throw new IllegalArgumentException("Invalid input"); }. - Replace all usage of
factory.createValueExpression(elContext, expressionString, String.class)whereexpressionStringcontains unsanitized user input, with a safer alternative that does not allow dynamic code execution, or parse the input and safely map it to predefined expressions. - Alternatively, if dynamic expression evaluation is absolutely required, use a secure sandbox or expression language configuration that restricts the set of accessible methods, classes, and properties available to the user input.
Expression injection enables attackers to execute arbitrary code, so any user input included in expressions must be validated and restricted to known-safe values.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by spring-tainted-code-execution.
You can view more details about this finding in the Semgrep AppSec Platform.
| public String bad1(@RequestParam String input) { | ||
| ExpressionParser expressionParser = new SpelExpressionParser(); | ||
| // ruleid: spring-tainted-code-execution | ||
| Expression expression = expressionParser.parseExpression(input).getValue(); |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
User data flows into a script engine or another means of dynamic code evaluation. This is unsafe and could lead to code injection or arbitrary code execution as a result. Avoid inputting user data into code execution or use proper sandboxing if user code evaluation is necessary.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>vuln15.java</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L9 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 9] input</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L9 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 9] input</a>"]
end
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/kyle-semgrep/bad-python-app-kyle-managed/blob/795966a770f03a0790876a78bd54c4674598810b/vuln15.java#L12 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 12] input</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
✨ Commit Assistant fix suggestion
| Expression expression = expressionParser.parseExpression(input).getValue(); | |
| // Validate or whitelist user input before parsing it as an expression to prevent code injection | |
| // Here, allow only simple alphanumeric identifiers (letters, numbers, and underscore); adjust as needed for your application context | |
| if (!input.matches("[a-zA-Z0-9_]+")) { | |
| throw new IllegalArgumentException("Invalid input"); | |
| } | |
| Expression expression = expressionParser.parseExpression(input); |
View step-by-step instructions
- Do not pass user-supplied input directly to
expressionParser.parseExpression(input)as this allows users to execute arbitrary code. - If dynamic expression evaluation is not strictly necessary, remove or replace the use of
parseExpressionon untrusted input.
For example, validate or whitelist expected values ininput, such as allowing only certain expression strings. - If you must evaluate user input, implement strict validation using a whitelist, such as allowing only letters, numbers, or specific permitted expressions:
if (!input.matches("[a-zA-Z0-9_]+")) throw new IllegalArgumentException("Invalid input"); - Alternatively, if your use case can be satisfied with a fixed set of operations, map input values to predefined expressions or functions rather than using
parseExpressiondirectly:
switch (input) { case "add": ... break; default: throw ... } - Never echo or log unsanitized user input or results of dynamic expression evaluation.
Passing untrusted user input to expression or script evaluators such as SpEL is dangerous because these APIs are designed to execute arbitrary code and can lead to severe security vulnerabilities.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by spring-tainted-code-execution.
You can view more details about this finding in the Semgrep AppSec Platform.
1 participant
Added multiple endpoints demonstrating potential security vulnerabilities related to expression evaluation in Spring.