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

@@ -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
 }

testutils/source.go

@@ -614,26 +614,8 @@ func main() {
 
 	// SampleCodeG204 - Subprocess auditing
 	SampleCodeG204 = []CodeSample{{[]string{`
-package main
+// Calling any function which starts a new process
-import "syscall"
+// with a function call as an argument is considered a command injection
-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{`
 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{`