Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swift: Add tests and develop command injection query #13906

Merged
merged 12 commits into from
Sep 13, 2023

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Aug 7, 2023

Add tests and develop the command injection query (swift/command-line-injection). I haven't promoted it out of experimental yet, as there are a few more things I still hope to fix when we have all the data flow content work merged.

Currently there's also regression on MRVA (1 result, in vineetchoudhary/Downloader-for-Apple-Developers), a result I believe to be a true positive that the query isn't finding in it's current form. I believe this may be related to the MRVA snapshot for that project being old, i.e. an extractor bug that was fixed recently. So I expect that will fix itself, and if it doesn't I'll address the problem in a follow-up pull request as well.

@maikypedia FYI

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Aug 7, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner August 7, 2023 13:16
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

QHelp previews:

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

System command built from user-controlled sources

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

Recommendation

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.

Example

The following example executes code from user input without sanitizing it first:

var task = Process()
task.launchPath = "/bin/bash"
task.arguments = ["-c", userControlledString] // BAD

task.launch()

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:

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()
}

References

@geoffw0
Copy link
Contributor Author

geoffw0 commented Aug 8, 2023

@d10c would you mind reviewing this? It consists of improvements to the experimental swift/command-line-injection query that was contributed by a user a few weeks ago (though I haven't quite got to the point of promoting it out of experimental yet). There are also some incidental minor improvements to the URL model in this PR (adding .OptionalSome and .CollectionElement content flow tags to indicate when tainted data is inside an Optional or Set).

Copy link
Contributor

@d10c d10c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, left some comments.

var task = Process()
task.launchPath = "/bin/bash"
task.arguments = ["-c", validateCommand(userControlledString)] // GOOD
if let validatedString = validateCommand(userControlledString) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would the query know that validatedString, to which userControlledString flows, is not tainted? I see in the test cases that this specific pattern is currently a false positive. Any ideas what it would take to fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think implementing a barrier for the qualifier of Sequence.contains would do it, the assumption being that such a check is likely to be used to validate the data as in the example above.

I'd like this work to cover all of the injection queries rather than being done ad-hoc.

@@ -52,23 +44,15 @@ edges
| UnsafeWebViewFetch.swift:164:21:164:35 | call to getRemoteData() | UnsafeWebViewFetch.swift:186:25:186:25 | remoteString |
| UnsafeWebViewFetch.swift:164:21:164:35 | call to getRemoteData() | UnsafeWebViewFetch.swift:188:25:188:25 | remoteString |
| UnsafeWebViewFetch.swift:164:21:164:35 | call to getRemoteData() | UnsafeWebViewFetch.swift:197:24:197:37 | .utf8 |
| UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) | UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) [some:0] |
| UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) | UnsafeWebViewFetch.swift:179:52:179:52 | remoteURL |
| UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) | UnsafeWebViewFetch.swift:185:47:185:56 | ...! |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These old non-[some:0] taint steps were unnecessary, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think so. The lines in question (178, 179) are:

	let remoteURL = URL(string: remoteString)
	let remoteURL2 = URL(string: "/path", relativeTo: remoteURL)

The initializer URL.init?(string:) returns an optional, so taint is rightly in the [some:0] enum field of the return value. The old steps were less than ideal.

Notably, nothing in the #select of this test is affected.

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, didn't know you could have .OptionalSome in the CSV. Do you know how this string label "OptionalSome" is related to the actual implementation of the [some:0] ContentSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's a special case in parseContent for OptionalSome, it's just syntactic sugar for EnumElement[some:0] (only cleaner and hopefully less error prone).

@@ -83,3 +48,24 @@ private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node
private class DefaultCommandInjectionSink extends CommandInjectionSink {
DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting pattern: specify domain-specific sinks/sources/summaries as CSV with a custom label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do this for sinks in most queries now. In some cases (such as this one) we define the intended sinks via CSV, in other cases they're defined in QL but a CSV sink like this is provided purely as an extension point for users.

Note that "command-injection" is the same string as is used in cs/command-line-injection and java/command-line-injection. There's been an [imperfect] effort to unify these labels across languages where we can.

Copy link
Contributor

@d10c d10c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: 👍🏻

@geoffw0 geoffw0 merged commit 3bf0d66 into github:main Sep 13, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants