Skip to content

Commit 5a12d3d

Browse files
ravisastrykravisastryk
committed
taint mechanism, framework agnostic
1 parent 64da3c2 commit 5a12d3d

File tree

2 files changed

+102
-173
lines changed

2 files changed

+102
-173
lines changed

‎taint/analyzer_internal_test.go‎

Lines changed: 72 additions & 129 deletions
Original file line numberDiff line numberDiff line change
@@ -601,7 +601,7 @@ func f() W { return &B{} }
601601
t.Fatal("no MakeInterface instruction found in function f")
602602
}
603603

604-
// ── isHTTPHandlerSignature ──────────────────────────────────────────────────
604+
// ── mayHaveExternalCallers ──────────────────────────────────────────────────
605605

606606
// makeHTTPTypes builds synthetic net/http.ResponseWriter (interface) and
607607
// net/http.Request (struct) types, matching the real package path "net/http".
@@ -639,157 +639,118 @@ func makeFuncSSA(t *testing.T, name string, sig *types.Signature) *ssa.Function
639639
return fn
640640
}
641641

642-
func TestIsHTTPHandlerSignature(t *testing.T) {
642+
func TestMayHaveExternalCallers(t *testing.T) {
643643
t.Parallel()
644644

645-
_, rw, req := makeHTTPTypes()
646-
ptrReq := types.NewPointer(req)
645+
simpleSig := types.NewSignatureType(nil, nil, nil,
646+
types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])),
647+
nil, false)
647648

648649
cases := []struct {
649650
name string
650-
sig *types.Signature
651+
fn func() *ssa.Function
651652
want bool
652653
}{
653654
{
654-
name: "Handler",
655-
sig: types.NewSignatureType(nil, nil, nil,
656-
types.NewTuple(
657-
types.NewVar(token.NoPos, nil, "w", rw),
658-
types.NewVar(token.NoPos, nil, "r", ptrReq),
659-
), nil, false),
655+
name: "ExportedBareFunc",
656+
fn: func() *ssa.Function {
657+
return makeFuncSSA(t, "Handler", simpleSig)
658+
},
660659
want: true,
661660
},
662661
{
663-
name: "OneParam",
664-
sig: types.NewSignatureType(nil, nil, nil,
665-
types.NewTuple(
666-
types.NewVar(token.NoPos, nil, "r", ptrReq),
667-
), nil, false),
662+
name: "UnexportedBareFunc",
663+
fn: func() *ssa.Function {
664+
return makeFuncSSA(t, "handler", simpleSig)
665+
},
668666
want: false,
669667
},
670668
{
671-
name: "WrongFirstParam",
672-
sig: types.NewSignatureType(nil, nil, nil,
673-
types.NewTuple(
674-
types.NewVar(token.NoPos, nil, "n", types.Typ[types.Int]),
675-
types.NewVar(token.NoPos, nil, "r", ptrReq),
676-
), nil, false),
669+
name: "MethodWithReceiver",
670+
fn: func() *ssa.Function {
671+
recv := types.NewVar(token.NoPos, nil, "s", types.NewPointer(types.NewStruct(nil, nil)))
672+
methodSig := types.NewSignatureType(recv, nil, nil,
673+
types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])),
674+
nil, false)
675+
return makeFuncSSA(t, "Do", methodSig)
676+
},
677677
want: false,
678678
},
679679
{
680-
name: "WrongSecondParam",
681-
sig: types.NewSignatureType(nil, nil, nil,
682-
types.NewTuple(
683-
types.NewVar(token.NoPos, nil, "w", rw),
684-
types.NewVar(token.NoPos, nil, "s", types.Typ[types.String]),
685-
), nil, false),
686-
want: false,
687-
},
688-
{
689-
name: "ThreeParams",
690-
sig: types.NewSignatureType(nil, nil, nil,
691-
types.NewTuple(
692-
types.NewVar(token.NoPos, nil, "w", rw),
693-
types.NewVar(token.NoPos, nil, "r", ptrReq),
694-
types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int]),
695-
), nil, false),
696-
want: false,
697-
},
698-
{
699-
name: "NonPointerRequest",
700-
sig: types.NewSignatureType(nil, nil, nil,
701-
types.NewTuple(
702-
types.NewVar(token.NoPos, nil, "w", rw),
703-
types.NewVar(token.NoPos, nil, "r", req), // non-pointer
704-
), nil, false),
705-
want: false,
706-
},
707-
{
708-
name: "WrongPackageRequest",
709-
sig: func() *types.Signature {
710-
otherPkg := types.NewPackage("mypkg/http", "http")
711-
otherObj := types.NewTypeName(token.NoPos, otherPkg, "Request", nil)
712-
otherReq := types.NewNamed(otherObj, types.NewStruct(nil, nil), nil)
713-
return types.NewSignatureType(nil, nil, nil,
714-
types.NewTuple(
715-
types.NewVar(token.NoPos, nil, "w", rw),
716-
types.NewVar(token.NoPos, nil, "r", types.NewPointer(otherReq)),
717-
), nil, false)
718-
}(),
719-
want: false,
720-
},
721-
{
722-
name: "WrongPackageResponseWriter",
723-
sig: func() *types.Signature {
724-
otherPkg := types.NewPackage("mypkg/http", "http")
725-
otherIface := types.NewInterfaceType(nil, nil)
726-
otherIface.Complete()
727-
otherObj := types.NewTypeName(token.NoPos, otherPkg, "ResponseWriter", nil)
728-
otherRW := types.NewNamed(otherObj, otherIface, nil)
729-
return types.NewSignatureType(nil, nil, nil,
730-
types.NewTuple(
731-
types.NewVar(token.NoPos, nil, "w", otherRW),
732-
types.NewVar(token.NoPos, nil, "r", ptrReq),
733-
), nil, false)
734-
}(),
680+
name: "NilSignature",
681+
fn: func() *ssa.Function {
682+
return &ssa.Function{}
683+
},
735684
want: false,
736685
},
737686
}
738687

739688
for _, tc := range cases {
740-
fn := makeFuncSSA(t, tc.name, tc.sig)
741-
got := isHTTPHandlerSignature(fn)
689+
fn := tc.fn()
690+
got := mayHaveExternalCallers(fn)
742691
if got != tc.want {
743-
t.Errorf("isHTTPHandlerSignature(%s) = %v, want %v", tc.name, got, tc.want)
692+
t.Errorf("mayHaveExternalCallers(%s) = %v, want %v", tc.name, got, tc.want)
744693
}
745694
}
746695
}
747696

748-
func TestIsHTTPHandlerSignatureMethodReturnsFalse(t *testing.T) {
697+
func TestMayHaveExternalCallersClosureReturnsFalse(t *testing.T) {
749698
t.Parallel()
750699

751-
// A method (sig.Recv() != nil) with handler params must still return false.
752-
_, rw, req := makeHTTPTypes()
753-
structType := types.NewStruct(nil, nil)
754-
recvVar := types.NewVar(token.NoPos, nil, "s", types.NewPointer(structType))
755-
sig := types.NewSignatureType(recvVar, nil, nil,
756-
types.NewTuple(
757-
types.NewVar(token.NoPos, nil, "w", rw),
758-
types.NewVar(token.NoPos, nil, "r", types.NewPointer(req)),
759-
), nil, false)
700+
// A closure (fn.Parent() != nil) is never exported, even if its
701+
// synthesized name starts with an uppercase letter.
702+
src := `package p
760703
761-
fn := makeFuncSSA(t, "ServeHTTP", sig)
762-
if isHTTPHandlerSignature(fn) {
763-
t.Error("expected false for method with receiver")
764-
}
704+
func Outer() {
705+
fn := func(x int) { _ = x }
706+
fn(1)
765707
}
708+
`
709+
fset := token.NewFileSet()
710+
parsed, err := parser.ParseFile(fset, "p.go", src, 0)
711+
if err != nil {
712+
t.Fatalf("parse: %v", err)
713+
}
714+
info := &types.Info{
715+
Types: make(map[ast.Expr]types.TypeAndValue), Defs: make(map[*ast.Ident]types.Object),
716+
Uses: make(map[*ast.Ident]types.Object), Implicits: make(map[ast.Node]types.Object),
717+
Scopes: make(map[ast.Node]*types.Scope), Selections: make(map[*ast.SelectorExpr]*types.Selection),
718+
}
719+
pkg, _ := (&types.Config{}).Check("p", fset, []*ast.File{parsed}, info)
720+
prog := ssa.NewProgram(fset, 0)
721+
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
722+
prog.Build()
766723

767-
func TestIsHTTPHandlerSignatureNilSignature(t *testing.T) {
768-
t.Parallel()
769-
fn := &ssa.Function{}
770-
if isHTTPHandlerSignature(fn) {
771-
t.Fatal("expected false for nil Signature")
724+
outer := ssaPkg.Func("Outer")
725+
if outer == nil {
726+
t.Fatal("Outer not found")
727+
}
728+
// Find the anonymous closure inside Outer.
729+
for _, anon := range outer.AnonFuncs {
730+
if mayHaveExternalCallers(anon) {
731+
t.Errorf("mayHaveExternalCallers(closure %s) = true, want false", anon.Name())
732+
}
772733
}
773734
}
774735

775736
// ── isParameterTainted entry-point logic ────────────────────────────────────
776737

777-
func TestIsParameterTaintedHandlerWithCallersStillTainted(t *testing.T) {
738+
func TestIsParameterTaintedExportedFuncWithCallersStillTainted(t *testing.T) {
778739
t.Parallel()
779740

780-
// Build a minimal package where "handler" has the HTTP handler signature
781-
// and "caller" invokes it with locally-built args. We use synthetic
782-
// net/http types embedded via a fake importer.
741+
// An exported bare function with a source-type param must be auto-tainted
742+
// even when it has internal callers with safe args — because external
743+
// callers (framework dispatch) may be invisible to the call graph.
783744
httpPkg, _, _ := makeHTTPTypes()
784745

785746
src := `package p
786747
787748
import "net/http"
788749
789-
func handler(w http.ResponseWriter, r *http.Request) {}
750+
func Handler(w http.ResponseWriter, r *http.Request) {}
790751
791752
func caller() {
792-
handler(nil, nil)
753+
Handler(nil, nil)
793754
}
794755
`
795756
fset := token.NewFileSet()
@@ -823,12 +784,12 @@ func caller() {
823784
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
824785
prog.Build()
825786

826-
handlerFn := ssaPkg.Func("handler")
787+
handlerFn := ssaPkg.Func("Handler")
827788
if handlerFn == nil {
828-
t.Fatal("handler not found")
789+
t.Fatal("Handler not found")
829790
}
830791
if len(handlerFn.Params) < 2 {
831-
t.Fatal("expected handler to have 2 params")
792+
t.Fatal("expected Handler to have 2 params")
832793
}
833794

834795
analyzer := New(&Config{
@@ -843,14 +804,14 @@ func caller() {
843804
}
844805
_ = analyzer.Analyze(prog, srcFuncs)
845806

846-
// handler has callers (caller() calls it).
807+
// Handler has callers (caller() calls it).
847808
node := analyzer.callGraph.Nodes[handlerFn]
848809
if node == nil || len(node.In) == 0 {
849-
t.Fatal("expected handler to have callers in the call graph")
810+
t.Fatal("expected Handler to have callers in the call graph")
850811
}
851812

852813
// Despite callers, isParameterTainted must return true because the
853-
// function matches the HTTP handler signature.
814+
// function is an exported bare function (mayHaveExternalCallers).
854815
visited := make(map[ssa.Value]bool)
855816
tainted := analyzer.isParameterTainted(handlerFn.Params[1], handlerFn, visited, 0)
856817
if !tainted {
@@ -1174,21 +1135,3 @@ func lonely(r *http.Request) {}
11741135
t.Fatal("expected cache hit to return true")
11751136
}
11761137
}
1177-
1178-
func TestIsHTTPHandlerSignatureSecondParamUnnamedPointerElem(t *testing.T) {
1179-
t.Parallel()
1180-
1181-
// When the second param is a pointer to a non-Named type (e.g., *int),
1182-
// the named.Obj() == nil || named.Obj().Pkg() == nil guard must return false.
1183-
_, rw, _ := makeHTTPTypes()
1184-
sig := types.NewSignatureType(nil, nil, nil,
1185-
types.NewTuple(
1186-
types.NewVar(token.NoPos, nil, "w", rw),
1187-
types.NewVar(token.NoPos, nil, "r", types.NewPointer(types.Typ[types.Int])),
1188-
), nil, false)
1189-
1190-
fn := makeFuncSSA(t, "BadSecondParam", sig)
1191-
if isHTTPHandlerSignature(fn) {
1192-
t.Fatal("expected false for *int as second param")
1193-
}
1194-
}

‎taint/taint.go‎

Lines changed: 30 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -811,44 +811,31 @@ func (a *Analyzer) isSourceType(t types.Type) bool {
811811
return false
812812
}
813813

814-
// isHTTPHandlerSignature returns true when fn has the standard net/http
815-
// handler signature: func(http.ResponseWriter, *http.Request).
816-
// These functions are entry points whose caller (net/http framework) may
817-
// not appear in the call graph. Auto-tainting their *http.Request parameter
818-
// must not be suppressed by the presence of internal callers with safe args.
819-
func isHTTPHandlerSignature(fn *ssa.Function) bool {
820-
sig := fn.Signature
821-
if sig == nil || sig.Recv() != nil {
822-
// Methods implementing http.Handler.ServeHTTP are already handled
823-
// correctly through interface dispatch in CHA; only check bare funcs.
824-
return false
825-
}
826-
params := sig.Params()
827-
if params.Len() != 2 {
828-
return false
829-
}
830-
831-
// Second param must be *net/http.Request
832-
p1 := params.At(1).Type()
833-
ptr, ok := p1.(*types.Pointer)
834-
if !ok {
835-
return false
836-
}
837-
named, ok := ptr.Elem().(*types.Named)
838-
if !ok || named.Obj() == nil || named.Obj().Pkg() == nil {
814+
// mayHaveExternalCallers reports whether fn could be invoked by code outside
815+
// the analyzed package — code that is invisible to the call graph.
816+
//
817+
// Exported bare functions (non-methods) are the primary case: frameworks
818+
// register them via dynamic dispatch that CHA cannot resolve, so the call
819+
// graph may lack edges even though the function IS called at runtime.
820+
//
821+
// Methods with a receiver are excluded because CHA resolves interface dispatch
822+
// to concrete methods, so their callers are generally visible in the graph.
823+
// Unexported functions are only callable within the package, and the call graph
824+
// covers intra-package calls comprehensively.
825+
func mayHaveExternalCallers(fn *ssa.Function) bool {
826+
if fn.Signature == nil {
839827
return false
840828
}
841-
if named.Obj().Pkg().Path() != "net/http" || named.Obj().Name() != "Request" {
829+
// Methods — CHA handles interface dispatch; callers are visible.
830+
if fn.Signature.Recv() != nil {
842831
return false
843832
}
844-
845-
// First param must be net/http.ResponseWriter (interface)
846-
p0 := params.At(0).Type()
847-
p0Named, ok := p0.(*types.Named)
848-
if !ok || p0Named.Obj() == nil || p0Named.Obj().Pkg() == nil {
833+
// Closures / anonymous functions are never exported.
834+
if fn.Parent() != nil {
849835
return false
850836
}
851-
return p0Named.Obj().Pkg().Path() == "net/http" && p0Named.Obj().Name() == "ResponseWriter"
837+
// Exported bare function — may be called by external frameworks.
838+
return token.IsExported(fn.Name())
852839
}
853840

854841
// isSourceFuncCall checks if a call invokes a known source function
@@ -913,23 +900,22 @@ func (a *Analyzer) isParameterTainted(param *ssa.Parameter, fn *ssa.Function, vi
913900

914901
node := a.callGraph.Nodes[fn]
915902

916-
// Check if parameter type is a configured source type (e.g., *http.Request).
917-
//
918-
// For wrapper methods like NamedClient.Do(req *http.Request) called only
919-
// with constant URLs, we want to verify taint through callers instead of
920-
// unconditionally auto-tainting. But for HTTP handlers (signature
921-
// func(http.ResponseWriter, *http.Request)), the framework dispatch may
922-
// not be visible in the call graph even when CHA is used, and an internal
923-
// caller with safe args could suppress real external-entry-point taint.
903+
// Check if parameter type is a configured source type.
924904
//
925905
// Strategy:
926906
// 1. No callers in call graph → definite entry point → auto-taint.
927-
// 2. Matches HTTP handler signature → always auto-taint, because the
928-
// framework caller (net/http) may be invisible in the call graph.
929-
// 3. Has callers, not a handler → fall through to caller-based check.
907+
// 2. Exported bare function → may have invisible external callers
908+
// (framework dispatch) → auto-taint to avoid false negatives.
909+
// 3. Has callers, not exported bare func → fall through to caller check.
910+
//
911+
// Case 2 addresses a class of false negatives where an internal caller
912+
// with safe args suppresses taint for an exported entry point that is
913+
// also called externally by a framework (issue #1629 + Barry review).
914+
// Methods are excluded because CHA resolves interface dispatch, making
915+
// their callers visible in the call graph.
930916
if a.isSourceType(param.Type()) {
931917
isEntryPoint := (node == nil || len(node.In) == 0)
932-
if isEntryPoint || isHTTPHandlerSignature(fn) {
918+
if isEntryPoint || mayHaveExternalCallers(fn) {
933919
if paramIdx >= 0 && a.paramTaintCache != nil {
934920
a.paramTaintCache[paramKey{fn: fn, paramIdx: paramIdx}] = true
935921
}

0 commit comments

Comments
 (0)