Migrate path injection sinks to MaD

Deprecate and stop using PathCreation

Path creation sinks are now summaries

Author: atorralba

java/ql/lib/change-notes/2023-04-20-deprecated-path-creation.md

@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* The `PathCreation` class in `PathCreation.qll` has been deprecated.

java/ql/lib/ext/java.io.model.yml

@@ -3,18 +3,17 @@ 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, "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"]

java/ql/lib/ext/java.nio.file.model.yml

@@ -37,15 +37,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:
@@ -63,7 +56,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"]
@@ -76,7 +69,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"]

java/ql/lib/semmle/code/java/security/PathCreation.qll

@@ -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`.
@@ -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
@@ -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
@@ -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() {
@@ -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
@@ -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
@@ -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
@@ -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() {
@@ -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() {
@@ -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")
   }
@@ -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")
   }

java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll

@@ -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.
  *
@@ -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
@@ -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

java/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -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"

java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql

@@ -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"

java/ql/src/change-notes/2023-04-20-path-injection-precision-improved.md

@@ -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.

java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql

@@ -16,6 +16,10 @@ import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.dataflow.FlowSources
+<<<<<<< HEAD
+=======
+import semmle.code.java.security.TaintedPathQuery
+>>>>>>> 9e469c9c32 (Migrate path injection sinks to MaD)
 import JFinalController
 import semmle.code.java.security.PathSanitizer
 private import semmle.code.java.security.Sanitizers
@@ -52,7 +56,11 @@ module InjectFilePathConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }
 
   predicate isSink(DataFlow::Node sink) {
+<<<<<<< HEAD
     sinkNode(sink, "path-injection") and
+=======
+    sink instanceof TaintedPathSink and
+>>>>>>> 9e469c9c32 (Migrate path injection sinks to MaD)
     not sink instanceof NormalizedPathNode
   }
 

java/ql/test/library-tests/pathcreation/PathCreation.expected

@@ -1,3 +1,4 @@
+WARNING: Type PathCreation has been deprecated and may be removed in future (PathCreation.ql:4,6-18)
 | PathCreation.java:13:18:13:32 | new File(...) | PathCreation.java:13:27:13:31 | "dir" |
 | PathCreation.java:14:19:14:40 | new File(...) | PathCreation.java:14:28:14:32 | "dir" |
 | PathCreation.java:14:19:14:40 | new File(...) | PathCreation.java:14:35:14:39 | "sub" |