diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll new file mode 100644 index 000000000000..26e8782a7994 --- /dev/null +++ b/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") } +} diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll new file mode 100644 index 000000000000..4b67932209dc --- /dev/null +++ b/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; diff --git a/swift/ql/src/change-notes/2023-07-12-command-injection.md b/swift/ql/src/change-notes/2023-07-12-command-injection.md new file mode 100644 index 000000000000..2befc7592b94 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp new file mode 100644 index 000000000000..ab9562af12c2 --- /dev/null +++ b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp @@ -0,0 +1,45 @@ + + + + +

+Constructing a system command with unsanitized user input is dangerous, +since a malicious user may be able to craft input that executes arbitrary code. +

+ + + +

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

+

+If this is not possible, then add sanitization code to verify that the user input is safe before +using it. +

+ + + +

+The following examples execute code from user input without +sanitizing it first: +

+ +

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

+ + + + +
  • +OWASP: +Command Injection. +
    • +     +     \ No newline at end of file diff --git a/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql new file mode 100644 index 000000000000..98d68d0a1018 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/swift/ql/src/experimental/Security/CWE-078/CommandInjectionBad.swift b/swift/ql/src/experimental/Security/CWE-078/CommandInjectionBad.swift new file mode 100644 index 000000000000..ffdaaf907caa --- /dev/null +++ b/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() \ No newline at end of file diff --git a/swift/ql/src/experimental/Security/CWE-078/CommandInjectionGood.swift b/swift/ql/src/experimental/Security/CWE-078/CommandInjectionGood.swift new file mode 100644 index 000000000000..3482718eeac5 --- /dev/null +++ b/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() \ No newline at end of file