mirror of
https://github.com/securego/gosec.git
synced 2026-06-20 00:15:59 +02:00
fix(taint): gate *http.Request auto-taint on entry-point detection (#1630)
* fix(taint): gate *http.Request auto-taint on entry-point detection (#1629) isParameterTainted unconditionally tainted any *http.Request parameter by type, even when the function had known callers passing constant-URL requests. Check the CHA call graph first: only auto-taint when the function has no in-edges (true external entry point). When callers exist, fall through to the existing caller-verification loop instead. Fixes #1629 * Address Barry AI Security Analysis * improve code coverage * fix lint * taint mechanism, framework agnostic * address lint warning
This commit is contained in:
@@ -14,6 +14,7 @@ import (
|
||||
|
||||
"golang.org/x/tools/go/analysis"
|
||||
"golang.org/x/tools/go/analysis/passes/buildssa"
|
||||
"golang.org/x/tools/go/callgraph/cha"
|
||||
"golang.org/x/tools/go/ssa"
|
||||
|
||||
"github.com/securego/gosec/v2/internal/ssautil"
|
||||
@@ -599,3 +600,538 @@ func f() W { return &B{} }
|
||||
}
|
||||
t.Fatal("no MakeInterface instruction found in function f")
|
||||
}
|
||||
|
||||
// ── mayHaveExternalCallers ──────────────────────────────────────────────────
|
||||
|
||||
// makeHTTPPkg builds a synthetic net/http package with ResponseWriter and
|
||||
// Request types, matching the real package path "net/http".
|
||||
// This avoids depending on go/importer which may not resolve stdlib in CI.
|
||||
func makeHTTPPkg() *types.Package {
|
||||
httpPkg := types.NewPackage("net/http", "http")
|
||||
|
||||
// ResponseWriter — named interface.
|
||||
rwIface := types.NewInterfaceType(nil, nil)
|
||||
rwIface.Complete()
|
||||
rwObj := types.NewTypeName(token.NoPos, httpPkg, "ResponseWriter", nil)
|
||||
types.NewNamed(rwObj, rwIface, nil)
|
||||
httpPkg.Scope().Insert(rwObj)
|
||||
|
||||
// Request — named struct.
|
||||
reqObj := types.NewTypeName(token.NoPos, httpPkg, "Request", nil)
|
||||
types.NewNamed(reqObj, types.NewStruct(nil, nil), nil)
|
||||
httpPkg.Scope().Insert(reqObj)
|
||||
|
||||
httpPkg.MarkComplete()
|
||||
return httpPkg
|
||||
}
|
||||
|
||||
// makeFuncSSA creates an ssa.Function with the given signature and optional
|
||||
// receiver, attached to a trivial SSA program. The function has no body.
|
||||
func makeFuncSSA(t *testing.T, name string, sig *types.Signature) *ssa.Function {
|
||||
t.Helper()
|
||||
fset := token.NewFileSet()
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
pkg := types.NewPackage("p", "p")
|
||||
pkg.MarkComplete()
|
||||
ssaPkg := prog.CreatePackage(pkg, nil, nil, false)
|
||||
|
||||
fn := ssaPkg.Prog.NewFunction(name, sig, "test")
|
||||
return fn
|
||||
}
|
||||
|
||||
func TestMayHaveExternalCallers(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
simpleSig := types.NewSignatureType(nil, nil, nil,
|
||||
types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])),
|
||||
nil, false)
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
fn func() *ssa.Function
|
||||
want bool
|
||||
}{
|
||||
{
|
||||
name: "ExportedBareFunc",
|
||||
fn: func() *ssa.Function {
|
||||
return makeFuncSSA(t, "Handler", simpleSig)
|
||||
},
|
||||
want: true,
|
||||
},
|
||||
{
|
||||
name: "UnexportedBareFunc",
|
||||
fn: func() *ssa.Function {
|
||||
return makeFuncSSA(t, "handler", simpleSig)
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "MethodWithReceiver",
|
||||
fn: func() *ssa.Function {
|
||||
recv := types.NewVar(token.NoPos, nil, "s", types.NewPointer(types.NewStruct(nil, nil)))
|
||||
methodSig := types.NewSignatureType(recv, nil, nil,
|
||||
types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])),
|
||||
nil, false)
|
||||
return makeFuncSSA(t, "Do", methodSig)
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "NilSignature",
|
||||
fn: func() *ssa.Function {
|
||||
return &ssa.Function{}
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range cases {
|
||||
fn := tc.fn()
|
||||
got := mayHaveExternalCallers(fn)
|
||||
if got != tc.want {
|
||||
t.Errorf("mayHaveExternalCallers(%s) = %v, want %v", tc.name, got, tc.want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestMayHaveExternalCallersClosureReturnsFalse(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// A closure (fn.Parent() != nil) is never exported, even if its
|
||||
// synthesized name starts with an uppercase letter.
|
||||
src := `package p
|
||||
|
||||
func Outer() {
|
||||
fn := func(x int) { _ = x }
|
||||
fn(1)
|
||||
}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, err := parser.ParseFile(fset, "p.go", src, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("parse: %v", err)
|
||||
}
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue), Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object), Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope), Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, _ := (&types.Config{}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
outer := ssaPkg.Func("Outer")
|
||||
if outer == nil {
|
||||
t.Fatal("Outer not found")
|
||||
}
|
||||
// Find the anonymous closure inside Outer.
|
||||
for _, anon := range outer.AnonFuncs {
|
||||
if mayHaveExternalCallers(anon) {
|
||||
t.Errorf("mayHaveExternalCallers(closure %s) = true, want false", anon.Name())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── isParameterTainted entry-point logic ────────────────────────────────────
|
||||
|
||||
func TestIsParameterTaintedExportedFuncWithCallersStillTainted(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// An exported bare function with a source-type param must be auto-tainted
|
||||
// even when it has internal callers with safe args — because external
|
||||
// callers (framework dispatch) may be invisible to the call graph.
|
||||
httpPkg := makeHTTPPkg()
|
||||
|
||||
src := `package p
|
||||
|
||||
import "net/http"
|
||||
|
||||
func Handler(w http.ResponseWriter, r *http.Request) {}
|
||||
|
||||
func caller() {
|
||||
Handler(nil, nil)
|
||||
}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, err := parser.ParseFile(fset, "p.go", src, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("parse: %v", err)
|
||||
}
|
||||
|
||||
fakeImporter := fakeImporterFunc(func(path string) (*types.Package, error) {
|
||||
if path == "net/http" {
|
||||
return httpPkg, nil
|
||||
}
|
||||
return nil, fmt.Errorf("unknown import %q", path)
|
||||
})
|
||||
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue),
|
||||
Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object),
|
||||
Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope),
|
||||
Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, err := (&types.Config{Importer: fakeImporter}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
if err != nil {
|
||||
t.Fatalf("type-check: %v", err)
|
||||
}
|
||||
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
prog.CreatePackage(httpPkg, nil, nil, false) // register net/http in SSA
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
handlerFn := ssaPkg.Func("Handler")
|
||||
if handlerFn == nil {
|
||||
t.Fatal("Handler not found")
|
||||
}
|
||||
if len(handlerFn.Params) < 2 {
|
||||
t.Fatal("expected Handler to have 2 params")
|
||||
}
|
||||
|
||||
analyzer := New(&Config{
|
||||
Sources: []Source{{Package: "net/http", Name: "Request", Pointer: true}},
|
||||
})
|
||||
|
||||
var srcFuncs []*ssa.Function
|
||||
for _, m := range ssaPkg.Members {
|
||||
if fn, ok := m.(*ssa.Function); ok {
|
||||
srcFuncs = append(srcFuncs, fn)
|
||||
}
|
||||
}
|
||||
_ = analyzer.Analyze(prog, srcFuncs)
|
||||
|
||||
// Handler has callers (caller() calls it).
|
||||
node := analyzer.callGraph.Nodes[handlerFn]
|
||||
if node == nil || len(node.In) == 0 {
|
||||
t.Fatal("expected Handler to have callers in the call graph")
|
||||
}
|
||||
|
||||
// Despite callers, isParameterTainted must return true because the
|
||||
// function is an exported bare function (mayHaveExternalCallers).
|
||||
visited := make(map[ssa.Value]bool)
|
||||
tainted := analyzer.isParameterTainted(handlerFn.Params[1], handlerFn, visited, 0)
|
||||
if !tainted {
|
||||
t.Fatal("expected *http.Request param of HTTP handler to be auto-tainted even with internal callers")
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsParameterTaintedNonHandlerWithCallersNotAutoTainted(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Non-handler function accepting *http.Request with a safe internal caller
|
||||
// must NOT be auto-tainted.
|
||||
httpPkg := makeHTTPPkg()
|
||||
|
||||
src := `package p
|
||||
|
||||
import "net/http"
|
||||
|
||||
func wrapper(r *http.Request) {}
|
||||
|
||||
func caller() {
|
||||
wrapper(nil)
|
||||
}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, err := parser.ParseFile(fset, "p.go", src, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("parse: %v", err)
|
||||
}
|
||||
|
||||
fakeImporter := fakeImporterFunc(func(path string) (*types.Package, error) {
|
||||
if path == "net/http" {
|
||||
return httpPkg, nil
|
||||
}
|
||||
return nil, fmt.Errorf("unknown import %q", path)
|
||||
})
|
||||
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue),
|
||||
Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object),
|
||||
Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope),
|
||||
Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, err := (&types.Config{Importer: fakeImporter}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
if err != nil {
|
||||
t.Fatalf("type-check: %v", err)
|
||||
}
|
||||
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
prog.CreatePackage(httpPkg, nil, nil, false) // register net/http in SSA
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
wrapperFn := ssaPkg.Func("wrapper")
|
||||
if wrapperFn == nil {
|
||||
t.Fatal("wrapper not found")
|
||||
}
|
||||
if len(wrapperFn.Params) < 1 {
|
||||
t.Fatal("expected wrapper to have at least 1 param")
|
||||
}
|
||||
|
||||
analyzer := New(&Config{
|
||||
Sources: []Source{{Package: "net/http", Name: "Request", Pointer: true}},
|
||||
})
|
||||
|
||||
var srcFuncs []*ssa.Function
|
||||
for _, m := range ssaPkg.Members {
|
||||
if fn, ok := m.(*ssa.Function); ok {
|
||||
srcFuncs = append(srcFuncs, fn)
|
||||
}
|
||||
}
|
||||
_ = analyzer.Analyze(prog, srcFuncs)
|
||||
|
||||
node := analyzer.callGraph.Nodes[wrapperFn]
|
||||
if node == nil || len(node.In) == 0 {
|
||||
t.Fatal("expected wrapper to have callers in the call graph")
|
||||
}
|
||||
|
||||
visited := make(map[ssa.Value]bool)
|
||||
tainted := analyzer.isParameterTainted(wrapperFn.Params[0], wrapperFn, visited, 0)
|
||||
if tainted {
|
||||
t.Fatal("expected *http.Request param of non-handler wrapper to NOT be auto-tainted when caller is safe")
|
||||
}
|
||||
}
|
||||
|
||||
// fakeImporterFunc adapts a function to the types.Importer interface.
|
||||
type fakeImporterFunc func(path string) (*types.Package, error)
|
||||
|
||||
func (f fakeImporterFunc) Import(path string) (*types.Package, error) { return f(path) }
|
||||
|
||||
func TestIsParameterTaintedCacheHit(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// When isParameterTainted returns true for a handler param, the result is
|
||||
// cached. A second call for the same param must hit the cache and return
|
||||
// true immediately.
|
||||
httpPkg := makeHTTPPkg()
|
||||
|
||||
src := `package p
|
||||
|
||||
import "net/http"
|
||||
|
||||
func handler(w http.ResponseWriter, r *http.Request) {}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, _ := parser.ParseFile(fset, "p.go", src, 0)
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue), Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object), Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope), Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, _ := (&types.Config{Importer: fakeImporterFunc(func(path string) (*types.Package, error) {
|
||||
if path == "net/http" {
|
||||
return httpPkg, nil
|
||||
}
|
||||
return nil, fmt.Errorf("unknown %q", path)
|
||||
})}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
prog.CreatePackage(httpPkg, nil, nil, false)
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
handlerFn := ssaPkg.Func("handler")
|
||||
reqParam := handlerFn.Params[1]
|
||||
|
||||
analyzer := New(&Config{
|
||||
Sources: []Source{{Package: "net/http", Name: "Request", Pointer: true}},
|
||||
})
|
||||
var srcFuncs []*ssa.Function
|
||||
for _, m := range ssaPkg.Members {
|
||||
if fn, ok := m.(*ssa.Function); ok {
|
||||
srcFuncs = append(srcFuncs, fn)
|
||||
}
|
||||
}
|
||||
_ = analyzer.Analyze(prog, srcFuncs)
|
||||
|
||||
// First call populates cache.
|
||||
visited1 := make(map[ssa.Value]bool)
|
||||
if !analyzer.isParameterTainted(reqParam, handlerFn, visited1, 0) {
|
||||
t.Fatal("first call: expected tainted")
|
||||
}
|
||||
|
||||
// Second call must hit the cache (lines 895-898).
|
||||
visited2 := make(map[ssa.Value]bool)
|
||||
if !analyzer.isParameterTainted(reqParam, handlerFn, visited2, 0) {
|
||||
t.Fatal("second call (cache hit): expected tainted")
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsParameterTaintedNoCallGraph(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// When callGraph is nil, isParameterTainted falls back to type-based
|
||||
// auto-taint for source-typed params and returns false otherwise.
|
||||
httpPkg := makeHTTPPkg()
|
||||
|
||||
src := `package p
|
||||
|
||||
import "net/http"
|
||||
|
||||
func twoParams(r *http.Request, s string) {}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, _ := parser.ParseFile(fset, "p.go", src, 0)
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue), Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object), Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope), Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, _ := (&types.Config{Importer: fakeImporterFunc(func(path string) (*types.Package, error) {
|
||||
if path == "net/http" {
|
||||
return httpPkg, nil
|
||||
}
|
||||
return nil, fmt.Errorf("unknown %q", path)
|
||||
})}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
prog.CreatePackage(httpPkg, nil, nil, false)
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
fn := ssaPkg.Func("twoParams")
|
||||
if fn == nil || len(fn.Params) < 2 {
|
||||
t.Fatal("expected twoParams with 2 params")
|
||||
}
|
||||
|
||||
analyzer := New(&Config{
|
||||
Sources: []Source{{Package: "net/http", Name: "Request", Pointer: true}},
|
||||
})
|
||||
// Do NOT call Analyze — callGraph stays nil.
|
||||
// Initialize paramTaintCache so the cache-store branch is exercised.
|
||||
analyzer.paramTaintCache = make(map[paramKey]bool)
|
||||
|
||||
// Source-type param → auto-taint (and caches result).
|
||||
visited := make(map[ssa.Value]bool)
|
||||
if !analyzer.isParameterTainted(fn.Params[0], fn, visited, 0) {
|
||||
t.Fatal("expected source-type param to be auto-tainted when callGraph is nil")
|
||||
}
|
||||
|
||||
// Verify cache was populated.
|
||||
if !analyzer.paramTaintCache[paramKey{fn: fn, paramIdx: 0}] {
|
||||
t.Fatal("expected cache to contain taint result for param 0")
|
||||
}
|
||||
|
||||
// Non-source-type param → false.
|
||||
visited2 := make(map[ssa.Value]bool)
|
||||
if analyzer.isParameterTainted(fn.Params[1], fn, visited2, 0) {
|
||||
t.Fatal("expected non-source-type param to NOT be tainted when callGraph is nil")
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsParameterTaintedDepthExceeded(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// When recursion depth exceeds maxTaintDepth, isParameterTainted returns false.
|
||||
httpPkg := makeHTTPPkg()
|
||||
|
||||
src := `package p
|
||||
|
||||
import "net/http"
|
||||
|
||||
func handler(w http.ResponseWriter, r *http.Request) {}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, _ := parser.ParseFile(fset, "p.go", src, 0)
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue), Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object), Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope), Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, _ := (&types.Config{Importer: fakeImporterFunc(func(path string) (*types.Package, error) {
|
||||
if path == "net/http" {
|
||||
return httpPkg, nil
|
||||
}
|
||||
return nil, fmt.Errorf("unknown %q", path)
|
||||
})}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
prog.CreatePackage(httpPkg, nil, nil, false)
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
fn := ssaPkg.Func("handler")
|
||||
if fn == nil || len(fn.Params) < 2 {
|
||||
t.Fatal("expected handler with 2 params")
|
||||
}
|
||||
|
||||
analyzer := New(&Config{
|
||||
Sources: []Source{{Package: "net/http", Name: "Request", Pointer: true}},
|
||||
})
|
||||
|
||||
visited := make(map[ssa.Value]bool)
|
||||
// Passing depth > maxTaintDepth (50) → must return false.
|
||||
if analyzer.isParameterTainted(fn.Params[1], fn, visited, maxTaintDepth+1) {
|
||||
t.Fatal("expected false when depth exceeds maxTaintDepth")
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsParameterTaintedEntryPointCacheStoreAndHit(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Exercises the cache-store (line 934) and cache-hit (line 897) branches.
|
||||
// Analyze() sets paramTaintCache to nil on return, so we must invoke
|
||||
// isParameterTainted directly while the cache is live. We do this by
|
||||
// manually initialising the analyzer state the same way Analyze does.
|
||||
httpPkg := makeHTTPPkg()
|
||||
|
||||
src := `package p
|
||||
|
||||
import "net/http"
|
||||
|
||||
func lonely(r *http.Request) {}
|
||||
`
|
||||
fset := token.NewFileSet()
|
||||
parsed, _ := parser.ParseFile(fset, "p.go", src, 0)
|
||||
info := &types.Info{
|
||||
Types: make(map[ast.Expr]types.TypeAndValue), Defs: make(map[*ast.Ident]types.Object),
|
||||
Uses: make(map[*ast.Ident]types.Object), Implicits: make(map[ast.Node]types.Object),
|
||||
Scopes: make(map[ast.Node]*types.Scope), Selections: make(map[*ast.SelectorExpr]*types.Selection),
|
||||
}
|
||||
pkg, _ := (&types.Config{Importer: fakeImporterFunc(func(path string) (*types.Package, error) {
|
||||
if path == "net/http" {
|
||||
return httpPkg, nil
|
||||
}
|
||||
return nil, fmt.Errorf("unknown %q", path)
|
||||
})}).Check("p", fset, []*ast.File{parsed}, info)
|
||||
|
||||
prog := ssa.NewProgram(fset, 0)
|
||||
prog.CreatePackage(httpPkg, nil, nil, false)
|
||||
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
|
||||
prog.Build()
|
||||
|
||||
fn := ssaPkg.Func("lonely")
|
||||
if fn == nil || len(fn.Params) < 1 {
|
||||
t.Fatal("expected lonely with 1 param")
|
||||
}
|
||||
|
||||
analyzer := New(&Config{
|
||||
Sources: []Source{{Package: "net/http", Name: "Request", Pointer: true}},
|
||||
})
|
||||
// Manually set up call graph + cache (same as Analyze does internally).
|
||||
analyzer.callGraph = cha.CallGraph(prog)
|
||||
analyzer.paramTaintCache = make(map[paramKey]bool)
|
||||
analyzer.prog = prog
|
||||
|
||||
// First call: entry point (no callers) + source type → auto-taint + cache store.
|
||||
visited := make(map[ssa.Value]bool)
|
||||
if !analyzer.isParameterTainted(fn.Params[0], fn, visited, 0) {
|
||||
t.Fatal("expected entry-point source-type param to be tainted")
|
||||
}
|
||||
if !analyzer.paramTaintCache[paramKey{fn: fn, paramIdx: 0}] {
|
||||
t.Fatal("expected cache to be populated")
|
||||
}
|
||||
|
||||
// Second call: hits cache (line 897).
|
||||
visited2 := make(map[ssa.Value]bool)
|
||||
if !analyzer.isParameterTainted(fn.Params[0], fn, visited2, 0) {
|
||||
t.Fatal("expected cache hit to return true")
|
||||
}
|
||||
}
|
||||
|
||||
+61
-11
@@ -811,6 +811,33 @@ func (a *Analyzer) isSourceType(t types.Type) bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// mayHaveExternalCallers reports whether fn could be invoked by code outside
|
||||
// the analyzed package — code that is invisible to the call graph.
|
||||
//
|
||||
// Exported bare functions (non-methods) are the primary case: frameworks
|
||||
// register them via dynamic dispatch that CHA cannot resolve, so the call
|
||||
// graph may lack edges even though the function IS called at runtime.
|
||||
//
|
||||
// Methods with a receiver are excluded because CHA resolves interface dispatch
|
||||
// to concrete methods, so their callers are generally visible in the graph.
|
||||
// Unexported functions are only callable within the package, and the call graph
|
||||
// covers intra-package calls comprehensively.
|
||||
func mayHaveExternalCallers(fn *ssa.Function) bool {
|
||||
if fn.Signature == nil {
|
||||
return false
|
||||
}
|
||||
// Methods — CHA handles interface dispatch; callers are visible.
|
||||
if fn.Signature.Recv() != nil {
|
||||
return false
|
||||
}
|
||||
// Closures / anonymous functions are never exported.
|
||||
if fn.Parent() != nil {
|
||||
return false
|
||||
}
|
||||
// Exported bare function — may be called by external frameworks.
|
||||
return token.IsExported(fn.Name())
|
||||
}
|
||||
|
||||
// isSourceFuncCall checks if a call invokes a known source function
|
||||
// (a function explicitly configured as producing tainted data, e.g., os.Getenv).
|
||||
func (a *Analyzer) isSourceFuncCall(call *ssa.Call) bool {
|
||||
@@ -858,23 +885,46 @@ 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 {
|
||||
// No call graph: fall back to type-based auto-taint for source-typed params
|
||||
// (conservative — may produce false positives, but we have no callee info).
|
||||
if a.isSourceType(param.Type()) {
|
||||
if paramIdx >= 0 && a.paramTaintCache != nil {
|
||||
a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true
|
||||
}
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
node := a.callGraph.Nodes[fn]
|
||||
|
||||
// Check if parameter type is a configured source type.
|
||||
//
|
||||
// Strategy:
|
||||
// 1. No callers in call graph → definite entry point → auto-taint.
|
||||
// 2. Exported bare function → may have invisible external callers
|
||||
// (framework dispatch) → auto-taint to avoid false negatives.
|
||||
// 3. Has callers, not exported bare func → fall through to caller check.
|
||||
//
|
||||
// Case 2 addresses a class of false negatives where an internal caller
|
||||
// with safe args suppresses taint for an exported entry point that is
|
||||
// also called externally by a framework (issue #1629 + Barry review).
|
||||
// Methods are excluded because CHA resolves interface dispatch, making
|
||||
// their callers visible in the call graph.
|
||||
if a.isSourceType(param.Type()) {
|
||||
isEntryPoint := (node == nil || len(node.In) == 0)
|
||||
if isEntryPoint || mayHaveExternalCallers(fn) {
|
||||
if paramIdx >= 0 && a.paramTaintCache != nil {
|
||||
a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true
|
||||
}
|
||||
return true
|
||||
}
|
||||
// Has known callers and is not a handler — fall through to verify
|
||||
// taint via those callers.
|
||||
}
|
||||
|
||||
if node == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -100,5 +100,80 @@ func handler(r *http.Request) {
|
||||
target := r.URL.Query().Get("url")
|
||||
http.Get(target) //nolint:errcheck
|
||||
}
|
||||
`}, 1, gosec.NewConfig()},
|
||||
// Issue #1629: NamedClient wrapper delegates to http.Client.Do.
|
||||
// Request built with constant URL — must NOT trigger G704.
|
||||
{[]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
|
||||
}
|
||||
`}, 0, gosec.NewConfig()},
|
||||
// Issue #1629 counterpart: URL from os.Getenv through wrapper MUST still fire.
|
||||
{[]string{`
|
||||
package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"net/http"
|
||||
"os"
|
||||
)
|
||||
|
||||
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) {
|
||||
return c.HTTPClient.Do(req)
|
||||
}
|
||||
|
||||
func doImport(httpDoer HTTPDoer) error {
|
||||
target := os.Getenv("IMPORT_URL")
|
||||
ctx := context.Background()
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPost, target, http.NoBody)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
resp, err := httpDoer.Do(req)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
return nil
|
||||
}
|
||||
`}, 1, gosec.NewConfig()},
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user