From 27043a09b3899f7da6335b3cfa1e788ae42ed90c Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 23 Jan 2022 18:07:48 +0000 Subject: [PATCH 01/11] File path injection with the JFinal framework --- .../CWE/CWE-073/FilePathInjection.java | 21 ++ .../CWE/CWE-073/FilePathInjection.qhelp | 39 +++ .../Security/CWE/CWE-073/FilePathInjection.ql | 55 +++ .../Security/CWE/CWE-073/JFinalController.qll | 44 +++ .../Security/CWE/CWE-073/PathSanitizer.qll | 175 +++++++++ .../CWE-073/FilePathInjection.expected | 23 ++ .../security/CWE-073/FilePathInjection.java | 127 +++++++ .../security/CWE-073/FilePathInjection.qlref | 1 + .../query-tests/security/CWE-073/options | 1 + .../com/jfinal/core/Controller.java | 331 ++++++++++++++++++ .../jfinal-4.9.15/com/jfinal/kit/Kv.java | 6 + .../com/jfinal/upload/UploadFile.java | 32 ++ 12 files changed, 855 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll create mode 100644 java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll create mode 100644 java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-073/options create mode 100644 java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java create mode 100644 java/ql/test/stubs/jfinal-4.9.15/com/jfinal/kit/Kv.java create mode 100644 java/ql/test/stubs/jfinal-4.9.15/com/jfinal/upload/UploadFile.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.java b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.java new file mode 100644 index 000000000000..6bee08e2f3ac --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.java @@ -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("%")) { ... } \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp new file mode 100644 index 000000000000..e5f6426413e7 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp @@ -0,0 +1,39 @@ + + + + + +

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.

+ + + +

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 user_content/) or disallowed +(e.g. must not come within /internal) paths, ensuring that neither path traversal using ../ +or URL encoding are used to evade these checks. +

+ + + +

The following examples show the bad case and the good case respectively. +The BAD 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 GOOD method, file path +is validated. +

+ + + + +
  • OWASP: + Path Traversal. +
    • +
  • Veracode: + External Control of File Name or Path Flaw. +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql new file mode 100644 index 000000000000..6ad4c7baed11 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -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" diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll new file mode 100644 index 000000000000..a614c0d5be34 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -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" + ] + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll new file mode 100644 index 000000000000..f5521ab9c6d3 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll @@ -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") + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected new file mode 100644 index 000000000000..c8e400270cd0 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected @@ -0,0 +1,23 @@ +edges +| FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | +| FilePathInjection.java:61:50:61:58 | file : File | FilePathInjection.java:66:30:66:33 | file | +| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath | +| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath : String | +| FilePathInjection.java:92:15:92:32 | new File(...) : File | FilePathInjection.java:100:19:100:22 | file : File | +| FilePathInjection.java:92:24:92:31 | filePath : String | FilePathInjection.java:92:15:92:32 | new File(...) : File | +| FilePathInjection.java:100:19:100:22 | file : File | FilePathInjection.java:61:50:61:58 | file : File | +nodes +| FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String | +| FilePathInjection.java:25:47:25:59 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:61:50:61:58 | file : File | semmle.label | file : File | +| FilePathInjection.java:66:30:66:33 | file | semmle.label | file | +| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| FilePathInjection.java:92:15:92:32 | new File(...) : File | semmle.label | new File(...) : File | +| FilePathInjection.java:92:24:92:31 | filePath | semmle.label | filePath | +| FilePathInjection.java:92:24:92:31 | filePath : String | semmle.label | filePath : String | +| FilePathInjection.java:100:19:100:22 | file : File | semmle.label | file : File | +subpaths +#select +| FilePathInjection.java:25:47:25:59 | finalFilePath | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:20:21:20:34 | getPara(...) | user-provided value | +| FilePathInjection.java:66:30:66:33 | file | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:66:30:66:33 | file | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value | +| FilePathInjection.java:92:24:92:31 | filePath | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java new file mode 100644 index 000000000000..da718bfec9be --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java @@ -0,0 +1,127 @@ +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.OutputStream; +import java.io.IOException; +import java.io.File; +import java.nio.file.Path; +import java.nio.file.Paths; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import com.jfinal.core.Controller; + +public class FilePathInjection extends Controller { + private static final String BASE_PATH = "/pages"; + + // BAD: Upload file to user specified path without validation + public void uploadFile() throws IOException { + String savePath = getPara("dir"); + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + savePath; + + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(finalFilePath); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + + // GOOD: Upload file to user specified path with path normalization and validation + public void uploadFile2() throws IOException { + String savePath = getPara("dir"); + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + savePath; + Path path = Paths.get(finalFilePath).normalize(); + + if (path.startsWith(BASE_PATH)) { + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(path.toFile()); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + } + + private void readFile(HttpServletResponse resp, File file) { + OutputStream os = null; + FileInputStream fis = null; + try { + os = resp.getOutputStream(); + fis = new FileInputStream(file); + byte fileContent[] = new byte[(int) file.length()]; + fis.read(fileContent); + os.write(fileContent); + } catch (Exception e) { + System.err.println("Invalid directory or file " + file.getName()); + } finally { + try { + if (os != null) + os.close(); + } catch (Exception e2) { + } + try { + if (fis != null) + fis.close(); + } catch (Exception e2) { + } + } + } + + public void downloadFile() throws FileNotFoundException, IOException { + HttpServletRequest request = getRequest(); + String path = request.getParameter("path"); + String filePath = BASE_PATH + path; + + HttpServletResponse resp = getResponse(); + File file = new File(filePath); + if (path != null && file.exists()) { + resp.setHeader("Content-type", "application/force-download"); + resp.setHeader("Content-Disposition", "inline;filename=\"" + filePath + "\""); + resp.setHeader("Content-Transfer-Encoding", "Binary"); + resp.setHeader("Content-length", "" + file.length()); + resp.setHeader("Content-Type", "application/octet-stream"); + resp.setHeader("Content-Disposition", "attachment; filename=\"" + file.getName() + "\""); + readFile(resp, file); + } else { + System.err.println("File does not exist " + path); + } + } + + public void downloadFile2() throws FileNotFoundException, IOException { + HttpServletRequest request = getRequest(); + String path = request.getParameter("path"); + String filePath = BASE_PATH + path; + + HttpServletResponse resp = getResponse(); + if (!filePath.contains("..") && filePath.startsWith(BASE_PATH)) { + File file = new File(filePath); + if (file.exists()) { + resp.setHeader("Content-type", "application/force-download"); + resp.setHeader("Content-Disposition", "inline;filename=\"" + filePath + "\""); + resp.setHeader("Content-Transfer-Encoding", "Binary"); + resp.setHeader("Content-length", "" + file.length()); + resp.setHeader("Content-Type", "application/octet-stream"); + resp.setHeader("Content-Disposition", "attachment; filename=\"" + file.getName() + "\""); + readFile(resp, file); + } else { + System.err.println("File does not exist " + path); + } + } + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.qlref b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.qlref new file mode 100644 index 000000000000..3c6db8058a65 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-073/FilePathInjection.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/options b/java/ql/test/experimental/query-tests/security/CWE-073/options new file mode 100644 index 000000000000..373894974154 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-073/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jfinal-4.9.15 \ No newline at end of file diff --git a/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java new file mode 100644 index 000000000000..74e4335e442c --- /dev/null +++ b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java @@ -0,0 +1,331 @@ +package com.jfinal.core; + +import java.io.File; +import java.util.Date; +import java.util.Enumeration; +import java.util.List; +import java.util.Map; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import com.jfinal.kit.Kv; +import com.jfinal.upload.UploadFile; + +public abstract class Controller { + public String getPara(String name) { + return null; + } + + public String getPara(String name, String defaultValue) { + return null; + } + + public Map getParaMap() { + return null; + } + + public Enumeration getParaNames() { + return null; + } + + public String[] getParaValues(String name) { + return null; + } + + public Integer[] getParaValuesToInt(String name) { + return null; + } + + public Long[] getParaValuesToLong(String name) { + return null; + } + + public Enumeration getAttrNames() { + return null; + } + + public T getAttr(String name) { + return null; + } + + public T getAttr(String name, T defaultValue) { + return null; + } + + public String getAttrForStr(String name) { + return null; + } + + public Integer getAttrForInt(String name) { + return -1; + } + + public String getHeader(String name) { + return null; + } + + public Integer getParaToInt(String name) { + return null; + } + + public Integer getParaToInt(String name, Integer defaultValue) { + return null; + } + + public Long getParaToLong(String name) { + return null; + } + + public Long getParaToLong(String name, Long defaultValue) { + return null; + } + + public Boolean getParaToBoolean(String name) { + return false; + } + + public Boolean getParaToBoolean(String name, Boolean defaultValue) { + return false; + } + + public Boolean getParaToBoolean() { + return false; + } + + public Boolean getParaToBoolean(int index) { + return false; + } + + public Boolean getParaToBoolean(int index, Boolean defaultValue) { + return false; + } + + public Date getParaToDate(String name) { + return null; + } + + public Date getParaToDate(String name, Date defaultValue) { + return null; + } + + public Date getParaToDate() { + return null; + } + + public HttpServletRequest getRequest() { + return null; + } + + public HttpServletResponse getResponse() { + return null; + } + + public HttpSession getSession() { + return null; + } + + public HttpSession getSession(boolean create) { + return null; + } + + public T getSessionAttr(String key) { + return null; + } + + public T getSessionAttr(String key, T defaultValue) { + return null; + } + + public String getCookie(String name, String defaultValue) { + return null; + } + + public String getCookie(String name) { + return null; + } + + public Integer getCookieToInt(String name) { + return null; + } + + public Integer getCookieToInt(String name, Integer defaultValue) { + return null; + } + + public Long getCookieToLong(String name) { + return null; + } + + public Long getCookieToLong(String name, Long defaultValue) { + return null; + } + + public Cookie getCookieObject(String name) { + return null; + } + + public Cookie[] getCookieObjects() { + return null; + } + + public String getPara() { + return null; + } + + public String getPara(int index) { + return null; + } + + public String getPara(int index, String defaultValue) { + return null; + } + + public Integer getParaToInt(int index) { + return null; + } + + public Integer getParaToInt(int index, Integer defaultValue) { + return null; + } + + public Long getParaToLong(int index) { + return null; + } + + public Long getParaToLong(int index, Long defaultValue) { + return null; + } + + public Integer getParaToInt() { + return null; + } + + public Long getParaToLong() { + return null; + } + + public Kv getKv() { + return null; + } + + public List getFiles(String uploadPath, Integer maxPostSize, String encoding) { + return null; + } + + public UploadFile getFile(String parameterName, String uploadPath, Integer maxPostSize, String encoding) { + return null; + } + + public List getFiles(String uploadPath, int maxPostSize) { + return null; + } + + public UploadFile getFile(String parameterName, String uploadPath, int maxPostSize) { + return null; + } + + public List getFiles(String uploadPath) { + return null; + } + + public UploadFile getFile(String parameterName, String uploadPath) { + return null; + } + + public List getFiles() { + return null; + } + + public UploadFile getFile() { + return null; + } + + public UploadFile getFile(String parameterName) { + return null; + } + + public String get(String name) { + return null; + } + + public String get(String name, String defaultValue) { + return null; + } + + public Integer getInt(String name) { + return -1; + } + + public Integer getInt(String name, Integer defaultValue) { + return -1; + } + + public Long getLong(String name) { + return null; + } + + public Long getLong(String name, Long defaultValue) { + return null; + } + + public Boolean getBoolean(String name) { + return false; + } + + public Boolean getBoolean(String name, Boolean defaultValue) { + return false; + } + + public Date getDate(String name) { + return null; + } + + public Date getDate(String name, Date defaultValue) { + return null; + } + + public String get(int index) { + return null; + } + + public String get(int index, String defaultValue) { + return null; + } + + public Integer getInt() { + return -1; + } + + public Integer getInt(int index) { + return -1; + } + + public Integer getInt(int index, Integer defaultValue) { + return -1; + } + + public Long getLong() { + return null; + } + + public Long getLong(int index) { + return null; + } + + public Long getLong(int index, Long defaultValue) { + return null; + } + + public Boolean getBoolean() { + return false; + } + + public Boolean getBoolean(int index) { + return false; + } + + public Boolean getBoolean(int index, Boolean defaultValue) { + return false; + } +} diff --git a/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/kit/Kv.java b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/kit/Kv.java new file mode 100644 index 000000000000..ff8b112d05c9 --- /dev/null +++ b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/kit/Kv.java @@ -0,0 +1,6 @@ +package com.jfinal.kit; + +import java.util.HashMap; + +public class Kv extends HashMap { +} diff --git a/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/upload/UploadFile.java b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/upload/UploadFile.java new file mode 100644 index 000000000000..2e6063c48529 --- /dev/null +++ b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/upload/UploadFile.java @@ -0,0 +1,32 @@ +package com.jfinal.upload; + +import java.io.File; + +public class UploadFile { + public UploadFile(String parameterName, String uploadPath, String filesystemName, String originalFileName, String contentType) { + } + + public String getParameterName() { + return null; + } + + public String getFileName() { + return null; + } + + public String getOriginalFileName() { + return null; + } + + public String getContentType() { + return null; + } + + public String getUploadPath() { + return null; + } + + public File getFile() { + return null; + } +} From ff4826d20316d143c318020e54072bd1ca4cb97e Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 8 Feb 2022 04:02:27 +0000 Subject: [PATCH 02/11] Correct the data model and update qldoc --- .../CWE/CWE-073/FilePathInjection.qhelp | 14 ++-- .../Security/CWE/CWE-073/FilePathInjection.ql | 34 +++------ .../Security/CWE/CWE-073/JFinalController.qll | 76 +++++++++++++++---- .../CWE-073/FilePathInjection.expected | 27 +++---- .../security/CWE-073/FilePathInjection.java | 23 ++++++ .../com/jfinal/core/Controller.java | 16 ++++ 6 files changed, 132 insertions(+), 58 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp index e5f6426413e7..040bf906b1bd 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp @@ -5,24 +5,24 @@ -

    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 +

    External Control of File Name or Path, also called File Path Injection, is a vulnerability by which +a file path is created 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.

     -

    Unsanitized user provided data must not be used to construct the file path. In order to prevent File +

    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 user_content/) or disallowed -(e.g. must not come within /internal) paths, ensuring that neither path traversal using ../ -or URL encoding are used to evade these checks. +user input should be checked against allowed or disallowed paths (for example, the path must be within +/user_content/ or must not be within /internal), ensuring that neither path +traversal using ../ nor URL encoding is used to evade these checks.

    The following examples show the bad case and the good case respectively. The BAD 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 GOOD method, file path +without validating the input, which may cause file leakage. In the GOOD method, the file path is validated.

    diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index 6ad4c7baed11..8909e0856f6e 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -13,36 +13,26 @@ import java import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.PathCreation 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 isSource(DataFlow::Node source) { + source instanceof RemoteFlowSource or + isGetAttributeFromRemoteSource(source.asExpr()) + } + + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(PathCreation p).getAnInput() + } - override predicate isSink(DataFlow::Node sink) { sink instanceof FileAccessSink } + override predicate isSanitizer(DataFlow::Node node) { + exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType) + } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { guard instanceof PathTraversalBarrierGuard diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll index a614c0d5be34..b08073b0ec48 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -1,22 +1,73 @@ import java +import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.dataflow.FlowSources -/** The class `com.jfinal.config.Routes`. */ -class JFinalRoutes extends RefType { - JFinalRoutes() { this.hasQualifiedName("com.jfinal.config", "Routes") } +/** The class `com.jfinal.core.Controller`. */ +class JFinalController extends RefType { + JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") } } -/** The method `add` of the class `Routes`. */ -class AddJFinalRoutes extends Method { - AddJFinalRoutes() { - this.getDeclaringType() instanceof JFinalRoutes and - this.getName() = "add" +/** The method `getSessionAttr` of `JFinalController`. */ +class GetSessionAttributeMethod extends Method { + GetSessionAttributeMethod() { + this.getName() = "getSessionAttr" and + this.getDeclaringType().getASupertype*() instanceof JFinalController } } -/** The class `com.jfinal.core.Controller`. */ -class JFinalController extends RefType { - JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") } +/** The method `setSessionAttr` of `JFinalController`. */ +class SetSessionAttributeMethod extends Method { + SetSessionAttributeMethod() { + this.getName() = "setSessionAttr" and + this.getDeclaringType().getASupertype*() instanceof JFinalController + } +} + +/** The request attribute getter method of `JFinalController`. */ +class GetRequestAttributeMethod extends Method { + GetRequestAttributeMethod() { + this.getName().matches("getAttr%") and + this.getDeclaringType().getASupertype*() instanceof JFinalController + } +} + +/** The request attribute setter method of `JFinalController`. */ +class SetRequestAttributeMethod extends Method { + SetRequestAttributeMethod() { + this.getName() = ["set", "setAttr", "setAttrs"] and + this.getDeclaringType().getASupertype*() instanceof JFinalController + } +} + +/** Taint configuration of flow from remote source to attribute setter methods. */ +class SetRemoteAttributeConfig extends TaintTracking2::Configuration { + SetRemoteAttributeConfig() { this = "SetRemoteAttributeConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod() instanceof SetSessionAttributeMethod and + sink.asExpr() = ma.getArgument(1) + ) + } +} + +/** Holds if the result of an attribute getter call is from a method invocation of remote attribute setter. */ +predicate isGetAttributeFromRemoteSource(Expr expr) { + exists(MethodAccess gma, MethodAccess sma | + ( + gma.getMethod() instanceof GetSessionAttributeMethod and + sma.getMethod() instanceof SetSessionAttributeMethod + or + gma.getMethod() instanceof GetRequestAttributeMethod and + sma.getMethod() instanceof SetRequestAttributeMethod + ) and + expr = gma and + gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = + sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and + exists(SetRemoteAttributeConfig cc | cc.hasFlowToExpr(sma.getArgument(1))) + ) } /** Source model of remote flow source with `JFinal`. */ @@ -24,8 +75,6 @@ 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", @@ -36,7 +85,6 @@ private class JFinalControllerSource extends SourceModelCsv { "", "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" ] diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected index c8e400270cd0..6a0454c38cfe 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected @@ -1,23 +1,20 @@ edges | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | -| FilePathInjection.java:61:50:61:58 | file : File | FilePathInjection.java:66:30:66:33 | file | -| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath | -| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath : String | -| FilePathInjection.java:92:15:92:32 | new File(...) : File | FilePathInjection.java:100:19:100:22 | file : File | -| FilePathInjection.java:92:24:92:31 | filePath : String | FilePathInjection.java:92:15:92:32 | new File(...) : File | -| FilePathInjection.java:100:19:100:22 | file : File | FilePathInjection.java:61:50:61:58 | file : File | +| FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath | +| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath | +| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath | nodes | FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:25:47:25:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:61:50:61:58 | file : File | semmle.label | file : File | -| FilePathInjection.java:66:30:66:33 | file | semmle.label | file | -| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| FilePathInjection.java:92:15:92:32 | new File(...) : File | semmle.label | new File(...) : File | -| FilePathInjection.java:92:24:92:31 | filePath | semmle.label | filePath | -| FilePathInjection.java:92:24:92:31 | filePath : String | semmle.label | filePath : String | -| FilePathInjection.java:100:19:100:22 | file : File | semmle.label | file : File | +| FilePathInjection.java:40:21:40:34 | getPara(...) : String | semmle.label | getPara(...) : String | +| FilePathInjection.java:43:25:43:37 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String | +| FilePathInjection.java:71:47:71:59 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| FilePathInjection.java:115:24:115:31 | filePath | semmle.label | filePath | subpaths #select | FilePathInjection.java:25:47:25:59 | finalFilePath | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:20:21:20:34 | getPara(...) | user-provided value | -| FilePathInjection.java:66:30:66:33 | file | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:66:30:66:33 | file | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value | -| FilePathInjection.java:92:24:92:31 | filePath | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value | +| FilePathInjection.java:43:25:43:37 | finalFilePath | FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:40:21:40:34 | getPara(...) | user-provided value | +| FilePathInjection.java:71:47:71:59 | finalFilePath | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) | user-provided value | +| FilePathInjection.java:115:24:115:31 | filePath | FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:111:17:111:44 | getParameter(...) | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java index da718bfec9be..f5f253abf38a 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java @@ -58,6 +58,29 @@ public void uploadFile2() throws IOException { } } + // BAD: Upload file to user specified path without validation + public void uploadFile3() throws IOException { + String savePath = getPara("dir"); + setSessionAttr("uploadDir", savePath); + String sessionUploadDir = getSessionAttr("uploadDir"); + + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + sessionUploadDir; + + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(finalFilePath); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + private void readFile(HttpServletResponse resp, File file) { OutputStream os = null; FileInputStream fis = null; diff --git a/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java index 74e4335e442c..72884b70268b 100644 --- a/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java +++ b/java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java @@ -41,6 +41,14 @@ public Long[] getParaValuesToLong(String name) { return null; } + public Controller setAttr(String name, Object value) { + return null; + } + + public Controller setAttrs(Map attrMap) { + return null; + } + public Enumeration getAttrNames() { return null; } @@ -137,6 +145,10 @@ public T getSessionAttr(String key, T defaultValue) { return null; } + public Controller setSessionAttr(String key, Object value) { + return null; + } + public String getCookie(String name, String defaultValue) { return null; } @@ -245,6 +257,10 @@ public UploadFile getFile(String parameterName) { return null; } + public Controller set(String attributeName, Object attributeValue) { + return null; + } + public String get(String name) { return null; } From 4609227e769385a026ebff2435ab270bd7ce5bb3 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 9 Feb 2022 03:24:46 +0000 Subject: [PATCH 03/11] Use data model for request/session attribute operations --- .../Security/CWE/CWE-073/FilePathInjection.ql | 5 +- .../Security/CWE/CWE-073/JFinalController.qll | 52 +++++++------------ .../CWE-073/FilePathInjection.expected | 52 +++++++++++++------ .../security/CWE-073/FilePathInjection.java | 49 ++++++++++++++++- 4 files changed, 104 insertions(+), 54 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index 8909e0856f6e..8349606f7202 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -21,10 +21,7 @@ import DataFlow::PathGraph class InjectFilePathConfig extends TaintTracking::Configuration { InjectFilePathConfig() { this = "InjectFilePathConfig" } - override predicate isSource(DataFlow::Node source) { - source instanceof RemoteFlowSource or - isGetAttributeFromRemoteSource(source.asExpr()) - } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(PathCreation p).getAnInput() diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll index b08073b0ec48..49123859dfe8 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -1,5 +1,4 @@ import java -import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.dataflow.FlowSources /** The class `com.jfinal.core.Controller`. */ @@ -34,42 +33,11 @@ class GetRequestAttributeMethod extends Method { /** The request attribute setter method of `JFinalController`. */ class SetRequestAttributeMethod extends Method { SetRequestAttributeMethod() { - this.getName() = ["set", "setAttr", "setAttrs"] and + this.getName() = ["set", "setAttr"] and this.getDeclaringType().getASupertype*() instanceof JFinalController } } -/** Taint configuration of flow from remote source to attribute setter methods. */ -class SetRemoteAttributeConfig extends TaintTracking2::Configuration { - SetRemoteAttributeConfig() { this = "SetRemoteAttributeConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof SetSessionAttributeMethod and - sink.asExpr() = ma.getArgument(1) - ) - } -} - -/** Holds if the result of an attribute getter call is from a method invocation of remote attribute setter. */ -predicate isGetAttributeFromRemoteSource(Expr expr) { - exists(MethodAccess gma, MethodAccess sma | - ( - gma.getMethod() instanceof GetSessionAttributeMethod and - sma.getMethod() instanceof SetSessionAttributeMethod - or - gma.getMethod() instanceof GetRequestAttributeMethod and - sma.getMethod() instanceof SetRequestAttributeMethod - ) and - expr = gma and - gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = - sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and - exists(SetRemoteAttributeConfig cc | cc.hasFlowToExpr(sma.getArgument(1))) - ) -} - /** Source model of remote flow source with `JFinal`. */ private class JFinalControllerSource extends SourceModelCsv { override predicate row(string row) { @@ -90,3 +58,21 @@ private class JFinalControllerSource extends SourceModelCsv { ] } } + +/** `JFinal` data model related to session and request attribute operations. */ +private class JFinalDataModel extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + "com.jfinal.core;Controller;true;setSessionAttr;;;Argument[0];MapKey of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];value", + "com.jfinal.core;Controller;true;setSessionAttr;;;Argument[1];MapValue of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];value", + "com.jfinal.core;Controller;true;getSessionAttr;;;MapValue of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];ReturnValue;value", + "com.jfinal.core;Controller;true;set" + ["", "Attr"] + + ";;;Argument[0];MapKey of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];value", + "com.jfinal.core;Controller;true;set" + ["", "Attr"] + + ";;;Argument[1];MapValue of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];value", + "com.jfinal.core;Controller;true;get" + ["Attr", "AttrForStr"] + + ";;;MapValue of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];ReturnValue;value" + ] + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected index 6a0454c38cfe..650c7198d9fe 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected @@ -1,20 +1,40 @@ edges -| FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | -| FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath | -| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath | -| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath | +| FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | +| FilePathInjection.java:41:21:41:34 | getPara(...) : String | FilePathInjection.java:44:25:44:37 | finalFilePath | +| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:65:31:65:38 | savePath : String | +| FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | +| FilePathInjection.java:65:31:65:38 | savePath : String | FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | +| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | +| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | +| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:88:24:88:31 | savePath : String | +| FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | +| FilePathInjection.java:88:24:88:31 | savePath : String | FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | +| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | +| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | +| FilePathInjection.java:158:17:158:44 | getParameter(...) : String | FilePathInjection.java:162:24:162:31 | filePath | nodes -| FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String | -| FilePathInjection.java:25:47:25:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:40:21:40:34 | getPara(...) : String | semmle.label | getPara(...) : String | -| FilePathInjection.java:43:25:43:37 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String | -| FilePathInjection.java:71:47:71:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| FilePathInjection.java:115:24:115:31 | filePath | semmle.label | filePath | +| FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String | +| FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:41:21:41:34 | getPara(...) : String | semmle.label | getPara(...) : String | +| FilePathInjection.java:44:25:44:37 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String | +| FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | +| FilePathInjection.java:65:31:65:38 | savePath : String | semmle.label | savePath : String | +| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String | +| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | semmle.label | this <.method> [com.jfinal.core.Controller.session, ] : String | +| FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String | +| FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | +| FilePathInjection.java:88:24:88:31 | savePath : String | semmle.label | savePath : String | +| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | semmle.label | getAttr(...) : String | +| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | semmle.label | this <.method> [com.jfinal.core.Controller.request, ] : String | +| FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath | +| FilePathInjection.java:158:17:158:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| FilePathInjection.java:162:24:162:31 | filePath | semmle.label | filePath | subpaths #select -| FilePathInjection.java:25:47:25:59 | finalFilePath | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:20:21:20:34 | getPara(...) | user-provided value | -| FilePathInjection.java:43:25:43:37 | finalFilePath | FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:40:21:40:34 | getPara(...) | user-provided value | -| FilePathInjection.java:71:47:71:59 | finalFilePath | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) | user-provided value | -| FilePathInjection.java:115:24:115:31 | filePath | FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:111:17:111:44 | getParameter(...) | user-provided value | +| FilePathInjection.java:26:47:26:59 | finalFilePath | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:21:21:21:34 | getPara(...) | user-provided value | +| FilePathInjection.java:44:25:44:37 | finalFilePath | FilePathInjection.java:41:21:41:34 | getPara(...) : String | FilePathInjection.java:44:25:44:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:41:21:41:34 | getPara(...) | user-provided value | +| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:64:21:64:34 | getPara(...) | user-provided value | +| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:87:21:87:34 | getPara(...) | user-provided value | +| FilePathInjection.java:162:24:162:31 | filePath | FilePathInjection.java:158:17:158:44 | getParameter(...) : String | FilePathInjection.java:162:24:162:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:158:17:158:44 | getParameter(...) | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java index f5f253abf38a..7b7b081ae69c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java @@ -9,6 +9,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; import com.jfinal.core.Controller; @@ -58,7 +59,7 @@ public void uploadFile2() throws IOException { } } - // BAD: Upload file to user specified path without validation + // BAD: Upload file to user specified path without validation through session attribute public void uploadFile3() throws IOException { String savePath = getPara("dir"); setSessionAttr("uploadDir", savePath); @@ -81,6 +82,52 @@ public void uploadFile3() throws IOException { fos.close(); } + // BAD: Upload file to user specified path without validation through request attribute + public void uploadFile4() throws IOException { + String savePath = getPara("dir"); + setAttr("uploadDir", savePath); + String requestUploadDir = getAttr("uploadDir"); + + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + requestUploadDir; + + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(finalFilePath); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + + // BAD: Upload file to user specified path without validation through session object (not detected) + public void uploadFile5() throws IOException { + String savePath = getPara("dir"); + getSession().setAttribute("uploadDir", savePath); + String sessionUploadDir = getSessionAttr("uploadDir"); + + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + sessionUploadDir; + + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(finalFilePath); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + private void readFile(HttpServletResponse resp, File file) { OutputStream os = null; FileInputStream fis = null; From ce03aeb4d9cff7d35ae394d46d5b5360265bfabc Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 9 Feb 2022 23:19:40 +0000 Subject: [PATCH 04/11] Fixed an issue related to normalized path --- .../Security/CWE/CWE-073/FilePathInjection.ql | 3 ++- .../Security/CWE/CWE-073/PathSanitizer.qll | 22 +++++++++++++++++++ .../CWE-073/FilePathInjection.expected | 12 ++++------ .../security/CWE-073/FilePathInjection.java | 2 ++ 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index 8349606f7202..3ad7fa6ca424 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -24,7 +24,8 @@ class InjectFilePathConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(PathCreation p).getAnInput() + sink.asExpr() = any(PathCreation p).getAnInput() and + not sink instanceof SanitizedNode } override predicate isSanitizer(DataFlow::Node node) { diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll index f5521ab9c6d3..32e6e3067062 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll @@ -173,3 +173,25 @@ private class UrlDecodeSanitizer extends MethodAccess { this.getMethod().hasName("decode") } } + +/** A sanitized node that is protected against path traversal vulnerabilities. */ +abstract class SanitizedNode extends DataFlow::Node { } + +class NodeWithPathNormalizer extends SanitizedNode { + NodeWithPathNormalizer() { + exists(MethodAccess ma | + DataFlow::localExprFlow(this.asExpr(), ma) and ma instanceof PathNormalizeSanitizer + ) + } +} + +/** Data model related to `java.nio.file.Path`. */ +private class PathDataModel extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + "java.nio.file;Paths;true;get;;;Argument[0];ReturnValue;value", + "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;value" + ] + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected index 650c7198d9fe..93314cc633dd 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected @@ -1,6 +1,5 @@ edges | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | -| FilePathInjection.java:41:21:41:34 | getPara(...) : String | FilePathInjection.java:44:25:44:37 | finalFilePath | | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:65:31:65:38 | savePath : String | | FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | | FilePathInjection.java:65:31:65:38 | savePath : String | FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | @@ -11,12 +10,10 @@ edges | FilePathInjection.java:88:24:88:31 | savePath : String | FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | | FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | -| FilePathInjection.java:158:17:158:44 | getParameter(...) : String | FilePathInjection.java:162:24:162:31 | filePath | +| FilePathInjection.java:159:17:159:44 | getParameter(...) : String | FilePathInjection.java:163:24:163:31 | filePath | nodes | FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:41:21:41:34 | getPara(...) : String | semmle.label | getPara(...) : String | -| FilePathInjection.java:44:25:44:37 | finalFilePath | semmle.label | finalFilePath | | FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | | FilePathInjection.java:65:31:65:38 | savePath : String | semmle.label | savePath : String | @@ -29,12 +26,11 @@ nodes | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | semmle.label | getAttr(...) : String | | FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | semmle.label | this <.method> [com.jfinal.core.Controller.request, ] : String | | FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:158:17:158:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| FilePathInjection.java:162:24:162:31 | filePath | semmle.label | filePath | +| FilePathInjection.java:159:17:159:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| FilePathInjection.java:163:24:163:31 | filePath | semmle.label | filePath | subpaths #select | FilePathInjection.java:26:47:26:59 | finalFilePath | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:21:21:21:34 | getPara(...) | user-provided value | -| FilePathInjection.java:44:25:44:37 | finalFilePath | FilePathInjection.java:41:21:41:34 | getPara(...) : String | FilePathInjection.java:44:25:44:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:41:21:41:34 | getPara(...) | user-provided value | | FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:64:21:64:34 | getPara(...) | user-provided value | | FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:87:21:87:34 | getPara(...) | user-provided value | -| FilePathInjection.java:162:24:162:31 | filePath | FilePathInjection.java:158:17:158:44 | getParameter(...) : String | FilePathInjection.java:162:24:162:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:158:17:158:44 | getParameter(...) | user-provided value | +| FilePathInjection.java:163:24:163:31 | filePath | FilePathInjection.java:159:17:159:44 | getParameter(...) : String | FilePathInjection.java:163:24:163:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:159:17:159:44 | getParameter(...) | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java index 7b7b081ae69c..dbe9430a19a7 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java @@ -153,6 +153,7 @@ private void readFile(HttpServletResponse resp, File file) { } } + // BAD: Download file to user specified path without validation public void downloadFile() throws FileNotFoundException, IOException { HttpServletRequest request = getRequest(); String path = request.getParameter("path"); @@ -173,6 +174,7 @@ public void downloadFile() throws FileNotFoundException, IOException { } } + // GOOD: Download file with path validation public void downloadFile2() throws FileNotFoundException, IOException { HttpServletRequest request = getRequest(); String path = request.getParameter("path"); From 12c53baba4a4e85ded2ca6f4fb7d6465550bf868 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 11 Feb 2022 01:05:06 +0000 Subject: [PATCH 05/11] Simplify the query --- .../src/experimental/Security/CWE/CWE-073/PathSanitizer.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll index 32e6e3067062..1020c7dc5d54 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll @@ -179,9 +179,7 @@ abstract class SanitizedNode extends DataFlow::Node { } class NodeWithPathNormalizer extends SanitizedNode { NodeWithPathNormalizer() { - exists(MethodAccess ma | - DataFlow::localExprFlow(this.asExpr(), ma) and ma instanceof PathNormalizeSanitizer - ) + DataFlow::localExprFlow(this.asExpr(), any(PathNormalizeSanitizer ma)) } } From e3d0e9f083c01b590309615a16fd23543cc84257 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 11 Feb 2022 12:38:05 +0000 Subject: [PATCH 06/11] Update normalized path node --- .../Security/CWE/CWE-073/FilePathInjection.ql | 2 +- .../Security/CWE/CWE-073/PathSanitizer.qll | 14 ++++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index 3ad7fa6ca424..5d5349fb1ace 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -25,7 +25,7 @@ class InjectFilePathConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(PathCreation p).getAnInput() and - not sink instanceof SanitizedNode + not sink instanceof NormalizedPathNode } override predicate isSanitizer(DataFlow::Node node) { diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll index 1020c7dc5d54..9e76410a6ffb 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll @@ -174,12 +174,10 @@ private class UrlDecodeSanitizer extends MethodAccess { } } -/** A sanitized node that is protected against path traversal vulnerabilities. */ -abstract class SanitizedNode extends DataFlow::Node { } - -class NodeWithPathNormalizer extends SanitizedNode { - NodeWithPathNormalizer() { - DataFlow::localExprFlow(this.asExpr(), any(PathNormalizeSanitizer ma)) +/** A node with path normalization. */ +class NormalizedPathNode extends DataFlow::Node { + NormalizedPathNode() { + TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma)) } } @@ -188,8 +186,8 @@ private class PathDataModel extends SummaryModelCsv { override predicate row(string row) { row = [ - "java.nio.file;Paths;true;get;;;Argument[0];ReturnValue;value", - "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;value" + "java.nio.file;Paths;true;get;;;Argument[0];ReturnValue;taint", + "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint" ] } } From 78630f25dd8cfed0ccdca1c4afb49fb6fb0b74b8 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 11 Feb 2022 23:53:31 +0000 Subject: [PATCH 07/11] Match attribute name to reduce FP --- .../Security/CWE/CWE-073/JFinalController.qll | 46 +++++++++++-------- .../CWE-073/FilePathInjection.expected | 28 +++-------- .../security/CWE-073/FilePathInjection.java | 46 +++++++++++++++++++ 3 files changed, 80 insertions(+), 40 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll index 49123859dfe8..224257091bd7 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -38,6 +38,34 @@ class SetRequestAttributeMethod extends Method { } } +/** + * Holds if the result of an attribute getter call is from a method invocation of remote attribute setter. + * Only values received from remote flow source is to be checked by the query. + */ +predicate isGetAttributeFromRemoteSource(Expr expr) { + exists(MethodAccess gma, MethodAccess sma | + ( + gma.getMethod() instanceof GetSessionAttributeMethod and + sma.getMethod() instanceof SetSessionAttributeMethod + or + gma.getMethod() instanceof GetRequestAttributeMethod and + sma.getMethod() instanceof SetRequestAttributeMethod + ) and + expr = gma and + gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = + sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and + gma.getEnclosingCallable() = sma.getEnclosingCallable() and + TaintTracking::localExprTaint(any(RemoteFlowSource rs).asExpr(), sma.getArgument(1)) + ) +} + +/** Remote flow source of JFinal request or session attribute getters. */ +private class JFinalRequestSource extends RemoteFlowSource { + JFinalRequestSource() { isGetAttributeFromRemoteSource(this.asExpr()) } + + override string getSourceType() { result = "JFinal session or request attribute source" } +} + /** Source model of remote flow source with `JFinal`. */ private class JFinalControllerSource extends SourceModelCsv { override predicate row(string row) { @@ -58,21 +86,3 @@ private class JFinalControllerSource extends SourceModelCsv { ] } } - -/** `JFinal` data model related to session and request attribute operations. */ -private class JFinalDataModel extends SummaryModelCsv { - override predicate row(string row) { - row = - [ - "com.jfinal.core;Controller;true;setSessionAttr;;;Argument[0];MapKey of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];value", - "com.jfinal.core;Controller;true;setSessionAttr;;;Argument[1];MapValue of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];value", - "com.jfinal.core;Controller;true;getSessionAttr;;;MapValue of SyntheticField[com.jfinal.core.Controller.session] of Argument[-1];ReturnValue;value", - "com.jfinal.core;Controller;true;set" + ["", "Attr"] + - ";;;Argument[0];MapKey of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];value", - "com.jfinal.core;Controller;true;set" + ["", "Attr"] + - ";;;Argument[1];MapValue of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];value", - "com.jfinal.core;Controller;true;get" + ["Attr", "AttrForStr"] + - ";;;MapValue of SyntheticField[com.jfinal.core.Controller.request] of Argument[-1];ReturnValue;value" - ] - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected index 93314cc633dd..80f4cb4915c4 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected @@ -1,36 +1,20 @@ edges | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | -| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:65:31:65:38 | savePath : String | -| FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | -| FilePathInjection.java:65:31:65:38 | savePath : String | FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | -| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | -| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:88:24:88:31 | savePath : String | -| FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | -| FilePathInjection.java:88:24:88:31 | savePath : String | FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | -| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | -| FilePathInjection.java:159:17:159:44 | getParameter(...) : String | FilePathInjection.java:163:24:163:31 | filePath | +| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath | nodes | FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String | -| FilePathInjection.java:65:3:65:39 | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.session, ] : String | -| FilePathInjection.java:65:31:65:38 | savePath : String | semmle.label | savePath : String | | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String | -| FilePathInjection.java:66:29:66:55 | this <.method> [com.jfinal.core.Controller.session, ] : String | semmle.label | this <.method> [com.jfinal.core.Controller.session, ] : String | | FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String | -| FilePathInjection.java:88:3:88:32 | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | semmle.label | this <.method> [post update] [com.jfinal.core.Controller.request, ] : String | -| FilePathInjection.java:88:24:88:31 | savePath : String | semmle.label | savePath : String | | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | semmle.label | getAttr(...) : String | -| FilePathInjection.java:89:29:89:48 | this <.method> [com.jfinal.core.Controller.request, ] : String | semmle.label | this <.method> [com.jfinal.core.Controller.request, ] : String | | FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:159:17:159:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| FilePathInjection.java:163:24:163:31 | filePath | semmle.label | filePath | +| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| FilePathInjection.java:209:24:209:31 | filePath | semmle.label | filePath | subpaths #select | FilePathInjection.java:26:47:26:59 | finalFilePath | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:21:21:21:34 | getPara(...) | user-provided value | -| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:64:21:64:34 | getPara(...) | user-provided value | -| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:87:21:87:34 | getPara(...) | user-provided value | -| FilePathInjection.java:163:24:163:31 | filePath | FilePathInjection.java:159:17:159:44 | getParameter(...) : String | FilePathInjection.java:163:24:163:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:159:17:159:44 | getParameter(...) | user-provided value | +| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) | user-provided value | +| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:89:29:89:48 | getAttr(...) | user-provided value | +| FilePathInjection.java:209:24:209:31 | filePath | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:205:17:205:44 | getParameter(...) | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java index dbe9430a19a7..8b751a705bb3 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java @@ -128,6 +128,52 @@ public void uploadFile5() throws IOException { fos.close(); } + // GOOD: Upload file to a system path from a request object + public void uploadFile6() throws IOException { + setAttr("uploadDir", "/data/upload_dir/"); + String requestUploadDir = getAttr("uploadDir"); + + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + requestUploadDir; + + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(finalFilePath); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + + // GOOD: Upload file to a system path from a request object + public void uploadFile7() throws IOException { + String savePath = getPara("dir"); + setAttr("uploadDir", savePath); + setAttr("realUploadDir", "/data/upload_dir/"); + String requestUploadDir = getAttr("realUploadDir"); + + File file = getFile("fileParam").getFile(); + String finalFilePath = BASE_PATH + requestUploadDir; + + FileInputStream fis = new FileInputStream(file); + FileOutputStream fos = new FileOutputStream(finalFilePath); + int i = 0; + + do { + byte[] buf = new byte[1024]; + i = fis.read(buf); + fos.write(buf); + } while (i != -1); + + fis.close(); + fos.close(); + } + private void readFile(HttpServletResponse resp, File file) { OutputStream os = null; FileInputStream fis = null; From 35a924292bf739f248b34bf2fb7634fb6996c856 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Mon, 14 Feb 2022 14:08:55 +0000 Subject: [PATCH 08/11] Model value passing between a setter and a getter call as a value step --- .../Security/CWE/CWE-073/FilePathInjection.ql | 4 ++ .../Security/CWE/CWE-073/JFinalController.qll | 45 ++++++++----------- .../CWE-073/FilePathInjection.expected | 12 ++--- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index 5d5349fb1ace..e6b4c3ae22ed 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -28,6 +28,10 @@ class InjectFilePathConfig extends TaintTracking::Configuration { not sink instanceof NormalizedPathNode } + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + any(AdditionalValueStep r).step(pred, succ) + } + override predicate isSanitizer(DataFlow::Node node) { exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType) } diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll index 224257091bd7..05e9b98dcc8b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -38,32 +38,25 @@ class SetRequestAttributeMethod extends Method { } } -/** - * Holds if the result of an attribute getter call is from a method invocation of remote attribute setter. - * Only values received from remote flow source is to be checked by the query. - */ -predicate isGetAttributeFromRemoteSource(Expr expr) { - exists(MethodAccess gma, MethodAccess sma | - ( - gma.getMethod() instanceof GetSessionAttributeMethod and - sma.getMethod() instanceof SetSessionAttributeMethod - or - gma.getMethod() instanceof GetRequestAttributeMethod and - sma.getMethod() instanceof SetRequestAttributeMethod - ) and - expr = gma and - gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = - sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and - gma.getEnclosingCallable() = sma.getEnclosingCallable() and - TaintTracking::localExprTaint(any(RemoteFlowSource rs).asExpr(), sma.getArgument(1)) - ) -} - -/** Remote flow source of JFinal request or session attribute getters. */ -private class JFinalRequestSource extends RemoteFlowSource { - JFinalRequestSource() { isGetAttributeFromRemoteSource(this.asExpr()) } - - override string getSourceType() { result = "JFinal session or request attribute source" } +/** Value step from the setter call to the getter call of a session or request attribute. */ +private class SetToGetAttributeStep extends AdditionalValueStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(MethodAccess gma, MethodAccess sma | + ( + gma.getMethod() instanceof GetSessionAttributeMethod and + sma.getMethod() instanceof SetSessionAttributeMethod + or + gma.getMethod() instanceof GetRequestAttributeMethod and + sma.getMethod() instanceof SetRequestAttributeMethod + ) and + gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = + sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and + gma.getEnclosingCallable() = sma.getEnclosingCallable() + | + pred.asExpr() = sma.getArgument(1) and + succ.asExpr() = gma + ) + } } /** Source model of remote flow source with `JFinal`. */ diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected index 80f4cb4915c4..5720de5c4b98 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected @@ -1,20 +1,20 @@ edges | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | -| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | -| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | +| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | +| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath | nodes | FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String | +| FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath | -| FilePathInjection.java:89:29:89:48 | getAttr(...) : String | semmle.label | getAttr(...) : String | +| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String | | FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath | | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | | FilePathInjection.java:209:24:209:31 | filePath | semmle.label | filePath | subpaths #select | FilePathInjection.java:26:47:26:59 | finalFilePath | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:21:21:21:34 | getPara(...) | user-provided value | -| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:66:29:66:55 | getSessionAttr(...) | user-provided value | -| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:89:29:89:48 | getAttr(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:89:29:89:48 | getAttr(...) | user-provided value | +| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:64:21:64:34 | getPara(...) | user-provided value | +| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:87:21:87:34 | getPara(...) | user-provided value | | FilePathInjection.java:209:24:209:31 | filePath | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:205:17:205:44 | getParameter(...) | user-provided value | From 2b5982fd9d777ac367797f06999a8cd3d33b847b Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Mon, 14 Feb 2022 15:42:54 +0000 Subject: [PATCH 09/11] Remove specified value step from additional taint step --- .../experimental/Security/CWE/CWE-073/FilePathInjection.ql | 4 ---- 1 file changed, 4 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index e6b4c3ae22ed..5d5349fb1ace 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -28,10 +28,6 @@ class InjectFilePathConfig extends TaintTracking::Configuration { not sink instanceof NormalizedPathNode } - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - any(AdditionalValueStep r).step(pred, succ) - } - override predicate isSanitizer(DataFlow::Node node) { exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType) } From fd533f2ba88eef6e93c6898d564a1ace66494cf7 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 15 Feb 2022 12:44:23 +0000 Subject: [PATCH 10/11] Remove the same callable constraint --- .../Security/CWE/CWE-073/JFinalController.qll | 3 +-- .../security/CWE-073/FilePathInjection.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll index 05e9b98dcc8b..73e7379ce4d2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -50,8 +50,7 @@ private class SetToGetAttributeStep extends AdditionalValueStep { sma.getMethod() instanceof SetRequestAttributeMethod ) and gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = - sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and - gma.getEnclosingCallable() = sma.getEnclosingCallable() + sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() | pred.asExpr() = sma.getArgument(1) and succ.asExpr() = gma diff --git a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java index 8b751a705bb3..2534386a2106 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java @@ -85,8 +85,8 @@ public void uploadFile3() throws IOException { // BAD: Upload file to user specified path without validation through request attribute public void uploadFile4() throws IOException { String savePath = getPara("dir"); - setAttr("uploadDir", savePath); - String requestUploadDir = getAttr("uploadDir"); + setAttr("uploadDir2", savePath); + String requestUploadDir = getAttr("uploadDir2"); File file = getFile("fileParam").getFile(); String finalFilePath = BASE_PATH + requestUploadDir; @@ -108,8 +108,8 @@ public void uploadFile4() throws IOException { // BAD: Upload file to user specified path without validation through session object (not detected) public void uploadFile5() throws IOException { String savePath = getPara("dir"); - getSession().setAttribute("uploadDir", savePath); - String sessionUploadDir = getSessionAttr("uploadDir"); + getSession().setAttribute("uploadDir3", savePath); + String sessionUploadDir = getSessionAttr("uploadDir3"); File file = getFile("fileParam").getFile(); String finalFilePath = BASE_PATH + sessionUploadDir; @@ -130,8 +130,8 @@ public void uploadFile5() throws IOException { // GOOD: Upload file to a system path from a request object public void uploadFile6() throws IOException { - setAttr("uploadDir", "/data/upload_dir/"); - String requestUploadDir = getAttr("uploadDir"); + setAttr("uploadDir4", "/data/upload_dir/"); + String requestUploadDir = getAttr("uploadDir4"); File file = getFile("fileParam").getFile(); String finalFilePath = BASE_PATH + requestUploadDir; @@ -153,9 +153,9 @@ public void uploadFile6() throws IOException { // GOOD: Upload file to a system path from a request object public void uploadFile7() throws IOException { String savePath = getPara("dir"); - setAttr("uploadDir", savePath); + setAttr("uploadDir5", savePath); setAttr("realUploadDir", "/data/upload_dir/"); - String requestUploadDir = getAttr("realUploadDir"); + String requestUploadDir = getAttr("realUploadDir5"); File file = getFile("fileParam").getFile(); String finalFilePath = BASE_PATH + requestUploadDir; From 40bf093d34e7538b083bdd483bfff4ea8fde7f8c Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 15 Feb 2022 17:28:13 +0000 Subject: [PATCH 11/11] Move shared code to the lib folder and update qldoc --- .../Security/CWE/CWE-073/FilePathInjection.qhelp | 6 +++--- .../Security/CWE/CWE-073/FilePathInjection.ql | 2 +- .../Security/CWE/CWE-073/JFinalController.qll | 11 +++++++---- .../CWE-073 => semmle/code/java}/PathSanitizer.qll | 0 4 files changed, 11 insertions(+), 8 deletions(-) rename java/ql/src/experimental/{Security/CWE/CWE-073 => semmle/code/java}/PathSanitizer.qll (100%) diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp index 040bf906b1bd..0797fb366ffc 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp @@ -6,12 +6,12 @@

    External Control of File Name or Path, also called File Path Injection, is a vulnerability by which -a file path is created 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.

    +a file path is created using data from outside the application (such as the HTTP request). It allows +an attacker to traverse through the filesystem and access arbitrary files.

     -

    Unsanitized user-provided data must not be used to construct the file path. In order to prevent File +

    Unsanitized user-provided data must not be used to construct file paths. 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 or disallowed paths (for example, the path must be within /user_content/ or must not be within /internal), ensuring that neither path diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index 5d5349fb1ace..bf4f1ec33bb9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -15,7 +15,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.PathCreation import JFinalController -import PathSanitizer +import experimental.semmle.code.java.PathSanitizer import DataFlow::PathGraph class InjectFilePathConfig extends TaintTracking::Configuration { diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll index 73e7379ce4d2..264e1e31ed26 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll @@ -22,7 +22,7 @@ class SetSessionAttributeMethod extends Method { } } -/** The request attribute getter method of `JFinalController`. */ +/** A request attribute getter method of `JFinalController`. */ class GetRequestAttributeMethod extends Method { GetRequestAttributeMethod() { this.getName().matches("getAttr%") and @@ -30,7 +30,7 @@ class GetRequestAttributeMethod extends Method { } } -/** The request attribute setter method of `JFinalController`. */ +/** A request attribute setter method of `JFinalController`. */ class SetRequestAttributeMethod extends Method { SetRequestAttributeMethod() { this.getName() = ["set", "setAttr"] and @@ -38,7 +38,10 @@ class SetRequestAttributeMethod extends Method { } } -/** Value step from the setter call to the getter call of a session or request attribute. */ +/** + * Value step from a setter call to a corresponding getter call relating to a + * session or request attribute. + */ private class SetToGetAttributeStep extends AdditionalValueStep { override predicate step(DataFlow::Node pred, DataFlow::Node succ) { exists(MethodAccess gma, MethodAccess sma | @@ -58,7 +61,7 @@ private class SetToGetAttributeStep extends AdditionalValueStep { } } -/** Source model of remote flow source with `JFinal`. */ +/** Remote flow source models relating to `JFinal`. */ private class JFinalControllerSource extends SourceModelCsv { override predicate row(string row) { row = diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll b/java/ql/src/experimental/semmle/code/java/PathSanitizer.qll similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-073/PathSanitizer.qll rename to java/ql/src/experimental/semmle/code/java/PathSanitizer.qll