From e45a31744f31acf3dc27c2602ab23210c1b48bf5 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Sun, 9 Jul 2023 04:17:35 +0200 Subject: [PATCH 1/9] Initial commit --- .../security/CommandInjectionExtensions.qll | 50 +++++++++++++++++++ .../swift/security/CommandInjectionQuery.qll | 27 ++++++++++ 2 files changed, 77 insertions(+) create mode 100644 swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll create mode 100644 swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll 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..a553ab089558 --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll @@ -0,0 +1,50 @@ +/** + * 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); +} + +/** 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 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..520794a365e1 --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll @@ -0,0 +1,27 @@ +/** + * 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 } +} + +/** + * Detect taint flow of tainted data that reaches a Command Injection sink. + */ +module CommandInjectionFlow = TaintTracking::Global; From cea3477ac204424b1ecdc05f14a8ec894c9c12d5 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Wed, 12 Jul 2023 02:13:07 +0200 Subject: [PATCH 2/9] Qhelp and examples --- .../security/CommandInjectionExtensions.qll | 19 +++++++- .../2023-07-12-command-injection.md | 4 ++ .../Security/CWE-078/CommandInjection.qhelp | 45 +++++++++++++++++++ .../Security/CWE-078/CommandInjection.ql | 21 +++++++++ .../CWE-078/CommandInjectionBad.swift | 5 +++ .../CWE-078/CommandInjectionGood.swift | 13 ++++++ 6 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 swift/ql/src/change-notes/2023-07-12-command-injection.md create mode 100644 swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp create mode 100644 swift/ql/src/queries/Security/CWE-078/CommandInjection.ql create mode 100644 swift/ql/src/queries/Security/CWE-078/CommandInjectionBad.swift create mode 100644 swift/ql/src/queries/Security/CWE-078/CommandInjectionGood.swift diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll index a553ab089558..5c34a2b9a853 100644 --- a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll @@ -42,9 +42,26 @@ private class ProcessType extends NominalType { ProcessType() { this.getFullName() = "Process" } } +/** + * A `DataFlow::Node` that is an expression stored with the Realm database + * library. + */ +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") } + DefaultCommandInjectionSink() { sinkNode(this, "command-line-injection") } } 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/queries/Security/CWE-078/CommandInjection.qhelp b/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp new file mode 100644 index 000000000000..80ebda716e06 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp @@ -0,0 +1,45 @@ + + + + +

+Constructing a system command with unsanitized user input is dangerous, +since a malicious user to execute 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 regular expression 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/queries/Security/CWE-078/CommandInjection.ql b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql new file mode 100644 index 000000000000..c81b13af10f7 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql @@ -0,0 +1,21 @@ +/** + * @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 + */ + +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" diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjectionBad.swift b/swift/ql/src/queries/Security/CWE-078/CommandInjectionBad.swift new file mode 100644 index 000000000000..ffdaaf907caa --- /dev/null +++ b/swift/ql/src/queries/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/queries/Security/CWE-078/CommandInjectionGood.swift b/swift/ql/src/queries/Security/CWE-078/CommandInjectionGood.swift new file mode 100644 index 000000000000..3482718eeac5 --- /dev/null +++ b/swift/ql/src/queries/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 From d7d9ffc449b6d0f76702c937ce734f1b8071a233 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Wed, 12 Jul 2023 16:44:17 +0200 Subject: [PATCH 3/9] Doc error Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com> --- .../lib/codeql/swift/security/CommandInjectionExtensions.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll index 5c34a2b9a853..bbcbe244daab 100644 --- a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll @@ -43,8 +43,7 @@ private class ProcessType extends NominalType { } /** - * A `DataFlow::Node` that is an expression stored with the Realm database - * library. + * A `DataFlow::Node` that is written into a `Process` object. */ private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node { ProcessSink() { From c9fadd98f47fe8c5a050c6baed4fa53782b5a6b3 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Wed, 12 Jul 2023 16:48:27 +0200 Subject: [PATCH 4/9] Support `CommandInjectionAdditionalFlowStep` and fix doc errors --- .../lib/codeql/swift/security/CommandInjectionExtensions.qll | 2 +- swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll | 4 ++++ swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp | 4 ++-- swift/ql/src/queries/Security/CWE-078/CommandInjection.ql | 1 + 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll index bbcbe244daab..fee86714cf2c 100644 --- a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll @@ -62,5 +62,5 @@ private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node * A sink defined in a CSV model. */ private class DefaultCommandInjectionSink extends CommandInjectionSink { - DefaultCommandInjectionSink() { sinkNode(this, "command-line-injection") } + 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 index 520794a365e1..4b67932209dc 100644 --- a/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll @@ -19,6 +19,10 @@ module CommandInjectionConfig implements DataFlow::ConfigSig { 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) + } } /** diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp b/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp index 80ebda716e06..ab9562af12c2 100644 --- a/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp +++ b/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp @@ -6,7 +6,7 @@

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

    @@ -29,7 +29,7 @@ sanitizing it first:

    -If user input is used to construct a regular expression it should be checked +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.

    diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql index c81b13af10f7..148676e1ac38 100644 --- a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql +++ b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql @@ -8,6 +8,7 @@ * @id swift/command-line-injection * @tags security * external/cwe/cwe-078 + * external/cwe/cwe-088 */ import swift From 378313332be8e3c8d473f89260bf27da665ba7ac Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Fri, 14 Jul 2023 20:55:24 +0200 Subject: [PATCH 5/9] Fix sink --- .../security/CommandInjectionExtensions.qll | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll index fee86714cf2c..26e8782a7994 100644 --- a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll @@ -29,6 +29,25 @@ 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() { From d0a912fb025f75c0762fec39bd857b2f1dbb9752 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Thu, 27 Jul 2023 22:45:05 +0200 Subject: [PATCH 6/9] Update swift/ql/src/queries/Security/CWE-078/CommandInjection.ql Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com> --- swift/ql/src/queries/Security/CWE-078/CommandInjection.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql index 148676e1ac38..eb501556b290 100644 --- a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql +++ b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql @@ -9,6 +9,7 @@ * @tags security * external/cwe/cwe-078 * external/cwe/cwe-088 + * external/cwe/cwe-088 */ import swift From d9800c7bb63a5246fc77a986e9273a91082deb22 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Thu, 27 Jul 2023 22:45:50 +0200 Subject: [PATCH 7/9] Update CommandInjection.ql --- swift/ql/src/queries/Security/CWE-078/CommandInjection.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql index eb501556b290..148676e1ac38 100644 --- a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql +++ b/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql @@ -9,7 +9,6 @@ * @tags security * external/cwe/cwe-078 * external/cwe/cwe-088 - * external/cwe/cwe-088 */ import swift From 2a49219127c4d0b289901ee8e062049ee560fc17 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Fri, 28 Jul 2023 00:15:33 +0200 Subject: [PATCH 8/9] Move query to experimental --- .../Security/CWE-078/CommandInjection.qhelp | 0 .../Security/CWE-078/CommandInjection.ql | 0 .../Security/CWE-078/CommandInjectionBad.swift | 0 .../Security/CWE-078/CommandInjectionGood.swift | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename swift/ql/src/{queries => experimental}/Security/CWE-078/CommandInjection.qhelp (100%) rename swift/ql/src/{queries => experimental}/Security/CWE-078/CommandInjection.ql (100%) rename swift/ql/src/{queries => experimental}/Security/CWE-078/CommandInjectionBad.swift (100%) rename swift/ql/src/{queries => experimental}/Security/CWE-078/CommandInjectionGood.swift (100%) diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp similarity index 100% rename from swift/ql/src/queries/Security/CWE-078/CommandInjection.qhelp rename to swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjection.ql b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql similarity index 100% rename from swift/ql/src/queries/Security/CWE-078/CommandInjection.ql rename to swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjectionBad.swift b/swift/ql/src/experimental/Security/CWE-078/CommandInjectionBad.swift similarity index 100% rename from swift/ql/src/queries/Security/CWE-078/CommandInjectionBad.swift rename to swift/ql/src/experimental/Security/CWE-078/CommandInjectionBad.swift diff --git a/swift/ql/src/queries/Security/CWE-078/CommandInjectionGood.swift b/swift/ql/src/experimental/Security/CWE-078/CommandInjectionGood.swift similarity index 100% rename from swift/ql/src/queries/Security/CWE-078/CommandInjectionGood.swift rename to swift/ql/src/experimental/Security/CWE-078/CommandInjectionGood.swift From 90ac5b905b89aa92f8b14a97945a3ad0e902ed40 Mon Sep 17 00:00:00 2001 From: Maiky <76447395+maikypedia@users.noreply.github.com> Date: Fri, 28 Jul 2023 00:21:02 +0200 Subject: [PATCH 9/9] --- swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql index 148676e1ac38..98d68d0a1018 100644 --- a/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql +++ b/swift/ql/src/experimental/Security/CWE-078/CommandInjection.ql @@ -19,4 +19,4 @@ 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" + sourceNode.getNode(), "user-provided value" \ No newline at end of file