Merge pull request #14127 from egregius313/egregius313/java/mad/localuserinput

egregius313 · web-flow · commit 8e653d01a8a5 · 2023-10-04T12:55:44.000-04:00
Java: Convert implementations of `LocalUserInput` to Models-as-Data
diff --git a/java/ql/lib/change-notes/2023-10-05-moved-localuserinput-to-mad.md b/java/ql/lib/change-notes/2023-10-05-moved-localuserinput-to-mad.md
@@ -0,0 +1,22 @@
+---
+category: minorAnalysis
+---
+* Modified the `EnvInput` class in `semmle.code.java.dataflow.FlowSources` to include `environment` and `file` source nodes.
+  There are no changes to results unless you add source models using the `environment` or `file` source kinds.
+* Added `environment` source models for the following methods:
+  * `java.lang.System#getenv`
+  * `java.lang.System#getProperties`
+  * `java.lang.System#getProperty`
+  * `java.util.Properties#get`
+  * `java.util.Properties#getProperty`
+* Added `file` source models for the following methods:
+  * the `java.io.FileInputStream` constructor
+  * `hudson.FilePath#newInputStreamDenyingSymlinkAsNeeded`
+  * `hudson.FilePath#openInputStream`
+  * `hudson.FilePath#read`
+  * `hudson.FilePath#readFromOffset`
+  * `hudson.FilePath#readToString`
+* Modified the `DatabaseInput` class in `semmle.code.java.dataflow.FlowSources` to include `database` source nodes.
+  There are no changes to results unless you add source models using the `database` source kind.
+* Added `database` source models for the following method:
+  * `java.sql.ResultSet#getString`
diff --git a/java/ql/lib/ext/hudson.model.yml b/java/ql/lib/ext/hudson.model.yml
@@ -36,6 +36,11 @@ extensions:
       pack: codeql/java-all
       extensible: sourceModel
     data:
+      - ["hudson", "FilePath", False, "newInputStreamDenyingSymlinkAsNeeded", "", "", "ReturnValue", "file", "manual"]
+      - ["hudson", "FilePath", False, "openInputStream", "", "", "ReturnValue", "file", "manual"]
+      - ["hudson", "FilePath", False, "read", "", "", "ReturnValue", "file", "manual"]
+      - ["hudson", "FilePath", False, "readFromOffset", "", "", "ReturnValue", "file", "manual"]
+      - ["hudson", "FilePath", False, "readToString", "", "", "ReturnValue", "file", "manual"]
       - ["hudson", "Plugin", True, "configure", "", "", "Parameter", "remote", "manual"]
       - ["hudson", "Plugin", True, "newInstance", "", "", "Parameter", "remote", "manual"]
   - addsTo:
diff --git a/java/ql/lib/ext/java.io.model.yml b/java/ql/lib/ext/java.io.model.yml
@@ -128,3 +128,8 @@ extensions:
       # sink neutrals
       - ["java.io", "File", "compareTo", "", "sink", "hq-manual"]
       - ["java.io", "File", "exists", "()", "sink", "hq-manual"]
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sourceModel
+    data:
+      - ["java.io", "FileInputStream", True, "FileInputStream", "", "", "Argument[this]", "file", "manual"]
diff --git a/java/ql/lib/ext/java.lang.model.yml b/java/ql/lib/ext/java.lang.model.yml
@@ -40,6 +40,13 @@ extensions:
       - ["java.lang", "System$Logger", True, "log", "(System$Logger$Level,String,Supplier)", "", "Argument[1..2]", "log-injection", "manual"]
       - ["java.lang", "System$Logger", True, "log", "(System$Logger$Level,String,Supplier,Throwable)", "", "Argument[1..2]", "log-injection", "manual"]
       - ["java.lang", "System$Logger", True, "log", "(System$Logger$Level,String,Throwable)", "", "Argument[1]", "log-injection", "manual"]
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sourceModel
+    data:
+      - ["java.lang", "System", False, "getenv", "", "", "ReturnValue", "environment", "manual"]
+      - ["java.lang", "System", False, "getProperties", "", "", "ReturnValue", "environment", "manual"]
+      - ["java.lang", "System", False, "getProperty", "", "", "ReturnValue", "environment", "manual"]
   - addsTo:
       pack: codeql/java-all
       extensible: summaryModel
diff --git a/java/ql/lib/ext/java.sql.model.yml b/java/ql/lib/ext/java.sql.model.yml
@@ -45,3 +45,8 @@ extensions:
       - ["java.sql", "ResultSet", "getTimestamp", "(String)", "summary", "manual"]      # taint-numeric
       - ["java.sql", "Timestamp", "Timestamp", "(long)", "summary", "manual"]           # taint-numeric
       - ["java.sql", "Timestamp", "getTime", "()", "summary", "manual"]                 # taint-numeric
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sourceModel
+    data:
+      - ["java.sql", "ResultSet", True, "getString", "", "", "ReturnValue", "database", "manual"]
diff --git a/java/ql/lib/ext/java.util.model.yml b/java/ql/lib/ext/java.util.model.yml
@@ -1,4 +1,11 @@
 extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sourceModel
+    data:
+      - ["java.util", "Properties", True, "get", "", "", "ReturnValue", "environment", "manual"]
+      - ["java.util", "Properties", True, "getProperty", "", "", "ReturnValue", "environment", "manual"]
+
   - addsTo:
       pack: codeql/java-all
       extensible: summaryModel
diff --git a/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll b/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll
@@ -233,10 +233,7 @@ deprecated class EnvInput extends DataFlow::Node {
  * environment variables.
  */
 private class EnvironmentInput extends LocalUserInput {
-  EnvironmentInput() {
-    // Results from various specific methods.
-    this.asExpr().(MethodAccess).getMethod() instanceof EnvReadMethod
-  }
+  EnvironmentInput() { sourceNode(this, "environment") }
 
   override string getThreatModel() { result = "environment" }
 }
@@ -268,10 +265,7 @@ private class CliInput extends LocalUserInput {
 private class FileInput extends LocalUserInput {
   FileInput() {
     // Access to files.
-    this.asExpr()
-        .(ConstructorCall)
-        .getConstructedType()
-        .hasQualifiedName("java.io", "FileInputStream")
+    sourceNode(this, "file")
   }
 
   override string getThreatModel() { result = "file" }
@@ -292,7 +286,7 @@ deprecated class DatabaseInput = DbInput;
  * A node with input from a database.
  */
 private class DbInput extends LocalUserInput {
-  DbInput() { this.asExpr().(MethodAccess).getMethod() instanceof ResultSetGetStringMethod }
+  DbInput() { sourceNode(this, "database") }
 
   override string getThreatModel() { result = "database" }
 }
diff --git a/java/ql/lib/semmle/code/java/frameworks/hudson/Hudson.qll b/java/ql/lib/semmle/code/java/frameworks/hudson/Hudson.qll
@@ -13,21 +13,6 @@ class HudsonWebMethod extends Method {
   }
 }
 
-private class FilePathRead extends LocalUserInput {
-  FilePathRead() {
-    this.asExpr()
-        .(MethodAccess)
-        .getMethod()
-        .hasQualifiedName("hudson", "FilePath",
-          [
-            "newInputStreamDenyingSymlinkAsNeeded", "openInputStream", "read", "readFromOffset",
-            "readToString"
-          ])
-  }
-
-  override string getThreatModel() { result = "file" }
-}
-
 private class HudsonUtilXssSanitizer extends XssSanitizer {
   HudsonUtilXssSanitizer() {
     this.asExpr()
diff --git a/shared/mad/codeql/mad/ModelValidation.qll b/shared/mad/codeql/mad/ModelValidation.qll
@@ -115,11 +115,11 @@ module KindValidation<KindValidationConfigSig Config> {
       this =
         [
           // shared
-          "local", "remote",
+          "local", "remote", "file",
           // Java
-          "android-external-storage-dir", "contentprovider",
+          "android-external-storage-dir", "contentprovider", "database", "environment",
           // C#
-          "file", "file-write",
+          "file-write",
           // JavaScript
           "database-access-result"
         ]
