From bc9d2bc879d1e246d48cf4b9e18a975c67e1712b Mon Sep 17 00:00:00 2001
From: oittaa <8972248+oittaa@users.noreply.github.com>
Date: Tue, 6 Jan 2026 14:20:23 +0100
Subject: [PATCH] feat(rules): enhance subprocess variable checks (#1453)

* feat(rules): enhance subprocess variable checks

* ast.Object is deprecated
---
 rules/subproc.go          | 76 ++++++++++++++++++++-------------------
 testutils/g204_samples.go | 37 +++++++++++++++++++
 2 files changed, 76 insertions(+), 37 deletions(-)

diff --git a/rules/subproc.go b/rules/subproc.go
index 1e2ceda..7df82e4 100644
--- a/rules/subproc.go
+++ b/rules/subproc.go
@@ -16,6 +16,7 @@ package rules
 
 import (
 	"go/ast"
+	"go/token"
 	"go/types"
 
 	"github.com/securego/gosec/v2"
@@ -31,6 +32,29 @@ func (r *subprocess) ID() string {
 	return r.MetaData.ID
 }
 
+// getEnclosingBodyStart returns the position of the '{' for the innermost function body enclosing the given position.
+// Returns token.NoPos if no enclosing body found.
+func getEnclosingBodyStart(pos token.Pos, ctx *gosec.Context) token.Pos {
+	if ctx.Root == nil {
+		return token.NoPos
+	}
+	var bodyStart token.Pos
+	ast.Inspect(ctx.Root, func(n ast.Node) bool {
+		var body *ast.BlockStmt
+		switch f := n.(type) {
+		case *ast.FuncDecl:
+			body = f.Body
+		case *ast.FuncLit:
+			body = f.Body
+		}
+		if body != nil && body.Pos() <= pos && pos < body.End() && body.Lbrace.IsValid() {
+			bodyStart = body.Lbrace
+		}
+		return true
+	})
+	return bodyStart
+}
+
 // TODO(gm) The only real potential for command injection with a Go project
 // is something like this:
 //
@@ -46,49 +70,27 @@ func (r *subprocess) Match(n ast.Node, c *gosec.Context) (*issue.Issue, error) {
 		if r.isContext(n, c) {
 			args = args[1:]
 		}
-		for _, arg := range args {
+		for i, arg := range args {
 			if ident, ok := arg.(*ast.Ident); ok {
 				obj := c.Info.ObjectOf(ident)
-
-				// need to cast and check whether it is for a variable ?
-				_, variable := obj.(*types.Var)
-
-				// .. indeed it is a variable then processing is different than a normal
-				// field assignment
-				if variable {
-					// skip the check when the declaration is not available
-					if ident.Obj == nil {
-						continue
+				if v, ok := obj.(*types.Var); ok {
+					// Special case: struct fields OR function parameters/receivers used as executable name (i==0) -> skip
+					if i == 0 {
+						if v.IsField() {
+							continue
+						}
+						bodyStart := getEnclosingBodyStart(ident.Pos(), c)
+						if bodyStart != token.NoPos && obj.Pos() < bodyStart {
+							continue // Parameter or receiver (declared before body brace)
+						}
 					}
-					switch ident.Obj.Decl.(type) {
-					case *ast.AssignStmt:
-						_, assignment := ident.Obj.Decl.(*ast.AssignStmt)
-						if variable && assignment {
-							if !gosec.TryResolve(ident, c) {
-								return c.NewIssue(n, r.ID(), "Subprocess launched with variable", issue.Medium, issue.High), nil
-							}
-						}
-					case *ast.Field:
-						_, field := ident.Obj.Decl.(*ast.Field)
-						if variable && field {
-							// check if the variable exist in the scope
-							vv, vvok := obj.(*types.Var)
-
-							if vvok && vv.Parent().Lookup(ident.Name) == nil {
-								return c.NewIssue(n, r.ID(), "Subprocess launched with variable", issue.Medium, issue.High), nil
-							}
-						}
-					case *ast.ValueSpec:
-						_, valueSpec := ident.Obj.Decl.(*ast.ValueSpec)
-						if variable && valueSpec {
-							if !gosec.TryResolve(ident, c) {
-								return c.NewIssue(n, r.ID(), "Subprocess launched with variable", issue.Medium, issue.High), nil
-							}
-						}
+					// For all variables: flag if not resolvable to a constant
+					if !gosec.TryResolve(ident, c) {
+						return c.NewIssue(n, r.ID(), "Subprocess launched with variable", issue.Medium, issue.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]
+				// Non-identifier arguments that cannot be resolved
 				return c.NewIssue(n, r.ID(), "Subprocess launched with a potential tainted input or cmd arguments", issue.Medium, issue.High), nil
 			}
 		}
diff --git a/testutils/g204_samples.go b/testutils/g204_samples.go
index 64c96e2..da05d00 100644
--- a/testutils/g204_samples.go
+++ b/testutils/g204_samples.go
@@ -278,4 +278,41 @@ func main() {
 	_ = exec.Command(exe, args...)
 }
 `}, 0, gosec.NewConfig()},
+	{[]string{`
+package main
+
+import (
+	"os/exec"
+)
+
+// Direct use of a function parameter in exec.Command.
+// This is clearly tainted input (parameter from caller, potentially user-controlled).
+func vulnerable(command string) {
+	// Dangerous pattern: passing unsanitized input to a shell
+	_ = exec.Command("bash", "-c", command)
+}
+
+func main() {
+	// In real scenarios, this could be user input (e.g., via flag, HTTP param, etc.)
+	vulnerable("echo safe")
+}
+`}, 1, gosec.NewConfig()},
+	{[]string{`
+package main
+
+import (
+	"os/exec"
+)
+
+// Indirect use: assign parameter to local variable before use.
+// Included for comparison/regression testing.
+func vulnerable(command string) {
+	cmdStr := command // local assignment
+	_ = exec.Command("bash", "-c", cmdStr)
+}
+
+func main() {
+	vulnerable("echo safe")
+}
+`}, 1, gosec.NewConfig()},
 }