hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-25 16:55:19 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #13225 from atorralba/atorralba/java/path-injection-mad-sinks-2

Browse Source 
Java: Migrate path injection sinks to models-as-data (simplified)
This commit is contained in:
Tony Torralba
2023-06-07 14:27:36 +02:00
committed by GitHub
parent 911835c30e 27763d6bbe
commit 6d7234f8ed
10 changed files with 58 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/ql/lib/change-notes/2023-05-19-path-injection-sinks-mad.md Normal file
View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Path creation sinks modeled in `PathCreation.qll` have been added to the models-as-data sink kind `path-injection`.

5
java/ql/lib/ext/java.io.model.yml
View File

@@ -3,6 +3,10 @@ extensions:
pack: codeql/java-all
extensible: sinkModel
data:
- ["java.io", "File", False, "File", "(File,String)", "", "Argument[1]", "path-injection", "manual"] # old PathCreation
- ["java.io", "File", False, "File", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
- ["java.io", "File", False, "File", "(String,String)", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
- ["java.io", "File", False, "File", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
- ["java.io", "File", True, "createTempFile", "(String,String,File)", "", "Argument[2]", "path-injection", "ai-manual"]
- ["java.io", "File", True, "renameTo", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.io", "FileInputStream", True, "FileInputStream", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
@@ -11,6 +15,7 @@ extensions:
- ["java.io", "FileOutputStream", False, "write", "", "", "Argument[0]", "file-content-store", "manual"]
- ["java.io", "FileReader", True, "FileReader", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.io", "FileReader", True, "FileReader", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.io", "FileReader", True, "FileReader", "(String,Charset)", "", "Argument[0]", "path-injection", "manual"]
- ["java.io", "FileSystem", True, "createDirectory", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.io", "FileWriter", False, "FileWriter", "", "", "Argument[0]", "path-injection", "manual"]
- ["java.io", "PrintStream", False, "PrintStream", "(File)", "", "Argument[0]", "path-injection", "manual"]

7
java/ql/lib/ext/java.nio.file.model.yml
View File

@@ -42,6 +42,13 @@ extensions:
- ["java.nio.file", "Files", True, "delete", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", True, "newInputStream", "(Path,OpenOption[])", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", True, "newOutputStream", "(Path,OpenOption[])", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "FileSystem", False, "getPath", "", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "Path", False, "of", "(String,String[])", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "Path", False, "of", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "Path", False, "resolve", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "Path", False, "resolveSibling", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "Paths", False, "get", "(String,String[])", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "Paths", False, "get", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "SecureDirectoryStream", True, "deleteDirectory", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "SecureDirectoryStream", True, "deleteFile", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- addsTo:

13
java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll
View File

@@ -5,7 +5,6 @@ import semmle.code.java.frameworks.Networking
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.FlowSources
private import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.security.PathCreation
import semmle.code.java.security.PathSanitizer
/**
@@ -55,11 +54,7 @@ private class TaintPreservingUriCtorParam extends Parameter {
module TaintedPathConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
predicate isSink(DataFlow::Node sink) {
sink.asExpr() = any(PathCreation p).getAnInput()
or
sinkNode(sink, "path-injection")
}
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
predicate isBarrier(DataFlow::Node sanitizer) {
sanitizer.getType() instanceof BoxedType or
@@ -82,11 +77,7 @@ module TaintedPathFlow = TaintTracking::Global<TaintedPathConfig>;
module TaintedPathLocalConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
predicate isSink(DataFlow::Node sink) {
sink.asExpr() = any(PathCreation p).getAnInput()
or
sinkNode(sink, "path-injection")
}
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
predicate isBarrier(DataFlow::Node sanitizer) {
sanitizer.getType() instanceof BoxedType or

26
java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
View File

@@ -4,6 +4,7 @@ import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.security.PathSanitizer
private import semmle.code.java.dataflow.ExternalFlow
private import semmle.code.java.security.PathCreation
/**
* A method that returns the name of an archive entry.
@@ -40,5 +41,28 @@ module ZipSlipFlow = TaintTracking::Global<ZipSlipConfig>;
* A sink that represents a file creation, such as a file write, copy or move operation.
*/
private class FileCreationSink extends DataFlow::Node {
FileCreationSink() { sinkNode(this, "path-injection") }
FileCreationSink() {
sinkNode(this, "path-injection") and
not isPathCreation(this)
}
}
/**
* Holds if `sink` is a path creation node that doesn't imply a read/write filesystem operation.
* This is to avoid creating new spurious alerts, since `PathCreation` sinks weren't
* previously part of this query.
*/
private predicate isPathCreation(DataFlow::Node sink) {
exists(PathCreation pc |
pc.getAnInput() = sink.asExpr()
or
pc.getAnInput().(Argument).isVararg() and sink.(DataFlow::ImplicitVarargsArray).getCall() = pc
|
// exclude actual read/write operations included in `PathCreation`
not pc.(Call)
.getCallee()
.getDeclaringType()
.hasQualifiedName("java.io",
["FileInputStream", "FileOutputStream", "FileReader", "FileWriter"])
)
}

1
java/ql/src/Security/CWE/CWE-022/TaintedPath.ql
View File

@@ -14,6 +14,7 @@
*/
import java
import semmle.code.java.security.PathCreation
import semmle.code.java.security.TaintedPathQuery
import TaintedPathFlow::PathGraph

1
java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql
View File

@@ -14,6 +14,7 @@
*/
import java
import semmle.code.java.security.PathCreation
import semmle.code.java.security.TaintedPathQuery
import TaintedPathLocalFlow::PathGraph

3
java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql
View File

@@ -16,7 +16,6 @@ import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.security.PathCreation
import JFinalController
import semmle.code.java.security.PathSanitizer
import InjectFilePathFlow::PathGraph
@@ -52,7 +51,7 @@ module InjectFilePathConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
predicate isSink(DataFlow::Node sink) {
sink.asExpr() = any(PathCreation p).getAnInput() and
sinkNode(sink, "path-injection") and
not sink instanceof NormalizedPathNode
}

11
java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected
View File

@@ -2,7 +2,12 @@ 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:72:47:72:59 | finalFilePath |
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath |
| FilePathInjection.java:177:50:177:58 | file : File | FilePathInjection.java:182:30:182:33 | file |
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath |
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath : String |
| FilePathInjection.java:209:15:209:32 | new File(...) : File | FilePathInjection.java:217:19:217:22 | file : File |
| FilePathInjection.java:209:24:209:31 | filePath : String | FilePathInjection.java:209:15:209:32 | new File(...) : File |
| FilePathInjection.java:217:19:217:22 | file : File | FilePathInjection.java:177:50:177:58 | file : File |
nodes
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String |
| FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath |
@@ -10,11 +15,17 @@ nodes
| FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath |
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String |
| FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath |
| FilePathInjection.java:177:50:177:58 | file : File | semmle.label | file : File |
| FilePathInjection.java:182:30:182:33 | file | semmle.label | file |
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| FilePathInjection.java:209:15:209:32 | new File(...) : File | semmle.label | new File(...) : File |
| FilePathInjection.java:209:24:209:31 | filePath | semmle.label | filePath |
| FilePathInjection.java:209:24:209:31 | filePath : String | semmle.label | filePath : String |
| FilePathInjection.java:217:19:217:22 | file : File | semmle.label | file : File |
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:182:30:182:33 | file | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:182:30:182:33 | file | External control of file name or path due to $@. | FilePathInjection.java:205:17:205:44 | getParameter(...) | 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 |

1
java/ql/test/query-tests/Telemetry/SupportedExternalSinks/SupportedExternalSinks.expected
View File

@@ -1,2 +1,3 @@
| java.io.File#File(String) | 1 |
| java.io.FileWriter#FileWriter(File) | 1 |
| java.net.URL#openStream() | 1 |