Skip to content

Commit

Permalink
Merge pull request #12886 from atorralba/atorralba/java/path-injectio…
Browse files Browse the repository at this point in the history
…n-mad-sinks

Java: Refactor path injection sinks
  • Loading branch information
atorralba authored Feb 9, 2024
2 parents d46028f + e2bf9ea commit 4c0d535
Show file tree
Hide file tree
Showing 21 changed files with 232 additions and 940 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: deprecated
---
* The `PathCreation` class in `PathCreation.qll` has been deprecated.
9 changes: 4 additions & 5 deletions java/ql/lib/ext/java.io.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@ 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, "createNewFile", "()", "", "Argument[this]", "path-injection", "ai-manual"]
- ["java.io", "File", True, "createTempFile", "(String,String,File)", "", "Argument[2]", "path-injection", "ai-manual"]
- ["java.io", "File", True, "exists", "()", "", "Argument[this]", "path-injection", "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"]
- ["java.io", "FileInputStream", True, "FileInputStream", "(FileDescriptor)", "", "Argument[0]", "path-injection", "manual"]
- ["java.io", "FileInputStream", True, "FileInputStream", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.io", "FileOutputStream", False, "FileOutputStream", "", "", "Argument[0]", "path-injection", "manual"]
- ["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", "(FileDescriptor)", "", "Argument[0]", "path-injection", "manual"]
- ["java.io", "FileReader", True, "FileReader", "(File,Charset)", "", "Argument[0]", "path-injection", "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"]
Expand Down Expand Up @@ -127,7 +127,6 @@ extensions:
- ["java.io", "DataOutput", "writeLong", "(long)", "summary", "manual"] # taint-numeric
# sink neutrals
- ["java.io", "File", "compareTo", "", "sink", "hq-manual"]
- ["java.io", "File", "exists", "()", "sink", "hq-manual"]
- addsTo:
pack: codeql/java-all
extensible: sourceModel
Expand Down
16 changes: 5 additions & 11 deletions java/ql/lib/ext/java.nio.file.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ extensions:
- ["java.nio.file", "Files", False, "delete", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", False, "deleteIfExists", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", False, "getFileStore", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"] # the FileStore class is unlikely to be used for later sanitization
- ["java.nio.file", "Files", False, "exists", "(Path,LinkOption[])", "", "Argument[0]", "path-injection", "manual"]
- ["java.nio.file", "Files", False, "lines", "(Path,Charset)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", False, "lines", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", False, "move", "", "", "Argument[1]", "path-injection", "manual"]
Expand All @@ -27,6 +28,7 @@ extensions:
- ["java.nio.file", "Files", False, "newBufferedWriter", "", "", "Argument[0]", "path-injection", "manual"]
- ["java.nio.file", "Files", False, "newInputStream", "(Path,OpenOption[])", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", False, "newOutputStream", "", "", "Argument[0]", "path-injection", "manual"]
- ["java.nio.file", "Files", False, "notExists", "(Path,LinkOption[])", "", "Argument[0]", "path-injection", "manual"]
- ["java.nio.file", "Files", False, "probeContentType", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"] # accesses the file based on user input, but only reads its content type from it
- ["java.nio.file", "Files", False, "readAllBytes", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "Files", False, "readAllLines", "(Path,Charset)", "", "Argument[0]", "path-injection", "ai-manual"]
Expand All @@ -37,15 +39,8 @@ extensions:
- ["java.nio.file", "Files", False, "write", "", "", "Argument[1]", "file-content-store", "manual"]
- ["java.nio.file", "Files", False, "writeString", "", "", "Argument[0]", "path-injection", "manual"]
- ["java.nio.file", "Files", False, "writeString", "", "", "Argument[1]", "file-content-store", "manual"]
- ["java.nio.file", "FileSystem", False, "getPath", "", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
- ["java.nio.file", "FileSystems", False, "newFileSystem", "(URI,Map)", "", "Argument[0]", "path-injection", "ai-manual"]
- ["java.nio.file", "FileSystems", False, "newFileSystem", "(URI,Map)", "", "Argument[0]", "request-forgery", "ai-manual"]
- ["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:
Expand All @@ -63,7 +58,7 @@ extensions:
- ["java.nio.file", "Files", True, "newDirectoryStream", "(Path,DirectoryStream$Filter)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "Files", True, "newDirectoryStream", "(Path)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "Files", True, "walk", "(Path,FileVisitOption[])", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "FileSystem", True, "getPath", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "FileSystem", True, "getPath", "(String,String[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "FileSystem", True, "getPath", "(String,String[])", "", "Argument[1]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "FileSystem", True, "getPathMatcher", "(String)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "FileSystem", True, "getRootDirectories", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
Expand All @@ -76,7 +71,8 @@ extensions:
- ["java.nio.file", "Path", True, "relativize", "(Path)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "Path", True, "resolve", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "Path", True, "resolve", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "Path", True, "resolveSibling", "(String)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "Path", True, "resolveSibling", "", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
- ["java.nio.file", "Path", True, "resolveSibling", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "Path", True, "toAbsolutePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "Path", False, "toFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.nio.file", "Path", True, "toString", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
Expand All @@ -95,7 +91,6 @@ extensions:
# summary neutrals
- ["java.nio.file", "Files", "exists", "(Path,LinkOption[])", "summary", "manual"]
# sink neutrals
- ["java.nio.file", "Files", "exists", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "getLastModifiedTime", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "getOwner", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "getPosixFilePermissions", "", "sink", "hq-manual"]
Expand All @@ -107,6 +102,5 @@ extensions:
- ["java.nio.file", "Files", "isSameFile", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "isSymbolicLink", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "isWritable", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "notExists", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "setLastModifiedTime", "", "sink", "hq-manual"]
- ["java.nio.file", "Files", "size", "", "sink", "hq-manual"]
26 changes: 14 additions & 12 deletions java/ql/lib/semmle/code/java/security/PathCreation.qll
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
/**
* DEPRECATED.
*
* Models the different ways to create paths. Either by using `java.io.File`-related APIs or `java.nio.file.Path`-related APIs.
*/

import java

/** Models the creation of a path. */
abstract class PathCreation extends Expr {
/** DEPRECATED: Models the creation of a path. */
abstract deprecated class PathCreation extends Expr {
/**
* Gets an input that is used in the creation of this path.
* This excludes inputs of type `File` and `Path`.
Expand All @@ -14,7 +16,7 @@ abstract class PathCreation extends Expr {
}

/** Models the `java.nio.file.Paths.get` method. */
private class PathsGet extends PathCreation, MethodCall {
deprecated private class PathsGet extends PathCreation, MethodCall {
PathsGet() {
exists(Method m | m = this.getMethod() |
m.getDeclaringType() instanceof TypePaths and
Expand All @@ -26,7 +28,7 @@ private class PathsGet extends PathCreation, MethodCall {
}

/** Models the `java.nio.file.FileSystem.getPath` method. */
private class FileSystemGetPath extends PathCreation, MethodCall {
deprecated private class FileSystemGetPath extends PathCreation, MethodCall {
FileSystemGetPath() {
exists(Method m | m = this.getMethod() |
m.getDeclaringType() instanceof TypeFileSystem and
Expand All @@ -38,7 +40,7 @@ private class FileSystemGetPath extends PathCreation, MethodCall {
}

/** Models the `new java.io.File(...)` constructor. */
private class FileCreation extends PathCreation, ClassInstanceExpr {
deprecated private class FileCreation extends PathCreation, ClassInstanceExpr {
FileCreation() { this.getConstructedType() instanceof TypeFile }

override Expr getAnInput() {
Expand All @@ -49,7 +51,7 @@ private class FileCreation extends PathCreation, ClassInstanceExpr {
}

/** Models the `java.nio.file.Path.resolveSibling` method. */
private class PathResolveSiblingCreation extends PathCreation, MethodCall {
deprecated private class PathResolveSiblingCreation extends PathCreation, MethodCall {
PathResolveSiblingCreation() {
exists(Method m | m = this.getMethod() |
m.getDeclaringType() instanceof TypePath and
Expand All @@ -65,7 +67,7 @@ private class PathResolveSiblingCreation extends PathCreation, MethodCall {
}

/** Models the `java.nio.file.Path.resolve` method. */
private class PathResolveCreation extends PathCreation, MethodCall {
deprecated private class PathResolveCreation extends PathCreation, MethodCall {
PathResolveCreation() {
exists(Method m | m = this.getMethod() |
m.getDeclaringType() instanceof TypePath and
Expand All @@ -81,7 +83,7 @@ private class PathResolveCreation extends PathCreation, MethodCall {
}

/** Models the `java.nio.file.Path.of` method. */
private class PathOfCreation extends PathCreation, MethodCall {
deprecated private class PathOfCreation extends PathCreation, MethodCall {
PathOfCreation() {
exists(Method m | m = this.getMethod() |
m.getDeclaringType() instanceof TypePath and
Expand All @@ -93,7 +95,7 @@ private class PathOfCreation extends PathCreation, MethodCall {
}

/** Models the `new java.io.FileWriter(...)` constructor. */
private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
deprecated private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
FileWriterCreation() { this.getConstructedType().hasQualifiedName("java.io", "FileWriter") }

override Expr getAnInput() {
Expand All @@ -104,7 +106,7 @@ private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
}

/** Models the `new java.io.FileReader(...)` constructor. */
private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
deprecated private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
FileReaderCreation() { this.getConstructedType().hasQualifiedName("java.io", "FileReader") }

override Expr getAnInput() {
Expand All @@ -115,7 +117,7 @@ private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
}

/** Models the `new java.io.FileInputStream(...)` constructor. */
private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
deprecated private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
FileInputStreamCreation() {
this.getConstructedType().hasQualifiedName("java.io", "FileInputStream")
}
Expand All @@ -128,7 +130,7 @@ private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
}

/** Models the `new java.io.FileOutputStream(...)` constructor. */
private class FileOutputStreamCreation extends PathCreation, ClassInstanceExpr {
deprecated private class FileOutputStreamCreation extends PathCreation, ClassInstanceExpr {
FileOutputStreamCreation() {
this.getConstructedType().hasQualifiedName("java.io", "FileOutputStream")
}
Expand Down
11 changes: 9 additions & 2 deletions java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ private import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.security.PathSanitizer
private import semmle.code.java.security.Sanitizers

/** A sink for tainted path flow configurations. */
abstract class TaintedPathSink extends DataFlow::Node { }

private class DefaultTaintedPathSink extends TaintedPathSink {
DefaultTaintedPathSink() { sinkNode(this, "path-injection") }
}

/**
* A unit class for adding additional taint steps.
*
Expand Down Expand Up @@ -55,7 +62,7 @@ private class TaintPreservingUriCtorParam extends Parameter {
module TaintedPathConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }

predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
predicate isSink(DataFlow::Node sink) { sink instanceof TaintedPathSink }

predicate isBarrier(DataFlow::Node sanitizer) {
sanitizer instanceof SimpleTypeSanitizer or
Expand All @@ -76,7 +83,7 @@ module TaintedPathFlow = TaintTracking::Global<TaintedPathConfig>;
module TaintedPathLocalConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }

predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
predicate isSink(DataFlow::Node sink) { sink instanceof TaintedPathSink }

predicate isBarrier(DataFlow::Node sanitizer) {
sanitizer instanceof SimpleTypeSanitizer or
Expand Down
25 changes: 1 addition & 24 deletions java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,5 @@ 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") 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"])
)
FileCreationSink() { sinkNode(this, "path-injection") }
}
18 changes: 2 additions & 16 deletions java/ql/src/Security/CWE/CWE-022/TaintedPath.ql
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,7 @@ import semmle.code.java.security.PathCreation
import semmle.code.java.security.TaintedPathQuery
import TaintedPathFlow::PathGraph

/**
* Gets the data-flow node at which to report a path ending at `sink`.
*
* Previously this query flagged alerts exclusively at `PathCreation` sites,
* so to avoid perturbing existing alerts, where a `PathCreation` exists we
* continue to report there; otherwise we report directly at `sink`.
*/
DataFlow::Node getReportingNode(DataFlow::Node sink) {
TaintedPathFlow::flowTo(sink) and
if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
else result = sink
}

from TaintedPathFlow::PathNode source, TaintedPathFlow::PathNode sink
where TaintedPathFlow::flowPath(source, sink)
select getReportingNode(sink.getNode()), source, sink, "This path depends on a $@.",
source.getNode(), "user-provided value"
select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
"user-provided value"
18 changes: 2 additions & 16 deletions java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,7 @@ import semmle.code.java.security.PathCreation
import semmle.code.java.security.TaintedPathQuery
import TaintedPathLocalFlow::PathGraph

/**
* Gets the data-flow node at which to report a path ending at `sink`.
*
* Previously this query flagged alerts exclusively at `PathCreation` sites,
* so to avoid perturbing existing alerts, where a `PathCreation` exists we
* continue to report there; otherwise we report directly at `sink`.
*/
DataFlow::Node getReportingNode(DataFlow::Node sink) {
TaintedPathLocalFlow::flowTo(sink) and
if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
else result = sink
}

from TaintedPathLocalFlow::PathNode source, TaintedPathLocalFlow::PathNode sink
where TaintedPathLocalFlow::flowPath(source, sink)
select getReportingNode(sink.getNode()), source, sink, "This path depends on a $@.",
source.getNode(), "user-provided value"
select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
"user-provided value"
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: majorAnalysis
---
* The sinks of the queries `java/path-injection` and `java/path-injection-local` have been reworked. Path creation sinks have been converted to summaries instead, while sinks now are actual file read/write operations only. This has reduced the false positive ratio of both queries.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ 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.TaintedPathQuery
import JFinalController
import semmle.code.java.security.PathSanitizer
private import semmle.code.java.security.Sanitizers
Expand Down Expand Up @@ -52,7 +53,7 @@ module InjectFilePathConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }

predicate isSink(DataFlow::Node sink) {
sinkNode(sink, "path-injection") and
sink instanceof TaintedPathSink and
not sink instanceof NormalizedPathNode
}

Expand Down
Loading

0 comments on commit 4c0d535

Please sign in to comment.