From 83cee4a33402240a43a3165d1c52a17ce74be1a0 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 5 Jan 2021 17:58:58 +0000 Subject: [PATCH] Add 'git' as a possible command-interpreter, unless arguments are sanitized using "--" This is because some git flags can specify arbitrary commands to execute, but its positional arguments cannot, and "--" like in many commands instructs git to consume no further flags. --- ql/src/Security/CWE-078/CommandInjection.ql | 6 +- ql/src/semmle/go/Concepts.qll | 6 + .../go/frameworks/SystemCommandExecutors.qll | 41 ++++- .../semmle/go/security/CommandInjection.qll | 63 ++++++- .../CommandInjectionCustomizations.qll | 11 +- .../CWE-078/CommandInjection.expected | 66 ++++++++ .../Security/CWE-078/GitSubcommands.go | 28 ++++ .../Security/CWE-078/SanitizingDoubleDash.go | 154 ++++++++++++++++++ 8 files changed, 368 insertions(+), 7 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-078/GitSubcommands.go create mode 100644 ql/test/query-tests/Security/CWE-078/SanitizingDoubleDash.go diff --git a/ql/src/Security/CWE-078/CommandInjection.ql b/ql/src/Security/CWE-078/CommandInjection.ql index aea01ed4ce1..d594cdcf4fa 100644 --- a/ql/src/Security/CWE-078/CommandInjection.ql +++ b/ql/src/Security/CWE-078/CommandInjection.ql @@ -14,7 +14,9 @@ import go import semmle.go.security.CommandInjection import DataFlow::PathGraph -from CommandInjection::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink -where cfg.hasFlowPath(source, sink) +from + CommandInjection::Configuration cfg, CommandInjection::DoubleDashSanitizingConfiguration cfg2, + DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) or cfg2.hasFlowPath(source, sink) select sink.getNode(), source, sink, "This command depends on $@.", source.getNode(), "a user-provided value" diff --git a/ql/src/semmle/go/Concepts.qll b/ql/src/semmle/go/Concepts.qll index 38739e0bb5c..4ffa06e3ba9 100644 --- a/ql/src/semmle/go/Concepts.qll +++ b/ql/src/semmle/go/Concepts.qll @@ -21,6 +21,9 @@ class SystemCommandExecution extends DataFlow::Node { /** Gets the argument that specifies the command to be executed. */ DataFlow::Node getCommandName() { result = self.getCommandName() } + + /** Holds if this node is sanitized whenever it follows `--` in an argument list. */ + predicate doubleDashIsSanitizing() { self.doubleDashIsSanitizing() } } /** Provides a class for modeling new system-command execution APIs. */ @@ -35,6 +38,9 @@ module SystemCommandExecution { abstract class Range extends DataFlow::Node { /** Gets the argument that specifies the command to be executed. */ abstract DataFlow::Node getCommandName(); + + /** Holds if this node is sanitized whenever it follows `--` in an argument list. */ + predicate doubleDashIsSanitizing() { none() } } } diff --git a/ql/src/semmle/go/frameworks/SystemCommandExecutors.qll b/ql/src/semmle/go/frameworks/SystemCommandExecutors.qll index 654fa9eb6dc..76e49c8c5bd 100644 --- a/ql/src/semmle/go/frameworks/SystemCommandExecutors.qll +++ b/ql/src/semmle/go/frameworks/SystemCommandExecutors.qll @@ -10,12 +10,17 @@ import go * such as a shell, `sudo`, or a programming-language interpreter. */ private class ShellOrSudoExecution extends SystemCommandExecution::Range, DataFlow::CallNode { + ShellLike shellCommand; + ShellOrSudoExecution() { this instanceof SystemCommandExecution and - this.getAnArgument().getAPredecessor*() instanceof ShellLike + shellCommand = this.getAnArgument().getAPredecessor*() and + not hasSafeSubcommand(shellCommand.getStringValue(), this.getAnArgument().getStringValue()) } override DataFlow::Node getCommandName() { result = getAnArgument() } + + override predicate doubleDashIsSanitizing() { shellCommand.getStringValue().matches("%git") } } private class SystemCommandExecutors extends SystemCommandExecution::Range, DataFlow::CallNode { @@ -151,7 +156,39 @@ private string getASudoCommand() { result = "awk" or result = "gawk" or result = "mawk" or - result = "nawk" + result = "nawk" or + result = "git" +} + +/** + * Excuse git commands other than those that interact with remotes, as only those currently + * take arbitrary commands to run on the remote host as arguments. + */ +bindingset[command, subcommand] +private predicate hasSafeSubcommand(string command, string subcommand) { + command.matches("%git") and + // All git subcommands except for clone, fetch, ls-remote, pull and fetch-pack + subcommand in [ + "add", "am", "archive", "bisect", "branch", "bundle", "checkout", "cherry-pick", "citool", + "clean", "commit", "describe", "diff", "format-patch", "gc", "gitk", "grep", "gui", "init", + "log", "merge", "mv", "notes", "push", "range-diff", "rebase", "reset", "restore", "revert", + "rm", "shortlog", "show", "sparse-checkout", "stash", "status", "submodule", "switch", "tag", + "worktree", "fast-export", "fast-import", "filter-branch", "mergetool", "pack-refs", "prune", + "reflog", "remote", "repack", "replace", "annotate", "blame", "bugreport", "count-objects", + "difftool", "fsck", "gitweb", "help", "instaweb", "merge-tree", "rerere", "show-branch", + "verify-commit", "verify-tag", "whatchanged", "archimport", "cvsexportcommit", "cvsimport", + "cvsserver", "imap-send", "p4", "quiltimport", "request-pull", "send-email", "apply", + "checkout-index", "commit-graph", "commit-tree", "hash-object", "index-pack", "merge-file", + "merge-index", "mktag", "mktree", "multi-pack-index", "pack-objects", "prune-packed", + "read-tree", "symbolic-ref", "unpack-objects", "update-index", "update-ref", "write-tree", + "cat-file", "cherry", "diff-files", "diff-index", "diff-tree", "for-each-ref", + "get-tar-commit-id", "ls-files", "ls-tree", "merge-base", "name-rev", "pack-redundant", + "rev-list", "rev-parse", "show-index", "show-ref", "unpack-file", "var", "verify-pack", + "http-backend", "send-pack", "update-server-info", "check-attr", "check-ignore", + "check-mailmap", "check-ref-format", "column", "credential", "credential-cache", + "credential-store", "fmt-merge-msg", "interpret-trailers", "mailinfo", "mailsplit", + "merge-one-file", "patch-id" + ] } /** diff --git a/ql/src/semmle/go/security/CommandInjection.qll b/ql/src/semmle/go/security/CommandInjection.qll index fe0b9706ff9..13963a2c546 100644 --- a/ql/src/semmle/go/security/CommandInjection.qll +++ b/ql/src/semmle/go/security/CommandInjection.qll @@ -24,7 +24,9 @@ module CommandInjection { override predicate isSource(DataFlow::Node source) { source instanceof Source } - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + override predicate isSink(DataFlow::Node sink) { + exists(Sink s | sink = s | not s.doubleDashIsSanitizing()) + } override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or @@ -35,4 +37,63 @@ module CommandInjection { guard instanceof SanitizerGuard } } + + private class ArgumentArrayWithDoubleDash extends DataFlow::Node { + int doubleDashIndex; + + ArgumentArrayWithDoubleDash() { + // Call whose argument list contains a "--" + exists(DataFlow::CallNode c | + this = c and + (c = Builtin::append().getACall() or c = any(SystemCommandExecution sce)) and + c.getArgument(doubleDashIndex).getStringValue() = "--" + ) + or + // array/slice literal containing a "--" + exists(ArrayOrSliceLit litExpr | + this = DataFlow::exprNode(litExpr) and + litExpr.getElement(doubleDashIndex).getStringValue() = "--" + ) + or + // call consuming an existing an array with a "--" + exists(ArgumentArrayWithDoubleDash alreadyHasDoubleDash, DataFlow::CallNode userCall | + ( + alreadyHasDoubleDash.getType().getUnderlyingType() instanceof ArrayType or + alreadyHasDoubleDash.getType() instanceof SliceType + ) and + this = userCall and + DataFlow::localFlow(alreadyHasDoubleDash, userCall.getArgument(doubleDashIndex)) + ) + } + + DataFlow::Node getASanitizedElement() { + exists(int sanitizedIndex | + sanitizedIndex > doubleDashIndex and + ( + result = this.(DataFlow::CallNode).getArgument(sanitizedIndex) or + result = DataFlow::exprNode(this.asExpr().(ArrayOrSliceLit).getElement(sanitizedIndex)) + ) + ) + } + } + + class DoubleDashSanitizingConfiguration extends TaintTracking::Configuration { + DoubleDashSanitizingConfiguration() { this = "CommandInjectionWithDoubleDashSanitizer" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { + exists(Sink s | sink = s | s.doubleDashIsSanitizing()) + } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof Sanitizer or + node = any(ArgumentArrayWithDoubleDash array).getASanitizedElement() + } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof SanitizerGuard + } + } } diff --git a/ql/src/semmle/go/security/CommandInjectionCustomizations.qll b/ql/src/semmle/go/security/CommandInjectionCustomizations.qll index 5a97331d505..f28f9452014 100644 --- a/ql/src/semmle/go/security/CommandInjectionCustomizations.qll +++ b/ql/src/semmle/go/security/CommandInjectionCustomizations.qll @@ -19,7 +19,10 @@ module CommandInjection { /** * A data flow sink for command-injection vulnerabilities. */ - abstract class Sink extends DataFlow::Node { } + abstract class Sink extends DataFlow::Node { + /** Holds if this node is sanitized whenever it follows `--` in an argument list. */ + predicate doubleDashIsSanitizing() { none() } + } /** * A sanitizer for command-injection vulnerabilities. @@ -38,6 +41,10 @@ module CommandInjection { /** A command name, considered as a taint sink for command injection. */ class CommandNameAsSink extends Sink { - CommandNameAsSink() { this = any(SystemCommandExecution exec).getCommandName() } + SystemCommandExecution exec; + + CommandNameAsSink() { this = exec.getCommandName() } + + override predicate doubleDashIsSanitizing() { exec.doubleDashIsSanitizing() } } } diff --git a/ql/test/query-tests/Security/CWE-078/CommandInjection.expected b/ql/test/query-tests/Security/CWE-078/CommandInjection.expected index 9b193aceccc..dc5af8d9049 100644 --- a/ql/test/query-tests/Security/CWE-078/CommandInjection.expected +++ b/ql/test/query-tests/Security/CWE-078/CommandInjection.expected @@ -1,7 +1,73 @@ edges | CommandInjection.go:9:13:9:19 | selection of URL : pointer type | CommandInjection.go:10:22:10:28 | cmdName | +| GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:12:31:12:37 | tainted | +| GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:13:31:13:37 | tainted | +| GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:14:30:14:36 | tainted | +| GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:15:35:15:41 | tainted | +| GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:16:36:16:42 | tainted | +| SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:14:23:14:33 | slice expression | +| SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:40:23:40:30 | arrayLit | +| SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:54:23:54:30 | arrayLit | +| SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:70:23:70:30 | arrayLit | +| SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:80:23:80:29 | tainted | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:96:24:96:34 | slice expression | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:101:24:101:34 | slice expression | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:106:24:106:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:112:24:112:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:118:24:118:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:124:24:124:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:130:24:130:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:137:24:137:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:144:24:144:31 | arrayLit | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:148:30:148:36 | tainted | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:152:24:152:30 | tainted | nodes | CommandInjection.go:9:13:9:19 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | | CommandInjection.go:10:22:10:28 | cmdName | semmle.label | cmdName | +| GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| GitSubcommands.go:12:31:12:37 | tainted | semmle.label | tainted | +| GitSubcommands.go:13:31:13:37 | tainted | semmle.label | tainted | +| GitSubcommands.go:14:30:14:36 | tainted | semmle.label | tainted | +| GitSubcommands.go:15:35:15:41 | tainted | semmle.label | tainted | +| GitSubcommands.go:16:36:16:42 | tainted | semmle.label | tainted | +| SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| SanitizingDoubleDash.go:14:23:14:33 | slice expression | semmle.label | slice expression | +| SanitizingDoubleDash.go:40:23:40:30 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:54:23:54:30 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:70:23:70:30 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:80:23:80:29 | tainted | semmle.label | tainted | +| SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| SanitizingDoubleDash.go:96:24:96:34 | slice expression | semmle.label | slice expression | +| SanitizingDoubleDash.go:101:24:101:34 | slice expression | semmle.label | slice expression | +| SanitizingDoubleDash.go:106:24:106:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:112:24:112:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:118:24:118:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:124:24:124:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:130:24:130:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:137:24:137:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:144:24:144:31 | arrayLit | semmle.label | arrayLit | +| SanitizingDoubleDash.go:148:30:148:36 | tainted | semmle.label | tainted | +| SanitizingDoubleDash.go:152:24:152:30 | tainted | semmle.label | tainted | #select | CommandInjection.go:10:22:10:28 | cmdName | CommandInjection.go:9:13:9:19 | selection of URL : pointer type | CommandInjection.go:10:22:10:28 | cmdName | This command depends on $@. | CommandInjection.go:9:13:9:19 | selection of URL | a user-provided value | +| GitSubcommands.go:12:31:12:37 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:12:31:12:37 | tainted | This command depends on $@. | GitSubcommands.go:10:13:10:19 | selection of URL | a user-provided value | +| GitSubcommands.go:13:31:13:37 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:13:31:13:37 | tainted | This command depends on $@. | GitSubcommands.go:10:13:10:19 | selection of URL | a user-provided value | +| GitSubcommands.go:14:30:14:36 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:14:30:14:36 | tainted | This command depends on $@. | GitSubcommands.go:10:13:10:19 | selection of URL | a user-provided value | +| GitSubcommands.go:15:35:15:41 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:15:35:15:41 | tainted | This command depends on $@. | GitSubcommands.go:10:13:10:19 | selection of URL | a user-provided value | +| GitSubcommands.go:16:36:16:42 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL : pointer type | GitSubcommands.go:16:36:16:42 | tainted | This command depends on $@. | GitSubcommands.go:10:13:10:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:14:23:14:33 | slice expression | SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:14:23:14:33 | slice expression | This command depends on $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:40:23:40:30 | arrayLit | SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:40:23:40:30 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:54:23:54:30 | arrayLit | SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:54:23:54:30 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:70:23:70:30 | arrayLit | SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:70:23:70:30 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:80:23:80:29 | tainted | SanitizingDoubleDash.go:9:13:9:19 | selection of URL : pointer type | SanitizingDoubleDash.go:80:23:80:29 | tainted | This command depends on $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:96:24:96:34 | slice expression | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:96:24:96:34 | slice expression | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:101:24:101:34 | slice expression | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:101:24:101:34 | slice expression | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:106:24:106:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:106:24:106:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:112:24:112:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:112:24:112:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:118:24:118:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:118:24:118:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:124:24:124:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:124:24:124:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:130:24:130:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:130:24:130:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:137:24:137:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:137:24:137:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:144:24:144:31 | arrayLit | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:144:24:144:31 | arrayLit | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:148:30:148:36 | tainted | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:148:30:148:36 | tainted | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | +| SanitizingDoubleDash.go:152:24:152:30 | tainted | SanitizingDoubleDash.go:92:13:92:19 | selection of URL : pointer type | SanitizingDoubleDash.go:152:24:152:30 | tainted | This command depends on $@. | SanitizingDoubleDash.go:92:13:92:19 | selection of URL | a user-provided value | diff --git a/ql/test/query-tests/Security/CWE-078/GitSubcommands.go b/ql/test/query-tests/Security/CWE-078/GitSubcommands.go new file mode 100644 index 00000000000..9d35fbfdb59 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-078/GitSubcommands.go @@ -0,0 +1,28 @@ +package main + +import ( + "net/http" + "os/exec" +) + +// BAD: using git subcommands that are vulnerable to arbitrary remote command execution +func gitSubcommandsBad(req *http.Request) { + tainted := req.URL.Query()["cmd"][0] + + exec.Command("git", "clone", tainted) + exec.Command("git", "fetch", tainted) + exec.Command("git", "pull", tainted) + exec.Command("git", "ls-remote", tainted) + exec.Command("git", "fetch-pack", tainted) +} + +// GOOD: using a sampling of git subcommands that are not vulnerable to arbitrary remote command execution +func gitSubcommandsGood(req *http.Request) { + tainted := req.URL.Query()["cmd"][0] + + exec.Command("git", "checkout", tainted) + exec.Command("git", "branch", tainted) + exec.Command("git", "diff", tainted) + exec.Command("git", "merge", tainted) + exec.Command("git", "add", tainted) +} diff --git a/ql/test/query-tests/Security/CWE-078/SanitizingDoubleDash.go b/ql/test/query-tests/Security/CWE-078/SanitizingDoubleDash.go new file mode 100644 index 00000000000..d69a970f0d0 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-078/SanitizingDoubleDash.go @@ -0,0 +1,154 @@ +package main + +import ( + "net/http" + "os/exec" +) + +func testDoubleDashSanitizes(req *http.Request) { + tainted := req.URL.Query()["cmd"][0] + + // BAD: no sanitizing "--" preceding tainted data + { + arrayLit := [1]string{tainted} + exec.Command("git", arrayLit[:]...) + } + + // GOOD: sanitizing "--" preceding tainted data + { + arrayLit := [2]string{"--", tainted} + exec.Command("git", arrayLit[:]...) + } + + // GOOD: sanitizing "--" preceding tainted data, as a slice + { + arrayLit := []string{"--", tainted} + exec.Command("git", arrayLit...) + } + + // GOOD: sanitizing "--" preceding tainted data, added during an append + { + arrayLit := []string{} + arrayLit = append(arrayLit, "--", tainted) + exec.Command("git", arrayLit...) + } + + // BAD: sanitizing "--" comes after tainted data, added during an append + { + arrayLit := []string{} + arrayLit = append(arrayLit, tainted, "--") + exec.Command("git", arrayLit...) + } + + // GOOD: sanitizing "--" preceding tainted data, built in two steps + { + arrayLit := []string{"--"} + arrayLit = append(arrayLit, tainted) + exec.Command("git", arrayLit...) + } + + // BAD: sanitizing "--" comes after tainted data, built in two steps + { + arrayLit := []string{tainted} + arrayLit = append(arrayLit, "--") + exec.Command("git", arrayLit...) + } + + // GOOD: sanitizing "--" preceding tainted data, built in three steps + { + arrayLit := []string{"--"} + arrayLit = append(arrayLit, "something else") + arrayLit = append(arrayLit, tainted) + exec.Command("git", arrayLit...) + } + + // BAD: sanitizing "--" preceding tainted data, built in three steps + { + arrayLit := []string{"something else"} + arrayLit = append(arrayLit, tainted) + arrayLit = append(arrayLit, "--") + exec.Command("git", arrayLit...) + } + + // GOOD: sanitizing "--" preceding tainted data, used directly in a Command + { + exec.Command("git", "--", tainted) + } + + // BAD: sanitizing "--" comes after tainted data, used directly in a Command + { + exec.Command("git", tainted, "--") + } + + // GOOD: sanitizing "--" preceding tainted data, used directly in a Command, after several other arguments + { + exec.Command("git", "--arg1", "--arg2", "--", tainted) + } +} + +// This test mirrors testDoubleDashSanitizes above, but uses sudo instead of git, where "--" is not sanitizing. +// All cases are therefore BAD. +func testDoubleDashIrrelevant(req *http.Request) { + tainted := req.URL.Query()["cmd"][0] + + { + arrayLit := [1]string{tainted} + exec.Command("sudo", arrayLit[:]...) + } + + { + arrayLit := [2]string{"--", tainted} + exec.Command("sudo", arrayLit[:]...) + } + + { + arrayLit := []string{"--", tainted} + exec.Command("sudo", arrayLit...) + } + + { + arrayLit := []string{} + arrayLit = append(arrayLit, "--", tainted) + exec.Command("sudo", arrayLit...) + } + + { + arrayLit := []string{} + arrayLit = append(arrayLit, tainted, "--") + exec.Command("sudo", arrayLit...) + } + + { + arrayLit := []string{"--"} + arrayLit = append(arrayLit, tainted) + exec.Command("sudo", arrayLit...) + } + + { + arrayLit := []string{tainted} + arrayLit = append(arrayLit, "--") + exec.Command("sudo", arrayLit...) + } + + { + arrayLit := []string{"--"} + arrayLit = append(arrayLit, "something else") + arrayLit = append(arrayLit, tainted) + exec.Command("sudo", arrayLit...) + } + + { + arrayLit := []string{"something else"} + arrayLit = append(arrayLit, tainted) + arrayLit = append(arrayLit, "--") + exec.Command("sudo", arrayLit...) + } + + { + exec.Command("sudo", "--", tainted) + } + + { + exec.Command("sudo", tainted, "--") + } +}