Merge pull request #3881 from intrigus-lgtm/more-pathcreations

Java: Centralize and model additional path creations.

java/ql/src/Security/CWE/CWE-022/PathsCommon.qll

@@ -1,74 +0,0 @@
-import java
-import semmle.code.java.controlflow.Guards
-
-abstract class PathCreation extends Expr {
-  abstract Expr getInput();
-}
-
-class PathsGet extends PathCreation, MethodAccess {
-  PathsGet() {
-    exists(Method m | m = this.getMethod() |
-      m.getDeclaringType() instanceof TypePaths and
-      m.getName() = "get"
-    )
-  }
-
-  override Expr getInput() { result = this.getAnArgument() }
-}
-
-class FileSystemGetPath extends PathCreation, MethodAccess {
-  FileSystemGetPath() {
-    exists(Method m | m = this.getMethod() |
-      m.getDeclaringType() instanceof TypeFileSystem and
-      m.getName() = "getPath"
-    )
-  }
-
-  override Expr getInput() { result = this.getAnArgument() }
-}
-
-class FileCreation extends PathCreation, ClassInstanceExpr {
-  FileCreation() { this.getConstructedType() instanceof TypeFile }
-
-  override Expr getInput() {
-    result = this.getAnArgument() and
-    // Relevant arguments include those that are not a `File`.
-    not result.getType() instanceof TypeFile
-  }
-}
-
-class FileWriterCreation extends PathCreation, ClassInstanceExpr {
-  FileWriterCreation() { this.getConstructedType().getQualifiedName() = "java.io.FileWriter" }
-
-  override Expr getInput() {
-    result = this.getAnArgument() and
-    // Relevant arguments are those of type `String`.
-    result.getType() instanceof TypeString
-  }
-}
-
-predicate inWeakCheck(Expr e) {
-  // None of these are sufficient to guarantee that a string is safe.
-  exists(MethodAccess m, Method def | m.getQualifier() = e and m.getMethod() = def |
-    def.getName() = "startsWith" or
-    def.getName() = "endsWith" or
-    def.getName() = "isEmpty" or
-    def.getName() = "equals"
-  )
-  or
-  // Checking against `null` has no bearing on path traversal.
-  exists(EqualityTest b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral)
-}
-
-// Ignore cases where the variable has been checked somehow,
-// but allow some particularly obviously bad cases.
-predicate guarded(VarAccess e) {
-  exists(PathCreation p | e = p.getInput()) and
-  exists(ConditionBlock cb, Expr c |
-    cb.getCondition().getAChildExpr*() = c and
-    c = e.getVariable().getAnAccess() and
-    cb.controls(e.getBasicBlock(), true) and
-    // Disallow a few obviously bad checks.
-    not inWeakCheck(c)
-  )
-}

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

@@ -14,8 +14,9 @@
 
 import java
 import semmle.code.java.dataflow.FlowSources
-import PathsCommon
+import semmle.code.java.security.PathCreation
 import DataFlow::PathGraph
+import TaintedPathCommon
 
 class ContainsDotDotSanitizer extends DataFlow::BarrierGuard {
   ContainsDotDotSanitizer() {
@@ -34,7 +35,7 @@ class TaintedPathConfig extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(Expr e | e = sink.asExpr() | e = any(PathCreation p).getInput() and not guarded(e))
+    exists(Expr e | e = sink.asExpr() | e = any(PathCreation p).getAnInput() and not guarded(e))
   }
 
   override predicate isSanitizer(DataFlow::Node node) {
@@ -48,7 +49,7 @@ class TaintedPathConfig extends TaintTracking::Configuration {
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, PathCreation p, TaintedPathConfig conf
 where
-  sink.getNode().asExpr() = p.getInput() and
+  sink.getNode().asExpr() = p.getAnInput() and
   conf.hasFlowPath(source, sink)
 select p, source, sink, "$@ flows to here and is used in a path.", source.getNode(),
   "User-provided value"

java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

@@ -0,0 +1,33 @@
+/**
+ * Models a very basic guard for the tainted path queries.
+ */
+
+import java
+import semmle.code.java.controlflow.Guards
+import semmle.code.java.security.PathCreation
+
+private predicate inWeakCheck(Expr e) {
+  // None of these are sufficient to guarantee that a string is safe.
+  exists(MethodAccess m, Method def | m.getQualifier() = e and m.getMethod() = def |
+    def.getName() = "startsWith" or
+    def.getName() = "endsWith" or
+    def.getName() = "isEmpty" or
+    def.getName() = "equals"
+  )
+  or
+  // Checking against `null` has no bearing on path traversal.
+  exists(EqualityTest b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral)
+}
+
+// Ignore cases where the variable has been checked somehow,
+// but allow some particularly obviously bad cases.
+predicate guarded(VarAccess e) {
+  exists(PathCreation p | e = p.getAnInput()) and
+  exists(ConditionBlock cb, Expr c |
+    cb.getCondition().getAChildExpr*() = c and
+    c = e.getVariable().getAnAccess() and
+    cb.controls(e.getBasicBlock(), true) and
+    // Disallow a few obviously bad checks.
+    not inWeakCheck(c)
+  )
+}

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

@@ -14,15 +14,18 @@
 
 import java
 import semmle.code.java.dataflow.FlowSources
-import PathsCommon
+import semmle.code.java.security.PathCreation
 import DataFlow::PathGraph
+import TaintedPathCommon
 
 class TaintedPathLocalConfig extends TaintTracking::Configuration {
   TaintedPathLocalConfig() { this = "TaintedPathLocalConfig" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
 
-  override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(PathCreation p).getInput() }
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(PathCreation p).getAnInput()
+  }
 }
 
 from
@@ -30,7 +33,7 @@ from
   TaintedPathLocalConfig conf
 where
   e = sink.getNode().asExpr() and
-  e = p.getInput() and
+  e = p.getAnInput() and
   conf.hasFlowPath(source, sink) and
   not guarded(e)
 select p, source, sink, "$@ flows to here and is used in a path.", source.getNode(),

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

@@ -0,0 +1,141 @@
+/**
+ * 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 {
+  /**
+   * Gets an input that is used in the creation of this path.
+   * This excludes inputs of type `File` and `Path`.
+   */
+  abstract Expr getAnInput();
+}
+
+/** Models the `java.nio.file.Paths.get` method. */
+private class PathsGet extends PathCreation, MethodAccess {
+  PathsGet() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypePaths and
+      m.getName() = "get"
+    )
+  }
+
+  override Expr getAnInput() { result = this.getAnArgument() }
+}
+
+/** Models the `java.nio.file.FileSystem.getPath` method. */
+private class FileSystemGetPath extends PathCreation, MethodAccess {
+  FileSystemGetPath() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypeFileSystem and
+      m.getName() = "getPath"
+    )
+  }
+
+  override Expr getAnInput() { result = this.getAnArgument() }
+}
+
+/** Models the `new java.io.File(...)` constructor. */
+private class FileCreation extends PathCreation, ClassInstanceExpr {
+  FileCreation() { this.getConstructedType() instanceof TypeFile }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments include those that are not a `File`.
+    not result.getType() instanceof TypeFile
+  }
+}
+
+/** Models the `java.nio.file.Path.resolveSibling` method. */
+private class PathResolveSiblingCreation extends PathCreation, MethodAccess {
+  PathResolveSiblingCreation() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypePath and
+      m.getName() = "resolveSibling"
+    )
+  }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments are those of type `String`.
+    result.getType() instanceof TypeString
+  }
+}
+
+/** Models the `java.nio.file.Path.resolve` method. */
+private class PathResolveCreation extends PathCreation, MethodAccess {
+  PathResolveCreation() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypePath and
+      m.getName() = "resolve"
+    )
+  }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments are those of type `String`.
+    result.getType() instanceof TypeString
+  }
+}
+
+/** Models the `java.nio.file.Path.of` method. */
+private class PathOfCreation extends PathCreation, MethodAccess {
+  PathOfCreation() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypePath and
+      m.getName() = "of"
+    )
+  }
+
+  override Expr getAnInput() { result = this.getAnArgument() }
+}
+
+/** Models the `new java.io.FileWriter(...)` constructor. */
+private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
+  FileWriterCreation() { this.getConstructedType().hasQualifiedName("java.io", "FileWriter") }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments are those of type `String`.
+    result.getType() instanceof TypeString
+  }
+}
+
+/** Models the `new java.io.FileReader(...)` constructor. */
+private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
+  FileReaderCreation() { this.getConstructedType().hasQualifiedName("java.io", "FileReader") }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments are those of type `String`.
+    result.getType() instanceof TypeString
+  }
+}
+
+/** Models the `new java.io.FileInputStream(...)` constructor. */
+private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
+  FileInputStreamCreation() {
+    this.getConstructedType().hasQualifiedName("java.io", "FileInputStream")
+  }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments are those of type `String`.
+    result.getType() instanceof TypeString
+  }
+}
+
+/** Models the `new java.io.FileOutputStream(...)` constructor. */
+private class FileOutputStreamCreation extends PathCreation, ClassInstanceExpr {
+  FileOutputStreamCreation() {
+    this.getConstructedType().hasQualifiedName("java.io", "FileOutputStream")
+  }
+
+  override Expr getAnInput() {
+    result = this.getAnArgument() and
+    // Relevant arguments are those of type `String`.
+    result.getType() instanceof TypeString
+  }
+}

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

@@ -0,0 +1,25 @@
+| 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" |
+| PathCreation.java:18:18:18:49 | new File(...) | PathCreation.java:18:44:18:48 | "sub" |
+| PathCreation.java:18:27:18:41 | new File(...) | PathCreation.java:18:36:18:40 | "dir" |
+| PathCreation.java:22:18:22:41 | new File(...) | PathCreation.java:22:27:22:40 | new URI(...) |
+| PathCreation.java:26:18:26:31 | of(...) | PathCreation.java:26:26:26:30 | "dir" |
+| PathCreation.java:27:19:27:39 | of(...) | PathCreation.java:27:27:27:31 | "dir" |
+| PathCreation.java:27:19:27:39 | of(...) | PathCreation.java:27:34:27:38 | "sub" |
+| PathCreation.java:31:18:31:40 | of(...) | PathCreation.java:31:26:31:39 | new URI(...) |
+| PathCreation.java:35:18:35:33 | get(...) | PathCreation.java:35:28:35:32 | "dir" |
+| PathCreation.java:36:19:36:41 | get(...) | PathCreation.java:36:29:36:33 | "dir" |
+| PathCreation.java:36:19:36:41 | get(...) | PathCreation.java:36:36:36:40 | "sub" |
+| PathCreation.java:40:18:40:42 | get(...) | PathCreation.java:40:28:40:41 | new URI(...) |
+| PathCreation.java:44:18:44:56 | getPath(...) | PathCreation.java:44:51:44:55 | "dir" |
+| PathCreation.java:45:19:45:64 | getPath(...) | PathCreation.java:45:52:45:56 | "dir" |
+| PathCreation.java:45:19:45:64 | getPath(...) | PathCreation.java:45:59:45:63 | "sub" |
+| PathCreation.java:49:18:49:31 | of(...) | PathCreation.java:49:26:49:30 | "dir" |
+| PathCreation.java:49:18:49:53 | resolveSibling(...) | PathCreation.java:49:48:49:52 | "sub" |
+| PathCreation.java:53:18:53:31 | of(...) | PathCreation.java:53:26:53:30 | "dir" |
+| PathCreation.java:53:18:53:46 | resolve(...) | PathCreation.java:53:41:53:45 | "sub" |
+| PathCreation.java:57:25:57:45 | new FileWriter(...) | PathCreation.java:57:40:57:44 | "dir" |
+| PathCreation.java:61:25:61:45 | new FileReader(...) | PathCreation.java:61:40:61:44 | "dir" |
+| PathCreation.java:65:32:65:58 | new FileOutputStream(...) | PathCreation.java:65:53:65:57 | "dir" |
+| PathCreation.java:69:31:69:56 | new FileInputStream(...) | PathCreation.java:69:51:69:55 | "dir" |

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

@@ -0,0 +1,71 @@
+import java.io.File;
+import java.io.FileWriter;
+import java.io.FileReader;
+import java.io.FileOutputStream;
+import java.io.FileInputStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.FileSystems;
+import java.net.URI;
+
+class PathCreation {
+    public void testNewFileWithString() {
+        File f = new File("dir");
+        File f2 = new File("dir", "sub");
+    }
+
+    public void testNewFileWithFileString() {
+        File f = new File(new File("dir"), "sub");
+    }
+
+    public void testNewFileWithURI() {
+        File f = new File(new URI("dir"));
+    }
+
+    public void testPathOfWithString() {
+        Path p = Path.of("dir");
+        Path p2 = Path.of("dir", "sub");
+    }
+
+    public void testPathOfWithURI() {
+        Path p = Path.of(new URI("dir"));
+    }
+
+    public void testPathsGetWithString() {
+        Path p = Paths.get("dir");
+        Path p2 = Paths.get("dir", "sub");
+    }
+
+    public void testPathsGetWithURI() {
+        Path p = Paths.get(new URI("dir"));
+    }
+
+    public void testFileSystemGetPathWithString() {
+        Path p = FileSystems.getDefault().getPath("dir");
+        Path p2 = FileSystems.getDefault().getPath("dir", "sub");
+    }
+
+    public void testPathResolveSiblingWithString() {
+        Path p = Path.of("dir").resolveSibling("sub");
+    }
+
+    public void testPathResolveWithString() {
+        Path p = Path.of("dir").resolve("sub");
+    }
+
+    public void testNewFileWriterWithString() {
+        FileWriter fw = new FileWriter("dir");
+    }
+
+    public void testNewFileReaderWithString() {
+        FileReader fr = new FileReader("dir");
+    }
+
+    public void testNewFileOutputStreamWithString() {
+        FileOutputStream fos = new FileOutputStream("dir");
+    }
+
+    public void testNewFileInputStreamWithString() {
+        FileInputStream fis = new FileInputStream("dir");
+    }
+}

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

@@ -0,0 +1,5 @@
+import java
+import semmle.code.java.security.PathCreation
+
+from PathCreation path
+select path, path.getAnInput()