-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Java: Promote Server-side template injection from experimental #10352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
atorralba
merged 14 commits into
github:main
from
atorralba:atorralba/promote-template-injection
Sep 20, 2022
Merged
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
cd61bd0
Move files from experimental
atorralba c972809
Generate stubs, adapt tests
atorralba b68e666
Refactor TemplateInjection libraries
atorralba e311155
Use InlineExpectationsTest
atorralba fb13e7f
Docs changes
atorralba 6413de6
Add change note
atorralba d748fb5
Fix bad models, add tests for those
atorralba 409a123
Tainting the velocity context isn't exploitable
atorralba dd6257c
Add security-severity
atorralba 79a32f1
Tainting the freemarker dataModel isn't exploitable
atorralba f412f43
Add thymeleaf steps
atorralba 3141fda
Address review comments re: flow states
atorralba 4997f36
Apply suggestions from code review
atorralba 4af29e6
Update java/ql/src/Security/CWE/CWE-094/TemplateInjection.qhelp
atorralba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/** | ||
* Provides classes and predicates for working with the Thymeleaf template engine. | ||
*/ | ||
|
||
import java | ||
private import semmle.code.java.dataflow.ExternalFlow | ||
|
||
private class ThymeleafSummaryModels extends SummaryModelCsv { | ||
override predicate row(string row) { | ||
row = | ||
[ | ||
"org.thymeleaf;TemplateSpec;false;TemplateSpec;;;Argument[0];Argument[-1];taint;manual", | ||
"org.thymeleaf;TemplateSpec;false;getTemplate;;;Argument[-1];ReturnValue;taint;manual", | ||
] | ||
} | ||
} |
108 changes: 108 additions & 0 deletions
108
java/ql/lib/semmle/code/java/security/TemplateInjection.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
/** Definitions related to the server-side template injection (SST) query. */ | ||
|
||
import java | ||
private import semmle.code.java.dataflow.FlowSources | ||
private import semmle.code.java.dataflow.ExternalFlow | ||
private import semmle.code.java.dataflow.TaintTracking | ||
|
||
/** | ||
* A source for server-side template injection (SST) vulnerabilities. | ||
*/ | ||
abstract class TemplateInjectionSource extends DataFlow::Node { | ||
/** Holds if this source has the specified `state`. */ | ||
predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } | ||
} | ||
|
||
/** | ||
* A sink for server-side template injection (SST) vulnerabilities. | ||
*/ | ||
abstract class TemplateInjectionSink extends DataFlow::Node { | ||
/** Holds if this sink has the specified `state`. */ | ||
predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } | ||
} | ||
|
||
/** | ||
* A unit class for adding additional taint steps. | ||
* | ||
* Extend this class to add additional taint steps that should apply to flows related to | ||
* server-side template injection (SST) vulnerabilities. | ||
*/ | ||
class TemplateInjectionAdditionalTaintStep extends Unit { | ||
/** | ||
* Holds if the step from `node1` to `node2` should be considered a taint | ||
* step for flows related to server-side template injection (SST) vulnerabilities. | ||
*/ | ||
predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { none() } | ||
|
||
/** | ||
* Holds if the step from `node1` to `node2` should be considered a taint | ||
* step for flows related toserver-side template injection (SST) vulnerabilities. | ||
* This step is only applicable in `state1` and updates the flow state to `state2`. | ||
*/ | ||
predicate isAdditionalTaintStep( | ||
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, | ||
DataFlow::FlowState state2 | ||
) { | ||
none() | ||
} | ||
} | ||
|
||
/** | ||
* A sanitizer for server-side template injection (SST) vulnerabilities. | ||
*/ | ||
abstract class TemplateInjectionSanitizer extends DataFlow::Node { } | ||
|
||
/** | ||
* A sanitizer for server-side template injection (SST) vulnerabilities. | ||
* This sanitizer is only applicable when `TemplateInjectionSanitizerWithState::hasState` | ||
* holds for the flow state. | ||
*/ | ||
abstract class TemplateInjectionSanitizerWithState extends DataFlow::Node { | ||
/** Holds if this sanitizer has the specified `state`. */ | ||
abstract predicate hasState(DataFlow::FlowState state); | ||
} | ||
|
||
private class DefaultTemplateInjectionSource extends TemplateInjectionSource instanceof RemoteFlowSource { | ||
} | ||
|
||
private class DefaultTemplateInjectionSink extends TemplateInjectionSink { | ||
DefaultTemplateInjectionSink() { sinkNode(this, "ssti") } | ||
} | ||
|
||
private class DefaultTemplateInjectionSanitizer extends TemplateInjectionSanitizer { | ||
DefaultTemplateInjectionSanitizer() { | ||
this.getType() instanceof PrimitiveType or | ||
this.getType() instanceof BoxedType or | ||
this.getType() instanceof NumericType | ||
} | ||
} | ||
|
||
private class TemplateInjectionSinkModels extends SinkModelCsv { | ||
override predicate row(string row) { | ||
row = | ||
[ | ||
"freemarker.template;Template;true;Template;(String,Reader);;Argument[1];ssti;manual", | ||
"freemarker.template;Template;true;Template;(String,Reader,Configuration);;Argument[1];ssti;manual", | ||
"freemarker.template;Template;true;Template;(String,Reader,Configuration,String);;Argument[1];ssti;manual", | ||
"freemarker.template;Template;true;Template;(String,String,Reader,Configuration);;Argument[2];ssti;manual", | ||
"freemarker.template;Template;true;Template;(String,String,Reader,Configuration,String);;Argument[2];ssti;manual", | ||
"freemarker.template;Template;true;Template;(String,String,Reader,Configuration,ParserConfiguration,String);;Argument[2];ssti;manual", | ||
"freemarker.template;Template;true;Template;(String,String,Configuration);;Argument[1];ssti;manual", | ||
"freemarker.cache;StringTemplateLoader;true;putTemplate;;;Argument[1];ssti;manual", | ||
"com.mitchellbosecke.pebble;PebbleEngine;true;getTemplate;;;Argument[0];ssti;manual", | ||
"com.mitchellbosecke.pebble;PebbleEngine;true;getLiteralTemplate;;;Argument[0];ssti;manual", | ||
"com.hubspot.jinjava;Jinjava;true;renderForResult;;;Argument[0];ssti;manual", | ||
"com.hubspot.jinjava;Jinjava;true;render;;;Argument[0];ssti;manual", | ||
"org.thymeleaf;ITemplateEngine;true;process;;;Argument[0];ssti;manual", | ||
"org.thymeleaf;ITemplateEngine;true;processThrottled;;;Argument[0];ssti;manual", | ||
"org.apache.velocity.app;Velocity;true;evaluate;;;Argument[3];ssti;manual", | ||
"org.apache.velocity.app;Velocity;true;mergeTemplate;;;Argument[2];ssti;manual", | ||
"org.apache.velocity.app;VelocityEngine;true;evaluate;;;Argument[3];ssti;manual", | ||
"org.apache.velocity.app;VelocityEngine;true;mergeTemplate;;;Argument[2];ssti;manual", | ||
"org.apache.velocity.runtime.resource.util;StringResourceRepository;true;putStringResource;;;Argument[1];ssti;manual", | ||
"org.apache.velocity.runtime;RuntimeServices;true;evaluate;;;Argument[3];ssti;manual", | ||
"org.apache.velocity.runtime;RuntimeServices;true;parse;;;Argument[0];ssti;manual", | ||
"org.apache.velocity.runtime;RuntimeSingleton;true;parse;;;Argument[0];ssti;manual" | ||
] | ||
} | ||
} |
38 changes: 38 additions & 0 deletions
38
java/ql/lib/semmle/code/java/security/TemplateInjectionQuery.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/** Provides a taint tracking configuration for server-side template injection (SST) vulnerabilities */ | ||
|
||
import java | ||
import semmle.code.java.dataflow.TaintTracking | ||
import semmle.code.java.dataflow.FlowSources | ||
import semmle.code.java.security.TemplateInjection | ||
|
||
/** A taint tracking configuration to reason about server-side template injection (SST) vulnerabilities */ | ||
class TemplateInjectionFlowConfig extends TaintTracking::Configuration { | ||
TemplateInjectionFlowConfig() { this = "TemplateInjectionFlowConfig" } | ||
|
||
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { | ||
source.(TemplateInjectionSource).hasState(state) | ||
} | ||
|
||
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { | ||
sink.(TemplateInjectionSink).hasState(state) | ||
} | ||
|
||
override predicate isSanitizer(DataFlow::Node sanitizer) { | ||
sanitizer instanceof TemplateInjectionSanitizer | ||
} | ||
|
||
override predicate isSanitizer(DataFlow::Node sanitizer, DataFlow::FlowState state) { | ||
sanitizer.(TemplateInjectionSanitizerWithState).hasState(state) | ||
} | ||
|
||
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { | ||
any(TemplateInjectionAdditionalTaintStep a).isAdditionalTaintStep(node1, node2) | ||
} | ||
|
||
override predicate isAdditionalTaintStep( | ||
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, | ||
DataFlow::FlowState state2 | ||
) { | ||
any(TemplateInjectionAdditionalTaintStep a).isAdditionalTaintStep(node1, state1, node2, state2) | ||
} | ||
} |
File renamed without changes.
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd"> | ||
<qhelp> | ||
<overview> | ||
<p> | ||
Template Injection occurs when user input is embedded in a template's code in an unsafe manner. | ||
An attacker can use native template syntax to inject a malicious payload into a template, which is then executed server-side. | ||
This permits the attacker to run arbitrary code in the server's context. | ||
</p> | ||
</overview> | ||
<recommendation> | ||
<p> | ||
To fix this, ensure that untrusted input is not used as part of a template's code. If the application requirements do not allow this, | ||
use a sandboxed environment where access to unsafe attributes and methods is prohibited. | ||
</p> | ||
</recommendation> | ||
<example> | ||
<p> | ||
In the example given below, an untrusted HTTP parameter <code>code</code> is used as a Velocity template string. | ||
This can lead to remote code execution. | ||
</p> | ||
<sample src="SSTIBad.java" /> | ||
|
||
<p> | ||
In the next example the problem is avoided by using a fixed template string <code>s</code>. | ||
atorralba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Since the template's code is not attacker-controlled in this case, the untrusted code execution is prevented. | ||
atorralba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</p> | ||
<sample src="SSTIGood.java" /> | ||
</example> | ||
<references> | ||
<li>Portswigger: <a href="https://portswigger.net/web-security/server-side-template-injection">Server Side Template Injection</a></li> | ||
atorralba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</references> | ||
</qhelp> |
8 changes: 5 additions & 3 deletions
8
...Security/CWE/CWE-094/TemplateInjection.ql → ...Security/CWE/CWE-094/TemplateInjection.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
java/ql/src/change-notes/2022-09-08-server-side-template-injection-query.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
--- | ||
category: newQuery | ||
--- | ||
* The query "Server-side template injection" (`java/server-side-template-injection`) has been promoted from experimental to the main query pack. This query was originally [submitted as an experimental query by @porcupineyhairs](https://github.com/github/codeql/pull/5935). |
31 changes: 0 additions & 31 deletions
31
java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qhelp
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.