diff --git a/taint/taint.go b/taint/taint.go index 5063b82cc0..7a674230fa 100644 --- a/taint/taint.go +++ b/taint/taint.go @@ -858,67 +858,67 @@ func (a *Analyzer) isParameterTainted(param *ssa.Parameter, fn *ssa.Function, vi } } - // Check if parameter type is a source type. - // This is the ONLY place where type-based source matching should trigger - // automatic taint — because parameters represent data flowing IN from - // external callers we don't control. - if a.isSourceType(param.Type()) { - if paramIdx >= 0 && a.paramTaintCache != nil { - a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true - } - return true - } - - // Use call graph to find callers and check their arguments - if a.callGraph == nil { - return false - } + // ── Step 1: Call-graph check (most precise) ────────────────────────── + // If we can see ALL callers and none pass tainted data, the parameter + // is safe — even if its type is a source type (e.g. *http.Request in a + // wrapper that only receives requests built from hardcoded URLs). + callGraphDefinitive := false + + if a.callGraph != nil && paramIdx >= 0 { + node := a.callGraph.Nodes[fn] + if node != nil && len(node.In) > 0 { + hasReceiver := fn.Signature.Recv() != nil + + edgesChecked := 0 + capped := false + for _, inEdge := range node.In { + if edgesChecked >= maxCallerEdges { + capped = true + break + } - node := a.callGraph.Nodes[fn] - if node == nil { - return false - } + site := inEdge.Site + if site == nil { + continue + } - if paramIdx < 0 { - return false - } + // For interface invocations (IsInvoke) the receiver is in + // Call.Value, not in Args, so argument indices are shifted + // by -1 compared to static method calls. + idx := paramIdx + if hasReceiver && site.Common().IsInvoke() { + idx = paramIdx - 1 + } - // Compute the adjusted index ONCE outside the loop. - adjustedIdx := paramIdx - if fn.Signature.Recv() != nil { - // In SSA, method parameters include the receiver at index 0. - // fn.Params already includes the receiver, so paramIdx is correct - // relative to fn.Params. But call site Args also include the receiver - // at index 0 for bound methods. So we don't need to adjust—the - // indices are already aligned. - // However, for interface method invocations (IsInvoke), the receiver - // is in Call.Value, not Args. We handle that separately below. - adjustedIdx = paramIdx - } - - // Check each caller, capping at maxCallerEdges to avoid combinatorial - // explosion from CHA over-approximation of interface method calls. - edgesChecked := 0 - for _, inEdge := range node.In { - if edgesChecked >= maxCallerEdges { - break - } + callArgs := site.Common().Args + if idx >= 0 && idx < len(callArgs) { + edgesChecked++ + if a.isTainted(callArgs[idx], inEdge.Caller.Func, visited, depth+1) { + if a.paramTaintCache != nil { + a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true + } + return true + } + } + } - site := inEdge.Site - if site == nil { - continue + // If we inspected every valid edge without hitting the cap, + // the call graph gives a definitive answer — skip type fallback. + callGraphDefinitive = !capped && edgesChecked > 0 } + } - callArgs := site.Common().Args - - if adjustedIdx < len(callArgs) { - edgesChecked++ - if a.isTainted(callArgs[adjustedIdx], inEdge.Caller.Func, visited, depth+1) { - if a.paramTaintCache != nil { - a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true - } - return true + // ── Step 2: Type-based fallback ────────────────────────────────────── + // Only apply when the call graph could NOT give a definitive answer: + // - No call graph / no node / no incoming edges (framework-registered + // handlers like http.HandleFunc have no visible callers) + // - We hit the maxCallerEdges cap (incomplete picture) + if !callGraphDefinitive { + if a.isSourceType(param.Type()) { + if paramIdx >= 0 && a.paramTaintCache != nil { + a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true } + return true } } diff --git a/testutils/g701_samples.go b/testutils/g701_samples.go index bc634212fd..6b735d06d3 100644 --- a/testutils/g701_samples.go +++ b/testutils/g701_samples.go @@ -1873,8 +1873,8 @@ execute: } `}, 1, gosec.NewConfig()}, - // Interprocedural with interface implementation - // NOTE: Interface method taint tracking not yet fully supported - documented limitation + // Interprocedural with interface implementation: taint flows through + // interface method call to the concrete implementation's sink. {[]string{` package main @@ -1900,7 +1900,7 @@ func handler(db *sql.DB, r *http.Request) { var executor QueryExecutor = &SimpleExecutor{} executor.Execute(db, query) } -`}, 0, gosec.NewConfig()}, +`}, 1, gosec.NewConfig()}, // Multiple Phi nodes with complex control flow {[]string{` diff --git a/testutils/g704_samples.go b/testutils/g704_samples.go index 8998107b5c..196ca970f5 100644 --- a/testutils/g704_samples.go +++ b/testutils/g704_samples.go @@ -101,4 +101,74 @@ func handler(r *http.Request) { http.Get(target) //nolint:errcheck } `}, 1, gosec.NewConfig()}, + // Wrapper method with hardcoded URL must NOT trigger G704. + // The *http.Request parameter in the wrapper is safe because + // all callers pass a request built from a constant URL. + {[]string{` +package main + +import ( + "context" + "fmt" + "net/http" +) + +type HTTPDoer interface { + Do(req *http.Request) (*http.Response, error) +} + +type NamedClient struct { + HTTPClient *http.Client +} + +func (c *NamedClient) Do(req *http.Request) (*http.Response, error) { + req.Header.Set("User-Agent", "test-agent") + return c.HTTPClient.Do(req) +} + +func doImport(httpDoer HTTPDoer) error { + ctx := context.Background() + req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/import", http.NoBody) + if err != nil { + return fmt.Errorf("creating import POST: %w", err) + } + resp, err := httpDoer.Do(req) + if err != nil { + return fmt.Errorf("performing import POST: %w", err) + } + defer resp.Body.Close() + return nil +} + +func main() { + client := &NamedClient{HTTPClient: http.DefaultClient} + _ = doImport(client) +} +`}, 0, gosec.NewConfig()}, + // Wrapper method with tainted URL MUST trigger G704. + // Two sinks fire: NewRequest (tainted URL arg) and the inner Client.Do + // (tainted request flows through the call graph to the wrapper's parameter). + {[]string{` +package main + +import ( + "net/http" + "os" +) + +type APIClient struct { + HTTPClient *http.Client +} + +func (c *APIClient) Do(req *http.Request) (*http.Response, error) { + return c.HTTPClient.Do(req) +} + +func main() { + client := &APIClient{HTTPClient: http.DefaultClient} + url := os.Getenv("TARGET_URL") + req, _ := http.NewRequest(http.MethodGet, url, nil) + client.Do(req) +} +`}, 2, gosec.NewConfig()}, }