From 7496b61b8df2a19178b9d85cdcb18edf0a54dae5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 17 Nov 2022 11:47:45 +0100 Subject: [PATCH 1/3] Add rsync since both --rsh and --rsync-path admit commands --- .../semmle/go/frameworks/SystemCommandExecutors.qll | 6 ++++-- .../Security/CWE-078/ArgumentInjection.go | 12 ++++++++++++ .../Security/CWE-078/CommandInjection.expected | 4 ++++ 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go diff --git a/go/ql/lib/semmle/go/frameworks/SystemCommandExecutors.qll b/go/ql/lib/semmle/go/frameworks/SystemCommandExecutors.qll index c2d9929fa54..1e4b7637581 100644 --- a/go/ql/lib/semmle/go/frameworks/SystemCommandExecutors.qll +++ b/go/ql/lib/semmle/go/frameworks/SystemCommandExecutors.qll @@ -20,7 +20,9 @@ private class ShellOrSudoExecution extends SystemCommandExecution::Range, DataFl override DataFlow::Node getCommandName() { result = this.getAnArgument() } - override predicate doubleDashIsSanitizing() { shellCommand.getStringValue().matches("%git") } + override predicate doubleDashIsSanitizing() { + shellCommand.getStringValue().matches("%" + ["git", "rsync"]) + } } private class SystemCommandExecutors extends SystemCommandExecution::Range, DataFlow::CallNode { @@ -126,7 +128,7 @@ private string getASudoCommand() { "fakeroot", "fakeroot-sysv", "su", "fakeroot-tcp", "fstab-decode", "jrunscript", "nohup", "parallel", "find", "pkexec", "sg", "sem", "runcon", "sudoedit", "runuser", "stdbuf", "system", "timeout", "xargs", "time", "awk", "gawk", "mawk", "nawk", "doas", "git", "access", - "vsys", "userv", "sus", "super" + "vsys", "userv", "sus", "super", "rsync" ] } diff --git a/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go b/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go new file mode 100644 index 00000000000..2731b907a4e --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go @@ -0,0 +1,12 @@ +package main + +import ( + "net/http" + "os/exec" +) + +func handler(req *http.Request) { + path := req.URL.Query()["path"][0] + cmd := exec.Command("rsync", path, "/tmp") + cmd.Run() +} diff --git a/go/ql/test/query-tests/Security/CWE-078/CommandInjection.expected b/go/ql/test/query-tests/Security/CWE-078/CommandInjection.expected index 02c56107106..3c93a4657ca 100644 --- a/go/ql/test/query-tests/Security/CWE-078/CommandInjection.expected +++ b/go/ql/test/query-tests/Security/CWE-078/CommandInjection.expected @@ -1,4 +1,5 @@ edges +| ArgumentInjection.go:9:10:9:16 | selection of URL : pointer type | ArgumentInjection.go:10:31:10:34 | path | | 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 | @@ -25,6 +26,8 @@ edges | SanitizingDoubleDash.go:105:15:105:37 | slice literal [array] : string | SanitizingDoubleDash.go:106:24:106:31 | arrayLit | | SanitizingDoubleDash.go:105:30:105:36 | tainted : string | SanitizingDoubleDash.go:105:15:105:37 | slice literal [array] : string | nodes +| ArgumentInjection.go:9:10:9:16 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| ArgumentInjection.go:10:31:10:34 | path | semmle.label | path | | 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 | @@ -55,6 +58,7 @@ nodes | SanitizingDoubleDash.go:152:24:152:30 | tainted | semmle.label | tainted | subpaths #select +| ArgumentInjection.go:10:31:10:34 | path | ArgumentInjection.go:9:10:9:16 | selection of URL : pointer type | ArgumentInjection.go:10:31:10:34 | path | This command depends on a $@. | ArgumentInjection.go:9:10:9:16 | selection of URL | user-provided value | | 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 a $@. | CommandInjection.go:9:13:9:19 | selection of URL | 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 a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | 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 a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | user-provided value | From 69ecbda133885f3ad909cfa7d4ffb196f9c8e41a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 17 Nov 2022 13:06:07 +0100 Subject: [PATCH 2/3] add change note --- go/ql/lib/change-notes/2022-11-17-rsync-argument-injection.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 go/ql/lib/change-notes/2022-11-17-rsync-argument-injection.md diff --git a/go/ql/lib/change-notes/2022-11-17-rsync-argument-injection.md b/go/ql/lib/change-notes/2022-11-17-rsync-argument-injection.md new file mode 100644 index 00000000000..fb39aebe09c --- /dev/null +++ b/go/ql/lib/change-notes/2022-11-17-rsync-argument-injection.md @@ -0,0 +1,4 @@ +--- + category: minorAnalysis +--- + * `rsync` has been added to the list of commands which may evaluate its parameters as a shell command. From 8a27660615eaf3da3b6865405fe04d4e26a33db6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 18 Nov 2022 09:41:30 +0100 Subject: [PATCH 3/3] change handler function name --- go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go b/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go index 2731b907a4e..d38d4662542 100644 --- a/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go +++ b/go/ql/test/query-tests/Security/CWE-078/ArgumentInjection.go @@ -5,7 +5,7 @@ import ( "os/exec" ) -func handler(req *http.Request) { +func handler2(req *http.Request) { path := req.URL.Query()["path"][0] cmd := exec.Command("rsync", path, "/tmp") cmd.Run()