Skip to content

Swift: Promote the command injection query out of experimental #14701

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

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Nov 6, 2023

Promote swift/command-line-injection out from experimental. The query was added in #13726 and has evolved a bit while in experimental (in particular #13906, #14357 and #14661 - the latter is yet to be merged but should not conflict with this PR).

@maikypedia FYI

TODO:

  • DCA run
  • docs review
  • team review

@geoffw0 geoffw0 added the Swift label Nov 6, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner November 6, 2023 19:22
Copy link
Contributor

github-actions bot commented Nov 6, 2023

QHelp previews:

swift/ql/src/queries/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 geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Nov 6, 2023
@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 7, 2023

DCA looks good to me.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

One comment, but otherwise this LGTM

subatoi
subatoi previously approved these changes Nov 7, 2023
Copy link
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

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

👍 from Docs

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@geoffw0 geoffw0 merged commit 6b434d1 into github:main Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants