Merge pull request #13906 from geoffw0/commandinject2

Swift: Add tests and develop command injection query

swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll

@@ -10,6 +10,6 @@ private import codeql.swift.dataflow.ExternalFlow
  */
 private class NsUrlSummaries extends SummaryModelCsv {
   override predicate row(string row) {
-    row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint"
+    row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint"
   }
 }

swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Url.qll

@@ -85,29 +85,33 @@ private class UrlSummaries extends SummaryModelCsv {
   override predicate row(string row) {
     row =
       [
-        ";URL;true;init(string:);(String);;Argument[0];ReturnValue;taint",
-        ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[0..1];ReturnValue;taint",
+        ";URL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint",
+        ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[0];ReturnValue.OptionalSome;taint",
+        ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[1].OptionalSome;ReturnValue.OptionalSome;taint",
         ";URL;true;init(fileURLWithPath:);;;Argument[0];ReturnValue;taint",
         ";URL;true;init(fileURLWithPath:isDirectory:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(fileURLWithPath:relativeTo:);;;Argument[0..1];ReturnValue;taint",
+        ";URL;true;init(fileURLWithPath:relativeTo:);;;Argument[0];ReturnValue;taint",
+        ";URL;true;init(fileURLWithPath:relativeTo:);;;Argument[1].OptionalSome;ReturnValue;taint",
         ";URL;true;init(fileURLWithPath:isDirectory:relativeTo:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(fileURLWithPath:isDirectory:relativeTo:);;;Argument[2];ReturnValue;taint",
+        ";URL;true;init(fileURLWithPath:isDirectory:relativeTo:);;;Argument[2].OptionalSome;ReturnValue;taint",
         ";URL;true;init(fileURLWithFileSystemRepresentation:isDirectory:relativeTo:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(fileURLWithFileSystemRepresentation:isDirectory:relativeTo:);;;Argument[2];ReturnValue;taint",
+        ";URL;true;init(fileURLWithFileSystemRepresentation:isDirectory:relativeTo:);;;Argument[2].OptionalSome;ReturnValue;taint",
         ";URL;true;init(fileReferenceLiteralResourceName:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(_:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(_:isDirectory:);;;Argument[0];ReturnValue;taint",
+        ";URL;true;init(_:);;;Argument[0];ReturnValue.OptionalSome;taint",
+        ";URL;true;init(_:isDirectory:);;;Argument[0];ReturnValue.OptionalSome;taint",
         ";URL;true;init(resolvingBookmarkData:options:relativeTo:bookmarkDataIsStale:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(resolvingBookmarkData:options:relativeTo:bookmarkDataIsStale:);;;Argument[2];ReturnValue;taint",
+        ";URL;true;init(resolvingBookmarkData:options:relativeTo:bookmarkDataIsStale:);;;Argument[2].OptionalSome;ReturnValue;taint",
         ";URL;true;init(resolvingAliasFileAt:options:);;;Argument[0];ReturnValue;taint",
         ";URL;true;init(resource:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[0..1];ReturnValue;taint",
+        ";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[0];ReturnValue;taint",
+        ";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[1].OptionalSome;ReturnValue;taint",
         ";URL;true;init(_:strategy:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(filePath:directoryHint:);;;Argument[0];ReturnValue;taint",
+        ";URL;true;init(filePath:directoryHint:);;;Argument[0];ReturnValue.OptionalSome;taint",
         ";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[0];ReturnValue;taint",
-        ";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[2];ReturnValue;taint",
-        ";URL;true;init(for:in:appropriateFor:create:);;;Argument[0..2];ReturnValue;taint",
-        ";URL;true;init(string:encodingInvalidCharacters:);;;Argument[0];ReturnValue;taint",
+        ";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[2].OptionalSome;ReturnValue;taint",
+        ";URL;true;init(for:in:appropriateFor:create:);;;Argument[0..1];ReturnValue;taint",
+        ";URL;true;init(for:in:appropriateFor:create:);;;Argument[2].OptionalSome;ReturnValue;taint",
+        ";URL;true;init(string:encodingInvalidCharacters:);;;Argument[0];ReturnValue.OptionalSome;taint",
         ";URL;true;resourceValues(forKeys:);;;Argument[-1];ReturnValue;taint",
         ";URL;true;setResourceValues(_:);;;Argument[0];Argument[-1];taint",
         ";URL;true;setTemporaryResourceValue(_:forKey:);;;Argument[-1..0];Argument[-1];taint",
@@ -125,7 +129,8 @@ private class UrlSummaries extends SummaryModelCsv {
         ";URL;true;deletingLastPathComponent();;;Argument[-1];ReturnValue;taint",
         ";URL;true;deletingPathExtension();;;Argument[-1];ReturnValue;taint",
         ";URL;true;bookmarkData(options:includingResourceValuesForKeys:relativeTo:);;;Argument[-1];ReturnValue;taint",
-        ";URL;true;bookmarkData(options:includingResourceValuesForKeys:relativeTo:);;;Argument[1..2];ReturnValue;taint",
+        ";URL;true;bookmarkData(options:includingResourceValuesForKeys:relativeTo:);;;Argument[1].OptionalSome.CollectionElement;ReturnValue;taint",
+        ";URL;true;bookmarkData(options:includingResourceValuesForKeys:relativeTo:);;;Argument[2].OptionalSome;ReturnValue;taint",
         ";URL;true;bookmarkData(withContentsOf:);;;Argument[0];ReturnValue;taint",
         ";URL;true;resourceValues(forKeys:fromBookmarkData:);;;Argument[1];ReturnValue;taint",
         ";URL;true;promisedItemResourceValues(forKeys:);;;Argument[-1];ReturnValue;taint",

swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll

@@ -29,50 +29,15 @@ class CommandInjectionAdditionalFlowStep extends Unit {
   abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
 }
 
-private class ProcessSink2 extends CommandInjectionSink instanceof DataFlow::Node {
-  ProcessSink2() {
-    exists(AssignExpr assign, ProcessHost s |
-      assign.getDest() = s and
-      this.asExpr() = assign.getSource()
-    )
-    or
-    exists(AssignExpr assign, ProcessHost s, ArrayExpr a |
-      assign.getDest() = s and
-      a = assign.getSource() and
-      this.asExpr() = a.getAnElement()
-    )
-  }
-}
-
-private class ProcessHost extends MemberRefExpr {
-  ProcessHost() { this.getBase() instanceof ProcessRef }
-}
-
-/** An expression of type `Process`. */
-private class ProcessRef extends Expr {
-  ProcessRef() {
-    this.getType() instanceof ProcessType or
-    this.getType() = any(OptionalType t | t.getBaseType() instanceof ProcessType)
-  }
-}
-
-/** The type `Process`. */
-private class ProcessType extends NominalType {
-  ProcessType() { this.getFullName() = "Process" }
-}
-
 /**
- * A `DataFlow::Node` that is written into a `Process` object.
+ * An additional taint step for command injection vulnerabilities.
  */
-private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node {
-  ProcessSink() {
-    // any write into a class derived from `Process` is a sink. For
-    // example in `Process.launchPath = sensitive` the post-update node corresponding
-    // with `Process.launchPath` is a sink.
-    exists(NominalType t, Expr e |
-      t.getABaseType*().getUnderlyingType().getName() = "Process" and
-      e.getFullyConverted() = this.asExpr() and
-      e.getFullyConverted().getType() = t
+private class CommandInjectionArrayAdditionalFlowStep extends CommandInjectionAdditionalFlowStep {
+  override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    // needed until we have proper content flow through arrays.
+    exists(ArrayExpr arr |
+      nodeFrom.asExpr() = arr.getAnElement() and
+      nodeTo.asExpr() = arr
     )
   }
 }
@@ -83,3 +48,24 @@ private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node
 private class DefaultCommandInjectionSink extends CommandInjectionSink {
   DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }
 }
+
+private class CommandInjectionSinks extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        ";Process;true;run(_:arguments:terminationHandler:);;;Argument[0..1];command-injection",
+        ";Process;true;launchedProcess(launchPath:arguments:);;;Argument[0..1];command-injection",
+        ";Process;true;arguments;;;PostUpdate;command-injection",
+        ";Process;true;currentDirectory;;;PostUpdate;command-injection",
+        ";Process;true;environment;;;PostUpdate;command-injection",
+        ";Process;true;executableURL;;;PostUpdate;command-injection",
+        ";Process;true;standardError;;;PostUpdate;command-injection",
+        ";Process;true;standardInput;;;PostUpdate;command-injection",
+        ";Process;true;standardOutput;;;PostUpdate;command-injection",
+        ";Process;true;currentDirectoryPath;;;PostUpdate;command-injection",
+        ";Process;true;launchPath;;;PostUpdate;command-injection",
+        ";NSUserScriptTask;true;init(url:);;;Argument[0];command-injection",
+        ";NSUserUnixTask;true;execute(withArguments:completionHandler:);;;Argument[0];command-injection",
+      ]
+  }
+}

swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll

@@ -1,6 +1,6 @@
 /**
  * Provides a taint-tracking configuration for reasoning about system
- * commands built from user-controlled sources (that is, Command injection
+ * commands built from user-controlled sources (that is, command injection
  * vulnerabilities).
  */
 
@@ -11,7 +11,7 @@ import codeql.swift.dataflow.FlowSources
 import codeql.swift.security.CommandInjectionExtensions
 
 /**
- * A taint configuration for tainted data that reaches a Command Injection sink.
+ * A taint configuration for tainted data that reaches a command injection sink.
  */
 module CommandInjectionConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) { node instanceof FlowSource }
@@ -26,6 +26,6 @@ module CommandInjectionConfig implements DataFlow::ConfigSig {
 }
 
 /**
- * Detect taint flow of tainted data that reaches a Command Injection sink.
+ * Detect taint flow of tainted data that reaches a command injection sink.
  */
 module CommandInjectionFlow = TaintTracking::Global<CommandInjectionConfig>;

swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp

@@ -24,14 +24,14 @@ using it.
 
 <example>
 <p>
-The following examples execute code from user input without
+The following example executes code from user input without
 sanitizing it first:
 </p>
 <sample src="CommandInjectionBad.swift" />
 <p>
 If user input is used to construct a command it should be checked
 first. This ensures that the user cannot insert characters that have special
-meanings.
+meanings:
 </p>
 <sample src="CommandInjectionGood.swift" />
 </example>
@@ -42,4 +42,4 @@ OWASP:
 <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
 </li>
 </references>
-</qhelp>
+</qhelp>

swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql

@@ -1,6 +1,7 @@
 /**
  * @name System command built from user-controlled sources
- * @description Building a system command from user-controlled sources is vulnerable to insertion of malicious code by the user.
+ * @description Building a system command from user-controlled sources may allow a malicious
+ *              user to change the meaning of the command.
  * @kind path-problem
  * @problem.severity error
  * @security-severity 9.8

swift/ql/src/experimental/Security/CWE-078/CommandInjectionGood.swift

@@ -6,8 +6,10 @@ func validateCommand(_ command: String) -> String? {
     return nil
 }
 
-var task = Process()
-task.launchPath = "/bin/bash"
-task.arguments = ["-c", validateCommand(userControlledString)] // GOOD
+if let validatedString = validateCommand(userControlledString) {
+    var task = Process()
+    task.launchPath = "/bin/bash"
+    task.arguments = ["-c", validatedString] // GOOD
 
-task.launch()
+    task.launch()
+}