From 709ed1ba6567cf9baeb02790fb0a37cb2a5370ed Mon Sep 17 00:00:00 2001
From: Martin Vrachev <mvrachev@vmware.com>
Date: Mon, 16 Sep 2019 17:15:06 +0300
Subject: [PATCH] Change rule G204 to be less restrictive (#339)

Currently, rule G204 warns you about every single use of the
functions syscall.Exec, os.exec.CommandContext and os.Exec.Command.
This can create false positives and it's not accurate because you can
use those functions with perfectly secure arguments like hardcoded
strings for example.

With this change, G204 will warn you in 3 cases when passing arguments
to a function which starts a new process the arguments:
1) are variables initialized by calling another function
2) are functions
3) are command-line arguments or environmental variables

Closes: https://github.com/securego/gosec/issues/338

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
---
 rules/subproc.go    |  4 +-
 testutils/source.go | 97 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/rules/subproc.go b/rules/subproc.go
index 809d808..c452898 100644
--- a/rules/subproc.go
+++ b/rules/subproc.go
@@ -47,9 +47,11 @@ func (r *subprocess) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
 				if _, ok := obj.(*types.Var); ok && !gosec.TryResolve(ident, c) {
 					return gosec.NewIssue(c, n, r.ID(), "Subprocess launched with variable", gosec.Medium, gosec.High), nil
 				}
+			} else if !gosec.TryResolve(arg, c) {
+				// the arg is not a constant or a variable but instead a function call or os.Args[i]
+				return gosec.NewIssue(c, n, r.ID(), "Subprocess launched with function call as argument or cmd arguments", gosec.Medium, gosec.High), nil
 			}
 		}
-		return gosec.NewIssue(c, n, r.ID(), "Subprocess launching should be audited", gosec.Low, gosec.High), nil
 	}
 	return nil, nil
 }
diff --git a/testutils/source.go b/testutils/source.go
index 750ab8e..0b12d5e 100644
--- a/testutils/source.go
+++ b/testutils/source.go
@@ -614,26 +614,8 @@ func main() {
 
 	// SampleCodeG204 - Subprocess auditing
 	SampleCodeG204 = []CodeSample{{[]string{`
-package main
-import "syscall"
-func main() {
-	syscall.Exec("/bin/cat", []string{ "/etc/passwd" }, nil)
-}`}, 1, gosec.NewConfig()}, {[]string{`
-package main
-import (
-	"log"
-	"os/exec"
-)
-func main() {
-	cmd := exec.Command("sleep", "5")
-	err := cmd.Start()
- 	if err != nil {
-		log.Fatal(err)
-	}
-	log.Printf("Waiting for command to finish...")
-  	err = cmd.Wait()
-  	log.Printf("Command finished with error: %v", err)
-}`}, 1, gosec.NewConfig()}, {[]string{`
+// Calling any function which starts a new process
+// with a function call as an argument is considered a command injection
 package main
 import (
 	"log"
@@ -647,6 +629,23 @@ func main() {
 	}
   	log.Printf("Command finished with error: %v", err)
 }`}, 1, gosec.NewConfig()}, {[]string{`
+// Calling any function which starts a new process with using
+// command line arguments as it's arguments is considered dangerous
+package main
+import (
+	"log"
+	"os"
+	"os/exec"
+)
+func main() {
+	err := exec.CommandContext(os.Args[0], "sleep", "5").Run()
+	if err != nil {
+		log.Fatal(err)
+	}
+	log.Printf("Command finished with error: %v", err)
+}`}, 1, gosec.NewConfig()}, {[]string{`
+// Initializing a local variable using a environmental
+// variable is consider as a dangerous user input
 package main
 import (
 	"log"
@@ -663,7 +662,63 @@ func main() {
 	log.Printf("Waiting for command to finish...")
 	err = cmd.Wait()
 	log.Printf("Command finished with error: %v", err)
-}`}, 1, gosec.NewConfig()}}
+}`}, 1, gosec.NewConfig()}, {[]string{`
+// gosec doesn't have enough context to decide that the
+// command argument of the RunCmd function is harcoded string
+// and that's why it's better to warn the user so he can audit it
+package main
+
+import (
+	"log"
+	"os/exec"
+)
+
+func RunCmd(command string) {
+	cmd := exec.Command(command, "5")
+	err := cmd.Start()
+	if err != nil {
+		log.Fatal(err)
+	}
+	log.Printf("Waiting for command to finish...")
+	err = cmd.Wait()
+}
+
+func main() {
+	RunCmd("sleep")
+}`}, 1, gosec.NewConfig()}, {[]string{`
+// syscall.Exec function called with harcoded arguments
+// shouldn't be consider as a command injection
+package main
+import (
+	"fmt"
+	"syscall"
+)
+func main() {
+	err := syscall.Exec("/bin/cat", []string{"/etc/passwd"}, nil)
+	if err != nil {
+		fmt.Printf("Error: %v\n", err)
+	}
+}`}, 0, gosec.NewConfig()},
+		{[]string{`
+// starting a process with a variable as an argument
+// even if not constant is not considered as dangerous
+// because it has harcoded value
+package main
+import (
+	"log"
+	"os/exec"
+)
+func main() {
+	run := "sleep"
+	cmd := exec.Command(run, "5")
+	err := cmd.Start()
+	if err != nil {
+		log.Fatal(err)
+	}
+	log.Printf("Waiting for command to finish...")
+	err = cmd.Wait()
+	log.Printf("Command finished with error: %v", err)
+}`}, 0, gosec.NewConfig()}}
 
 	// SampleCodeG301 - mkdir permission check
 	SampleCodeG301 = []CodeSample{{[]string{`