Merge pull request #13726 from maikypedia/maikypedia/swift-command-injection

Swift: Add Command Injection query (CWE-078)

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

@@ -0,0 +1,85 @@
+/**
+ * Provides classes and predicates for reasoning about system
+ * commands built from user-controlled sources (that is, command injection
+ * vulnerabilities).
+ */
+
+import swift
+import codeql.swift.dataflow.DataFlow
+import codeql.swift.dataflow.ExternalFlow
+
+/**
+ * A dataflow sink for command injection vulnerabilities.
+ */
+abstract class CommandInjectionSink extends DataFlow::Node { }
+
+/**
+ * A barrier for command injection vulnerabilities.
+ */
+abstract class CommandInjectionBarrier extends DataFlow::Node { }
+
+/**
+ * A unit class for adding additional flow steps.
+ */
+class CommandInjectionAdditionalFlowStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a flow
+   * step for paths related to command injection vulnerabilities.
+   */
+  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.
+ */
+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
+    )
+  }
+}
+
+/**
+ * A sink defined in a CSV model.
+ */
+private class DefaultCommandInjectionSink extends CommandInjectionSink {
+  DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }
+}

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

@@ -0,0 +1,31 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about system
+ * commands built from user-controlled sources (that is, Command injection
+ * vulnerabilities).
+ */
+
+import swift
+import codeql.swift.dataflow.DataFlow
+import codeql.swift.dataflow.TaintTracking
+import codeql.swift.dataflow.FlowSources
+import codeql.swift.security.CommandInjectionExtensions
+
+/**
+ * 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 }
+
+  predicate isSink(DataFlow::Node node) { node instanceof CommandInjectionSink }
+
+  predicate isBarrier(DataFlow::Node barrier) { barrier instanceof CommandInjectionBarrier }
+
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    any(CommandInjectionAdditionalFlowStep s).step(nodeFrom, nodeTo)
+  }
+}
+
+/**
+ * Detect taint flow of tainted data that reaches a Command Injection sink.
+ */
+module CommandInjectionFlow = TaintTracking::Global<CommandInjectionConfig>;

swift/ql/src/change-notes/2023-07-12-command-injection.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added new query "Command injection" (`swift/command-line-injection`). The query finds places where user input is used to execute system commands without proper escaping.

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

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Constructing a system command with unsanitized user input is dangerous,
+since a malicious user may be able to craft input that executes arbitrary code.
+</p>
+</overview>
+
+<recommendation>
+<p>
+If possible, use hard-coded string literals to specify the command to run. Instead of interpreting
+user input directly as command names, examine the input and then choose among hard-coded string
+literals.
+</p>
+<p>
+If this is not possible, then add sanitization code to verify that the user input is safe before
+using it.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following examples execute 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.
+</p>
+<sample src="CommandInjectionGood.swift" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+</references>
+</qhelp>

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

@@ -0,0 +1,22 @@
+/**
+ * @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.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.8
+ * @precision high
+ * @id swift/command-line-injection
+ * @tags security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ */
+
+import swift
+import codeql.swift.dataflow.DataFlow
+import codeql.swift.security.CommandInjectionQuery
+import CommandInjectionFlow::PathGraph
+
+from CommandInjectionFlow::PathNode sourceNode, CommandInjectionFlow::PathNode sinkNode
+where CommandInjectionFlow::flowPath(sourceNode, sinkNode)
+select sinkNode.getNode(), sourceNode, sinkNode, "This command depends on a $@.",
+  sourceNode.getNode(), "user-provided value"

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

@@ -0,0 +1,5 @@
+var task = Process()
+task.launchPath = "/bin/bash"
+task.arguments = ["-c", userControlledString] // BAD
+
+task.launch()

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

@@ -0,0 +1,13 @@
+func validateCommand(_ command: String) -> String? {
+    let allowedCommands = ["ls -l", "pwd", "echo"]
+    if allowedCommands.contains(command) {
+        return command
+    }
+    return nil
+}
+
+var task = Process()
+task.launchPath = "/bin/bash"
+task.arguments = ["-c", validateCommand(userControlledString)] // GOOD
+
+task.launch()