diff --git a/unified/ql/lib/ext/legacy-swift.model.yml b/unified/ql/lib/ext/legacy-swift.model.yml index 30d5be429efc..d262fadcec1d 100644 --- a/unified/ql/lib/ext/legacy-swift.model.yml +++ b/unified/ql/lib/ext/legacy-swift.model.yml @@ -220,6 +220,8 @@ extensions: - ["", "Process", true, "executableURL", "", "", "PostUpdate", "command-injection", "manual"] - ["", "Process", true, "launchPath", "", "", "PostUpdate", "command-injection", "manual"] - ["", "Process", true, "launchedProcess(arguments:launchPath:)", "", "", "Argument[launchPath:,arguments:]", "command-injection", "manual"] + # Calls may omit the defaulted terminationHandler argument. + - ["", "Process", true, "run(_:arguments:)", "", "", "Argument[0,arguments:]", "command-injection", "manual"] - ["", "Process", true, "run(_:arguments:terminationHandler:)", "", "", "Argument[0,arguments:]", "command-injection", "manual"] - ["", "Process", true, "standardError", "", "", "PostUpdate", "command-injection", "manual"] - ["", "Process", true, "standardInput", "", "", "PostUpdate", "command-injection", "manual"] diff --git a/unified/ql/src/queries/security/CWE-078/CommandInjection.qhelp b/unified/ql/src/queries/security/CWE-078/CommandInjection.qhelp new file mode 100644 index 000000000000..9fb39fd64a32 --- /dev/null +++ b/unified/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 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 example executes 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. +
  • +
    +
    diff --git a/unified/ql/src/queries/security/CWE-078/CommandInjection.ql b/unified/ql/src/queries/security/CWE-078/CommandInjection.ql new file mode 100644 index 000000000000..c1268d2bde21 --- /dev/null +++ b/unified/ql/src/queries/security/CWE-078/CommandInjection.ql @@ -0,0 +1,43 @@ +/** + * @name System command built from user-controlled sources + * @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 + * @precision high + * @id unified/swift/command-line-injection + * @tags security + * external/cwe/cwe-078 + * external/cwe/cwe-088 + */ + +import unified + +/** + * A taint configuration for tainted data that reaches a command injection sink. + */ +module CommandInjectionConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { Models::isSource(node, _) } + + predicate isSink(DataFlow::Node node) { + // TODO: Support Process property-write sinks from original Swift's CommandInjectionExtensions.qll. + Models::isSink(node, "command-injection") + } + + predicate isBarrier(DataFlow::Node node) { + // TODO: Port the numeric-type barrier from original Swift's CommandInjectionExtensions.qll. + none() + } + + predicate observeDiffInformedIncrementalMode() { any() } +} + +module CommandInjectionFlow = TaintTracking::Global; + +import CommandInjectionFlow::PathGraph + +from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink +where CommandInjectionFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "This command depends on a $@.", source.getNode(), + "user-provided value" diff --git a/unified/ql/src/queries/security/CWE-078/CommandInjectionBad.swift b/unified/ql/src/queries/security/CWE-078/CommandInjectionBad.swift new file mode 100644 index 000000000000..ffdaaf907caa --- /dev/null +++ b/unified/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/unified/ql/src/queries/security/CWE-078/CommandInjectionGood.swift b/unified/ql/src/queries/security/CWE-078/CommandInjectionGood.swift new file mode 100644 index 000000000000..1773d5b187ba --- /dev/null +++ b/unified/ql/src/queries/security/CWE-078/CommandInjectionGood.swift @@ -0,0 +1,15 @@ +func validateCommand(_ command: String) -> String? { + let allowedCommands = ["ls -l", "pwd", "echo"] + if allowedCommands.contains(command) { + return command + } + return nil +} + +if let validatedString = validateCommand(userControlledString) { + var task = Process() + task.launchPath = "/bin/bash" + task.arguments = ["-c", validatedString] // GOOD + + task.launch() +} diff --git a/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.expected b/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.expected new file mode 100644 index 000000000000..3d8ffa476377 --- /dev/null +++ b/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.expected @@ -0,0 +1,29 @@ +#select +| CommandInjection.swift:150:42:150:61 | userControlledString | CommandInjection.swift:105:40:105:94 | String(...) | CommandInjection.swift:150:42:150:61 | userControlledString | This command depends on a $@. | CommandInjection.swift:105:40:105:94 | String(...) | user-provided value | +| CommandInjection.swift:154:23:154:56 | ... ! | CommandInjection.swift:105:40:105:94 | String(...) | CommandInjection.swift:154:23:154:56 | ... ! | This command depends on a $@. | CommandInjection.swift:105:40:105:94 | String(...) | user-provided value | +| CommandInjection.swift:158:23:158:56 | ... ! | CommandInjection.swift:105:40:105:94 | String(...) | CommandInjection.swift:158:23:158:56 | ... ! | This command depends on a $@. | CommandInjection.swift:105:40:105:94 | String(...) | user-provided value | +| CommandInjection.swift:163:41:163:74 | ... ! | CommandInjection.swift:105:40:105:94 | String(...) | CommandInjection.swift:163:41:163:74 | ... ! | This command depends on a $@. | CommandInjection.swift:105:40:105:94 | String(...) | user-provided value | +edges +| CommandInjection.swift:105:8:105:31 | ... .some(...) [some.0] | CommandInjection.swift:105:12:105:31 | userControlledString | provenance | | +| CommandInjection.swift:105:12:105:31 | userControlledString | CommandInjection.swift:150:42:150:61 | userControlledString | provenance | | +| CommandInjection.swift:105:12:105:31 | userControlledString | CommandInjection.swift:154:35:154:54 | userControlledString | provenance | | +| CommandInjection.swift:105:12:105:31 | userControlledString | CommandInjection.swift:158:35:158:54 | userControlledString | provenance | | +| CommandInjection.swift:105:12:105:31 | userControlledString | CommandInjection.swift:163:53:163:72 | userControlledString | provenance | | +| CommandInjection.swift:105:35:105:94 | try? ... [some.0] | CommandInjection.swift:105:8:105:31 | ... .some(...) [some.0] | provenance | | +| CommandInjection.swift:105:40:105:94 | String(...) | CommandInjection.swift:105:35:105:94 | try? ... [some.0] | provenance | | +| CommandInjection.swift:154:35:154:54 | userControlledString | CommandInjection.swift:154:23:154:56 | ... ! | provenance | | +| CommandInjection.swift:158:35:158:54 | userControlledString | CommandInjection.swift:158:23:158:56 | ... ! | provenance | | +| CommandInjection.swift:163:53:163:72 | userControlledString | CommandInjection.swift:163:41:163:74 | ... ! | provenance | | +nodes +| CommandInjection.swift:105:8:105:31 | ... .some(...) [some.0] | semmle.label | ... .some(...) [some.0] | +| CommandInjection.swift:105:12:105:31 | userControlledString | semmle.label | userControlledString | +| CommandInjection.swift:105:35:105:94 | try? ... [some.0] | semmle.label | try? ... [some.0] | +| CommandInjection.swift:105:40:105:94 | String(...) | semmle.label | String(...) | +| CommandInjection.swift:150:42:150:61 | userControlledString | semmle.label | userControlledString | +| CommandInjection.swift:154:23:154:56 | ... ! | semmle.label | ... ! | +| CommandInjection.swift:154:35:154:54 | userControlledString | semmle.label | userControlledString | +| CommandInjection.swift:158:23:158:56 | ... ! | semmle.label | ... ! | +| CommandInjection.swift:158:35:158:54 | userControlledString | semmle.label | userControlledString | +| CommandInjection.swift:163:41:163:74 | ... ! | semmle.label | ... ! | +| CommandInjection.swift:163:53:163:72 | userControlledString | semmle.label | userControlledString | +subpaths diff --git a/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.qlref b/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.qlref new file mode 100644 index 000000000000..330ea2d5bede --- /dev/null +++ b/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.qlref @@ -0,0 +1,3 @@ +query: queries/security/CWE-078/CommandInjection.ql +postprocess: + - utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file diff --git a/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.swift b/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.swift new file mode 100644 index 000000000000..b822e8a258a8 --- /dev/null +++ b/unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.swift @@ -0,0 +1,222 @@ + +// --- stubs --- + +struct URL +{ + init?(string: String) {} + init(fileURLWithPath: String) {} +} + +class NSObject { +} + +class Process : NSObject { + var launchPath: String? { get { nil } set {} } + var arguments: [String]? { get { nil } set {} } + func launch() {} + + var executableURL: URL? { get { nil } set {} } + func run() throws {} + + class func launchedProcess(launchPath path: String, arguments: [String]) -> Process { + return Process() + } + + class func run(_ url: URL, arguments: [String], terminationHandler: (@Sendable (Process) -> Void)? = nil) throws -> Process { + return Process() + } +} + +extension String { + init(contentsOf url: URL) throws { + self.init("") + } +} + +class NSUserScriptTask : NSObject { + typealias CompletionHandler = @Sendable (Error?) -> Void + + init(url: URL) throws {} + func execute(completionHandler handler: NSUserScriptTask.CompletionHandler? = nil) { } +} + +class NSUserUnixTask : NSUserScriptTask { + func execute(withArguments arguments: [String]?, completionHandler handler: NSUserScriptTask.CompletionHandler? = nil) { } +} + +protocol NSSecureCoding { +} + +class NSUserAutomatorTask : NSUserScriptTask { + func execute(withInput input: NSSecureCoding?, completionHandler handler: NSUserScriptTask.CompletionHandler? = nil) { } + + var variables: [String: Any]? { get { return nil } set { } } +} + +class FileManager : NSObject { + class var `default`: FileManager { get { return 0 as! FileManager } } + + func contentsOfDirectory(atPath path: String) throws -> [String] { [] } +} + +// --- tests --- + +func validateCommand(_ command: String) -> String? { + let allowedCommands = ["ls -l", "pwd", "echo"] + + if allowedCommands.contains(command) { + return command + } + + return nil +} + +func testCommandInjectionQhelpExamples() { + guard let userControlledString = try? String(contentsOf: URL(string: "http://example.com/")!) else { // $ MISSING: Source + return + } + + let task1 = Process() + task1.launchPath = "/bin/bash" // GOOD + task1.arguments = ["-c", userControlledString] // $ MISSING: Alert + task1.launch() + + if let validatedString = validateCommand(userControlledString) { + let task2 = Process() + task2.launchPath = "/bin/bash" // GOOD + task2.arguments = ["-c", validatedString] // GOOD (original Swift false positive) + task2.launch() + } +} + +func mkProcess() -> Process? { + return Process() +} + +class MyProcess : Process { + var harmlessField: String? + + func setArguments(_ arguments: [String]) { + self.arguments = arguments // $ MISSING: Alert + } +} + +func testCommandInjectionMore(mySafeString: String) { + guard let userControlledString = try? String(contentsOf: URL(string: "http://example.com/")!) else { // $ Source + return + } + + let task1 = Process() + task1.executableURL = URL(string: mySafeString)! // GOOD + task1.arguments = ["abc"] // GOOD + try! task1.run() + + let task2 = Process() + task2.executableURL = URL(fileURLWithPath: mySafeString) // GOOD + task2.arguments = ["abc", "def"] // GOOD + try! task2.run() + + let task3 = Process() + task3.executableURL = URL(string: userControlledString)! // $ MISSING: Alert + task3.arguments = ["abc", userControlledString] // $ MISSING: Alert + try! task3.run() + + let task4 = Process() + task4.executableURL = URL(fileURLWithPath: userControlledString) // $ MISSING: Alert + task4.executableURL = URL(string: userControlledString)! // $ MISSING: Alert + task4.arguments = ["abc", "def" + userControlledString] // $ MISSING: Alert + try! task4.run() + + let task5 = mkProcess() + task5?.executableURL = URL(fileURLWithPath: userControlledString) // $ MISSING: Alert + task5?.arguments = [userControlledString] // $ MISSING: Alert + try! task5?.run() + + let task6 = MyProcess() + task6.executableURL = URL(fileURLWithPath: userControlledString) // $ MISSING: Alert + task6.executableURL = URL(string: userControlledString)! // $ MISSING: Alert + task6.arguments = [userControlledString] // $ MISSING: Alert + task6.setArguments([userControlledString]) // BAD (expected inside setArguments; currently missing) + task6.harmlessField = userControlledString // GOOD + try! task6.run() + + let task7 = Process() + task7.executableURL = URL(fileURLWithPath: mySafeString) // GOOD + task7.arguments = ["abc", "def"] + task7.arguments?.append(userControlledString) // $ MISSING: Alert // Originally missing + try! task7.run() + + _ = Process.launchedProcess(launchPath: mySafeString, arguments: ["abc", mySafeString]) // GOOD + _ = Process.launchedProcess(launchPath: userControlledString, arguments: ["abc", mySafeString]) // $ Alert + _ = Process.launchedProcess(launchPath: mySafeString, arguments: ["abc", userControlledString]) // $ MISSING: Alert + + _ = try? Process.run(URL(string: mySafeString)!, arguments: ["abc", mySafeString]) // GOOD + _ = try? Process.run(URL(string: userControlledString)!, arguments: ["abc", mySafeString]) // $ Alert + _ = try? Process.run(URL(string: mySafeString)!, arguments: ["abc", userControlledString]) // $ MISSING: Alert + + _ = try? Process.run(URL(string: mySafeString)!, arguments: [], terminationHandler: nil) // GOOD + _ = try? Process.run(URL(string: userControlledString)!, arguments: [], terminationHandler: nil) // $ Alert + + let task8 = try! NSUserScriptTask(url: URL(string: mySafeString)!) // GOOD + task8.execute() + + let task9 = try! NSUserScriptTask(url: URL(string: userControlledString)!) // $ Alert + task9.execute() + + let task10 = try! NSUserUnixTask(url: URL(string: userControlledString)!) // $ MISSING: Alert + task10.execute(withArguments: [userControlledString]) // $ MISSING: Alert + + let task11 = try! NSUserAutomatorTask(url: URL(string: userControlledString)!) // $ MISSING: Alert + task11.variables = ["abc": userControlledString] // $ MISSING: Alert // Originally missing + task11.execute(withInput: nil) + + let files = try! FileManager.default.contentsOfDirectory(atPath: "some/directory") + for file in files { + let task12 = Process() + task12.launchPath = "/bin/rm" // GOOD + task12.arguments = [file] // GOOD (cases like this vary, but our analysis doesn't work well on them) + task12.launch() + task12.arguments = files // GOOD (similar to previous) + task12.launch() + task12.arguments = [files[0]] // GOOD (similar to previous) + task12.launch() + } + +} + +struct MyClass { + let p1 : Process + let p2 : Process? + let p3 : Process! + + var myValue: [String]? { + get { + return nil + } + set { + p1.arguments = newValue // $ MISSING: Alert + p2!.arguments = newValue // $ MISSING: Alert + p3.arguments = newValue // $ MISSING: Alert + } + } + + mutating func myFunc() { + guard let userControlledString = try? String(contentsOf: URL(string: "http://example.com/")!) else { // $ MISSING: Source + return + } + + let tainted1 = [userControlledString] + + p1.arguments = tainted1 // $ MISSING: Alert + p2!.arguments = tainted1 // $ MISSING: Alert + p3.arguments = tainted1 // $ MISSING: Alert + + let tainted2 : [String]? = [userControlledString] + + p1.arguments = tainted2 // $ MISSING: Alert + p2!.arguments = tainted2 // $ MISSING: Alert + p3.arguments = tainted2 // $ MISSING: Alert + + myValue = tainted2 + } +}