mirror of
https://github.com/github/codeql.git
synced 2025-12-21 03:06:31 +01:00
File path injection with the JFinal framework
This commit is contained in:
luchua-bc
@@ -0,0 +1,21 @@
|
||||
// BAD: no file download validation
|
||||
HttpServletRequest request = getRequest();
|
||||
String path = request.getParameter("path");
|
||||
String filePath = "/pages/" + path;
|
||||
HttpServletResponse resp = getResponse();
|
||||
File file = new File(filePath);
|
||||
resp.getOutputStream().write(file.readContent());
|
||||
|
||||
// BAD: no file upload validation
|
||||
String savePath = getPara("dir");
|
||||
File file = getFile("fileParam").getFile();
|
||||
FileInputStream fis = new FileInputStream(file);
|
||||
String filePath = "/files/" + savePath;
|
||||
FileOutputStream fos = new FileOutputStream(filePath);
|
||||
|
||||
// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
|
||||
// (alternatively use `Path.normalize` instead of checking for `..`)
|
||||
if (!filePath.contains("..") && filePath.hasPrefix("/pages")) { ... }
|
||||
// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check:
|
||||
// (alternatively use `URLDecoder.decode` before `hasPrefix`)
|
||||
if (filePath.hasPrefix("/files") && !filePath.contains("%")) { ... }
|
||||
@@ -0,0 +1,39 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
|
||||
<overview>
|
||||
<p>External Control of File Name or Path, also called File Path Injection, is a vulnerability that a file path
|
||||
being accessed is composed using data from outside the application (such as the HTTP request, the database, or
|
||||
the filesystem). It allows an attacker to traverse through the filesystem and access arbitrary files.</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>Unsanitized user provided data must not be used to construct the file path. In order to prevent File
|
||||
Path Injection, it is recommended to avoid concatenating user input directly into the file path. Instead,
|
||||
user input should be checked against allowed (e.g., must come within <code>user_content/</code>) or disallowed
|
||||
(e.g. must not come within <code>/internal</code>) paths, ensuring that neither path traversal using <code>../</code>
|
||||
or URL encoding are used to evade these checks.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>The following examples show the bad case and the good case respectively.
|
||||
The <code>BAD</code> methods show an HTTP request parameter being used directly to construct a file path
|
||||
without validating the input, which may cause file leakage. In the <code>GOOD</code> method, file path
|
||||
is validated.
|
||||
</p>
|
||||
<sample src="FilePathInjection.java" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>OWASP:
|
||||
<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
|
||||
</li>
|
||||
<li>Veracode:
|
||||
<a href="https://www.veracode.com/security/dotnet/cwe-73">External Control of File Name or Path Flaw</a>.
|
||||
</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -0,0 +1,55 @@
|
||||
/**
|
||||
* @name File Path Injection
|
||||
* @description Loading files based on unvalidated user-input may cause file information disclosure
|
||||
* and uploading files with unvalidated file types to an arbitrary directory may lead to
|
||||
* Remote Command Execution (RCE).
|
||||
* @kind path-problem
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @id java/file-path-injection
|
||||
* @tags security
|
||||
* external/cwe-073
|
||||
*/
|
||||
|
||||
import java
|
||||
import semmle.code.java.dataflow.FlowSources
|
||||
import JFinalController
|
||||
import PathSanitizer
|
||||
import DataFlow::PathGraph
|
||||
|
||||
/**
|
||||
* A sink that represents a file read operation.
|
||||
*/
|
||||
private class ReadFileSinkModels extends SinkModelCsv {
|
||||
override predicate row(string row) {
|
||||
row =
|
||||
[
|
||||
"java.io;FileInputStream;false;FileInputStream;;;Argument[0];read-file",
|
||||
"java.io;File;false;File;;;Argument[0];read-file"
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A sink that represents a file creation or access, such as a file read, write, copy or move operation.
|
||||
*/
|
||||
private class FileAccessSink extends DataFlow::Node {
|
||||
FileAccessSink() { sinkNode(this, "create-file") or sinkNode(this, "read-file") }
|
||||
}
|
||||
|
||||
class InjectFilePathConfig extends TaintTracking::Configuration {
|
||||
InjectFilePathConfig() { this = "InjectFilePathConfig" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) { sink instanceof FileAccessSink }
|
||||
|
||||
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
|
||||
guard instanceof PathTraversalBarrierGuard
|
||||
}
|
||||
}
|
||||
|
||||
from DataFlow::PathNode source, DataFlow::PathNode sink, InjectFilePathConfig conf
|
||||
where conf.hasFlowPath(source, sink)
|
||||
select sink.getNode(), source, sink, "External control of file name or path due to $@.",
|
||||
source.getNode(), "user-provided value"
|
||||
@@ -0,0 +1,44 @@
|
||||
import java
|
||||
import semmle.code.java.dataflow.FlowSources
|
||||
|
||||
/** The class `com.jfinal.config.Routes`. */
|
||||
class JFinalRoutes extends RefType {
|
||||
JFinalRoutes() { this.hasQualifiedName("com.jfinal.config", "Routes") }
|
||||
}
|
||||
|
||||
/** The method `add` of the class `Routes`. */
|
||||
class AddJFinalRoutes extends Method {
|
||||
AddJFinalRoutes() {
|
||||
this.getDeclaringType() instanceof JFinalRoutes and
|
||||
this.getName() = "add"
|
||||
}
|
||||
}
|
||||
|
||||
/** The class `com.jfinal.core.Controller`. */
|
||||
class JFinalController extends RefType {
|
||||
JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") }
|
||||
}
|
||||
|
||||
/** Source model of remote flow source with `JFinal`. */
|
||||
private class JFinalControllerSource extends SourceModelCsv {
|
||||
override predicate row(string row) {
|
||||
row =
|
||||
[
|
||||
"com.jfinal.core;Controller;true;getAttr" + ["", "ForInt", "ForStr"] +
|
||||
";;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;getCookie" + ["", "Object", "Objects", "ToInt", "ToLong"] +
|
||||
";;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;getFile" + ["", "s"] + ";;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;getHeader;;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;getKv;;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;getPara" +
|
||||
[
|
||||
"", "Map", "ToBoolean", "ToDate", "ToInt", "ToLong", "Values", "ValuesToInt",
|
||||
"ValuesToLong"
|
||||
] + ";;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;getSession" + ["", "Attr"] + ";;;ReturnValue;remote",
|
||||
"com.jfinal.core;Controller;true;get" + ["", "Int", "Long", "Boolean", "Date"] +
|
||||
";;;ReturnValue;remote"
|
||||
]
|
||||
}
|
||||
}
|
||||
175
java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll
Normal file
175
java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll
Normal file
@@ -0,0 +1,175 @@
|
||||
import java
|
||||
import semmle.code.java.controlflow.Guards
|
||||
import semmle.code.java.dataflow.FlowSources
|
||||
|
||||
/** A barrier guard that protects against path traversal vulnerabilities. */
|
||||
abstract class PathTraversalBarrierGuard extends DataFlow::BarrierGuard { }
|
||||
|
||||
/**
|
||||
* A guard that considers safe a string being exactly compared to a trusted value.
|
||||
*/
|
||||
private class ExactStringPathMatchGuard extends PathTraversalBarrierGuard instanceof MethodAccess {
|
||||
ExactStringPathMatchGuard() {
|
||||
super.getMethod().getDeclaringType() instanceof TypeString and
|
||||
super.getMethod().getName() = ["equals", "equalsIgnoreCase"]
|
||||
}
|
||||
|
||||
override predicate checks(Expr e, boolean branch) {
|
||||
e = super.getQualifier() and
|
||||
branch = true
|
||||
}
|
||||
}
|
||||
|
||||
private class AllowListGuard extends Guard instanceof MethodAccess {
|
||||
AllowListGuard() {
|
||||
(isStringPartialMatch(this) or isPathPartialMatch(this)) and
|
||||
not isDisallowedWord(super.getAnArgument())
|
||||
}
|
||||
|
||||
Expr getCheckedExpr() { result = super.getQualifier() }
|
||||
}
|
||||
|
||||
/**
|
||||
* A guard that considers a path safe because it is checked against an allowlist of partial trusted values.
|
||||
* This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
|
||||
* or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
|
||||
*/
|
||||
private class AllowListBarrierGuard extends PathTraversalBarrierGuard instanceof AllowListGuard {
|
||||
override predicate checks(Expr e, boolean branch) {
|
||||
e = super.getCheckedExpr() and
|
||||
branch = true and
|
||||
(
|
||||
// Either a path normalization sanitizer comes before the guard,
|
||||
exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
|
||||
or
|
||||
// or a check like `!path.contains("..")` comes before the guard
|
||||
exists(PathTraversalGuard previousGuard |
|
||||
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
|
||||
previousGuard.controls(this.getBasicBlock().(ConditionBlock), false)
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A guard that considers a path safe because it is checked for `..` components, having previously
|
||||
* been checked for a trusted prefix.
|
||||
*/
|
||||
private class DotDotCheckBarrierGuard extends PathTraversalBarrierGuard instanceof PathTraversalGuard {
|
||||
override predicate checks(Expr e, boolean branch) {
|
||||
e = super.getCheckedExpr() and
|
||||
branch = false and
|
||||
// The same value has previously been checked against a list of allowed prefixes:
|
||||
exists(AllowListGuard previousGuard |
|
||||
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
|
||||
previousGuard.controls(this.getBasicBlock().(ConditionBlock), true)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private class BlockListGuard extends Guard instanceof MethodAccess {
|
||||
BlockListGuard() {
|
||||
(isStringPartialMatch(this) or isPathPartialMatch(this)) and
|
||||
isDisallowedWord(super.getAnArgument())
|
||||
}
|
||||
|
||||
Expr getCheckedExpr() { result = super.getQualifier() }
|
||||
}
|
||||
|
||||
/**
|
||||
* A guard that considers a string safe because it is checked against a blocklist of known dangerous values.
|
||||
* This requires a prior check for URL encoding concealing a forbidden value, either a guard (`UrlEncodingGuard`)
|
||||
* or a sanitizer (`UrlDecodeSanitizer`).
|
||||
*/
|
||||
private class BlockListBarrierGuard extends PathTraversalBarrierGuard instanceof BlockListGuard {
|
||||
override predicate checks(Expr e, boolean branch) {
|
||||
e = super.getCheckedExpr() and
|
||||
branch = false and
|
||||
(
|
||||
// Either `e` has been URL decoded:
|
||||
exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
|
||||
or
|
||||
// or `e` has previously been checked for URL encoding sequences:
|
||||
exists(UrlEncodingGuard previousGuard |
|
||||
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
|
||||
previousGuard.controls(this.getBasicBlock(), false)
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A guard that considers a string safe because it is checked for URL encoding sequences,
|
||||
* having previously been checked against a block-list of forbidden values.
|
||||
*/
|
||||
private class URLEncodingBarrierGuard extends PathTraversalBarrierGuard instanceof UrlEncodingGuard {
|
||||
override predicate checks(Expr e, boolean branch) {
|
||||
e = super.getCheckedExpr() and
|
||||
branch = false and
|
||||
exists(BlockListGuard previousGuard |
|
||||
DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
|
||||
previousGuard.controls(this.getBasicBlock(), false)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `ma` is a call to a method that checks a partial string match.
|
||||
*/
|
||||
private predicate isStringPartialMatch(MethodAccess ma) {
|
||||
ma.getMethod().getDeclaringType() instanceof TypeString and
|
||||
ma.getMethod().getName() =
|
||||
["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"]
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match.
|
||||
*/
|
||||
private predicate isPathPartialMatch(MethodAccess ma) {
|
||||
ma.getMethod().getDeclaringType() instanceof TypePath and
|
||||
ma.getMethod().getName() = "startsWith"
|
||||
}
|
||||
|
||||
private predicate isDisallowedWord(CompileTimeConstantExpr word) {
|
||||
word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"])
|
||||
}
|
||||
|
||||
/** A complementary guard that protects against path traversal, by looking for the literal `..`. */
|
||||
class PathTraversalGuard extends Guard instanceof MethodAccess {
|
||||
Expr checked;
|
||||
|
||||
PathTraversalGuard() {
|
||||
super.getMethod().getDeclaringType() instanceof TypeString and
|
||||
super.getMethod().hasName(["contains", "indexOf"]) and
|
||||
super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
|
||||
}
|
||||
|
||||
Expr getCheckedExpr() { result = super.getQualifier() }
|
||||
}
|
||||
|
||||
/** A complementary sanitizer that protects against path traversal using path normalization. */
|
||||
private class PathNormalizeSanitizer extends MethodAccess {
|
||||
PathNormalizeSanitizer() {
|
||||
this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
|
||||
this.getMethod().hasName("normalize")
|
||||
}
|
||||
}
|
||||
|
||||
/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */
|
||||
private class UrlEncodingGuard extends Guard instanceof MethodAccess {
|
||||
UrlEncodingGuard() {
|
||||
super.getMethod().getDeclaringType() instanceof TypeString and
|
||||
super.getMethod().hasName(["contains", "indexOf"]) and
|
||||
super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
|
||||
}
|
||||
|
||||
Expr getCheckedExpr() { result = super.getQualifier() }
|
||||
}
|
||||
|
||||
/** A complementary sanitizer that protects against double URL encoding using URL decoding. */
|
||||
private class UrlDecodeSanitizer extends MethodAccess {
|
||||
UrlDecodeSanitizer() {
|
||||
this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
|
||||
this.getMethod().hasName("decode")
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user