Skip to content

Commit 1341aea

Browse files
authored
fix(G705): eliminate false positive for non-HTTP io.Writer (#1550)
* fix(G705): eliminate false positive for non-HTTP io.Writer Adds ArgTypeGuards map[int]string to taint.Sink. The XSS analyzer now requires arg[0] of fmt.Fprint* to implement net/http.ResponseWriter. Writing exec pipe output to os.Stdout no longer triggers G705. Fixes: #1548 * improve code coverage
1 parent f2262c8 commit 1341aea

File tree

7 files changed

+564
-9
lines changed

7 files changed

+564
-9
lines changed

analyzers/xss.go

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,37 @@ func XSS() taint.Config {
3636
{Package: "bufio", Name: "Scanner", Pointer: true},
3737
},
3838
Sinks: []taint.Sink{
39+
// Direct write on the response writer itself — receiver already scopes it.
3940
{Package: "net/http", Receiver: "ResponseWriter", Method: "Write"},
40-
// For fmt print functions, Args[0] is writer - skip it, check format and data args
41-
{Package: "fmt", Method: "Fprintf", CheckArgs: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}},
42-
{Package: "fmt", Method: "Fprint", CheckArgs: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}},
43-
{Package: "fmt", Method: "Fprintln", CheckArgs: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}},
44-
{Package: "io", Method: "WriteString", CheckArgs: []int{1}},
41+
// fmt print family: arg[0] is the io.Writer target; args[1..n] are the
42+
// format string and variadic data (all checked for taint).
43+
// Guard: only treat as a sink when arg[0] implements net/http.ResponseWriter.
44+
// Writing to os.Stdout, os.Stderr, bytes.Buffer, exec pipes, etc. is NOT flagged.
45+
{
46+
Package: "fmt",
47+
Method: "Fprintf",
48+
CheckArgs: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
49+
ArgTypeGuards: map[int]string{0: "net/http.ResponseWriter"},
50+
},
51+
{
52+
Package: "fmt",
53+
Method: "Fprint",
54+
CheckArgs: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
55+
ArgTypeGuards: map[int]string{0: "net/http.ResponseWriter"},
56+
},
57+
{
58+
Package: "fmt",
59+
Method: "Fprintln",
60+
CheckArgs: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
61+
ArgTypeGuards: map[int]string{0: "net/http.ResponseWriter"},
62+
},
63+
// io.WriteString: same rationale — only a sink when the writer is HTTP.
64+
{
65+
Package: "io",
66+
Method: "WriteString",
67+
CheckArgs: []int{1},
68+
ArgTypeGuards: map[int]string{0: "net/http.ResponseWriter"},
69+
},
4570
// Template functions that unsafely inject untrusted content
4671
{Package: "html/template", Method: "HTML"},
4772
{Package: "html/template", Method: "HTMLAttr"},

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ require (
5050
go.opentelemetry.io/otel/metric v1.37.0 // indirect
5151
go.opentelemetry.io/otel/trace v1.37.0 // indirect
5252
golang.org/x/mod v0.33.0 // indirect
53-
golang.org/x/net v0.50.0 // indirect
53+
golang.org/x/net v0.51.0 // indirect
5454
golang.org/x/sys v0.41.0 // indirect
5555
google.golang.org/genproto/googleapis/rpc v0.0.0-20250818200422-3122310a409c // indirect
5656
google.golang.org/grpc v1.75.0 // indirect

go.sum

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ cloud.google.com/go/storage v1.6.0/go.mod h1:N7U0C8pVQ/+NIKOBQyamJIeKQKkZ+mxpohl
3939
cloud.google.com/go/storage v1.8.0/go.mod h1:Wv1Oy7z6Yz3DshWRJFhqM/UCfaWIRTdp0RXyy7KQOVs=
4040
contrib.go.opencensus.io/exporter/stackdriver v0.13.4/go.mod h1:aXENhDJ1Y4lIg4EUaVTwzvYETVNZk10Pu26tevFKLUc=
4141
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
42-
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
4342
github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
43+
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
4444
github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo=
4545
github.com/Masterminds/goutils v1.1.0/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU=
4646
github.com/Masterminds/semver v1.4.2/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y=
@@ -515,8 +515,8 @@ golang.org/x/net v0.0.0-20200513185701-a91f0712d120/go.mod h1:qpuaurCH72eLCgpAm/
515515
golang.org/x/net v0.0.0-20200520182314-0ba52f642ac2/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A=
516516
golang.org/x/net v0.0.0-20200625001655-4c5254603344/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA=
517517
golang.org/x/net v0.0.0-20201110031124-69a78807bb2b/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU=
518-
golang.org/x/net v0.50.0 h1:ucWh9eiCGyDR3vtzso0WMQinm2Dnt8cFMuQa9K33J60=
519-
golang.org/x/net v0.50.0/go.mod h1:UgoSli3F/pBgdJBHCTc+tp3gmrU4XswgGRgtnwWTfyM=
518+
golang.org/x/net v0.51.0 h1:94R/GTO7mt3/4wIKpcR5gkGmRLOuE/2hNGeWq/GBIFo=
519+
golang.org/x/net v0.51.0/go.mod h1:aamm+2QF5ogm02fjy5Bb7CQ0WMt1/WVM7FtyaTLlA9Y=
520520
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
521521
golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
522522
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=

taint/analyzer_internal_test.go

Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package taint
22

33
import (
4+
"go/ast"
5+
"go/constant"
46
"go/parser"
57
"go/token"
68
"go/types"
@@ -10,6 +12,7 @@ import (
1012

1113
"golang.org/x/tools/go/analysis"
1214
"golang.org/x/tools/go/analysis/passes/buildssa"
15+
"golang.org/x/tools/go/ssa"
1316

1417
"github.com/securego/gosec/v2/internal/ssautil"
1518
"github.com/securego/gosec/v2/issue"
@@ -195,3 +198,299 @@ func TestNewIssueReturnsEmptyWhenPositionCannotBeResolved(t *testing.T) {
195198
t.Fatalf("expected empty issue for unresolved position, got %+v", iss)
196199
}
197200
}
201+
202+
// ── lookupNamedType ───────────────────────────────────────────────────────────
203+
204+
func TestLookupNamedTypeNoDot(t *testing.T) {
205+
t.Parallel()
206+
// A path with no dot must return nil before touching prog.
207+
if got := lookupNamedType("nodot", nil); got != nil {
208+
t.Fatalf("expected nil for path with no dot, got %v", got)
209+
}
210+
}
211+
212+
func TestLookupNamedTypePackageNotInProgram(t *testing.T) {
213+
t.Parallel()
214+
prog := ssa.NewProgram(token.NewFileSet(), 0)
215+
// Program is empty — the requested package is not present.
216+
if got := lookupNamedType("net/http.ResponseWriter", prog); got != nil {
217+
t.Fatalf("expected nil when package is absent from program, got %v", got)
218+
}
219+
}
220+
221+
func TestLookupNamedTypeFound(t *testing.T) {
222+
t.Parallel()
223+
prog := ssa.NewProgram(token.NewFileSet(), 0)
224+
225+
// Manually construct a net/http package with ResponseWriter in its scope.
226+
httpPkg := types.NewPackage("net/http", "http")
227+
iface := types.NewInterfaceType(nil, nil)
228+
obj := types.NewTypeName(token.NoPos, httpPkg, "ResponseWriter", nil)
229+
_ = types.NewNamed(obj, iface, nil)
230+
httpPkg.Scope().Insert(obj)
231+
httpPkg.MarkComplete()
232+
prog.CreatePackage(httpPkg, nil, nil, false)
233+
234+
got := lookupNamedType("net/http.ResponseWriter", prog)
235+
if got == nil {
236+
t.Fatal("expected non-nil type for known type in program")
237+
}
238+
named, ok := got.(*types.Named)
239+
if !ok {
240+
t.Fatalf("expected *types.Named, got %T", got)
241+
}
242+
if named.Obj().Name() != "ResponseWriter" {
243+
t.Fatalf("expected name ResponseWriter, got %s", named.Obj().Name())
244+
}
245+
}
246+
247+
func TestLookupNamedTypeMemberIsNotTypeName(t *testing.T) {
248+
t.Parallel()
249+
prog := ssa.NewProgram(token.NewFileSet(), 0)
250+
251+
// Insert a Var (not a TypeName) into the package scope.
252+
pkg := types.NewPackage("mylib", "mylib")
253+
varObj := types.NewVar(token.NoPos, pkg, "SomeVar", types.Typ[types.String])
254+
pkg.Scope().Insert(varObj)
255+
pkg.MarkComplete()
256+
prog.CreatePackage(pkg, nil, nil, false)
257+
258+
// SomeVar is a *types.Var, not a *types.TypeName — lookup must return nil.
259+
if got := lookupNamedType("mylib.SomeVar", prog); got != nil {
260+
t.Fatalf("expected nil for non-TypeName member, got %v", got)
261+
}
262+
}
263+
264+
func TestLookupNamedTypeMemberNotInScope(t *testing.T) {
265+
t.Parallel()
266+
prog := ssa.NewProgram(token.NewFileSet(), 0)
267+
268+
// Package with the right path but the requested name is absent from scope.
269+
pkg := types.NewPackage("net/http", "http")
270+
pkg.MarkComplete()
271+
prog.CreatePackage(pkg, nil, nil, false)
272+
273+
// "Missing" is not in scope — exercises the member==nil continue branch.
274+
if got := lookupNamedType("net/http.Missing", prog); got != nil {
275+
t.Fatalf("expected nil for absent type name, got %v", got)
276+
}
277+
}
278+
279+
// ── guardsSatisfied ───────────────────────────────────────────────────────────
280+
281+
func TestGuardsSatisfiedEmptyGuards(t *testing.T) {
282+
t.Parallel()
283+
if !guardsSatisfied(nil, Sink{}, nil) {
284+
t.Fatal("expected true for empty ArgTypeGuards")
285+
}
286+
}
287+
288+
func TestGuardsSatisfiedNilProg(t *testing.T) {
289+
t.Parallel()
290+
sink := Sink{ArgTypeGuards: map[int]string{0: "net/http.ResponseWriter"}}
291+
if !guardsSatisfied(nil, sink, nil) {
292+
t.Fatal("expected true when prog is nil")
293+
}
294+
}
295+
296+
func TestGuardsSatisfiedArgIdxOutOfRange(t *testing.T) {
297+
t.Parallel()
298+
prog := ssa.NewProgram(token.NewFileSet(), 0)
299+
sink := Sink{ArgTypeGuards: map[int]string{0: "net/http.ResponseWriter"}}
300+
// Guard requires arg at index 0 but args slice is empty.
301+
if guardsSatisfied([]ssa.Value{}, sink, prog) {
302+
t.Fatal("expected false when arg index is out of range")
303+
}
304+
}
305+
306+
func TestGuardsSatisfiedRequiredTypeNotFound(t *testing.T) {
307+
t.Parallel()
308+
prog := ssa.NewProgram(token.NewFileSet(), 0)
309+
// Guard refers to a type that is not present in the program.
310+
// The guard must be skipped (continue), so the function returns true.
311+
sink := Sink{ArgTypeGuards: map[int]string{0: "missing/pkg.Type"}}
312+
arg := ssa.NewConst(constant.MakeString("x"), types.Typ[types.String])
313+
if !guardsSatisfied([]ssa.Value{arg}, sink, prog) {
314+
t.Fatal("expected true when required type is not found (guard skipped)")
315+
}
316+
}
317+
318+
func TestGuardsSatisfiedInterfaceNotSatisfied(t *testing.T) {
319+
t.Parallel()
320+
prog := ssa.NewProgram(token.NewFileSet(), 0)
321+
322+
// Build an interface with one method; string doesn't implement it.
323+
pkg := types.NewPackage("io", "io")
324+
sig := types.NewSignatureType(nil, nil, nil, nil, nil, false)
325+
closeMethod := types.NewFunc(token.NoPos, pkg, "Close", sig)
326+
closerIface := types.NewInterfaceType([]*types.Func{closeMethod}, nil)
327+
closerIface.Complete()
328+
obj := types.NewTypeName(token.NoPos, pkg, "Closer", nil)
329+
_ = types.NewNamed(obj, closerIface, nil)
330+
pkg.Scope().Insert(obj)
331+
pkg.MarkComplete()
332+
prog.CreatePackage(pkg, nil, nil, false)
333+
334+
arg := ssa.NewConst(constant.MakeString("x"), types.Typ[types.String])
335+
sink := Sink{ArgTypeGuards: map[int]string{0: "io.Closer"}}
336+
if guardsSatisfied([]ssa.Value{arg}, sink, prog) {
337+
t.Fatal("expected false when arg type does not implement required interface")
338+
}
339+
}
340+
341+
func TestGuardsSatisfiedEmptyInterfaceSatisfied(t *testing.T) {
342+
t.Parallel()
343+
prog := ssa.NewProgram(token.NewFileSet(), 0)
344+
345+
// Empty interface — every type satisfies it.
346+
pkg := types.NewPackage("any/pkg", "pkg")
347+
emptyIface := types.NewInterfaceType(nil, nil)
348+
emptyIface.Complete()
349+
obj := types.NewTypeName(token.NoPos, pkg, "AnyType", nil)
350+
_ = types.NewNamed(obj, emptyIface, nil)
351+
pkg.Scope().Insert(obj)
352+
pkg.MarkComplete()
353+
prog.CreatePackage(pkg, nil, nil, false)
354+
355+
arg := ssa.NewConst(constant.MakeString("x"), types.Typ[types.String])
356+
sink := Sink{ArgTypeGuards: map[int]string{0: "any/pkg.AnyType"}}
357+
if !guardsSatisfied([]ssa.Value{arg}, sink, prog) {
358+
t.Fatal("expected true when arg implements empty interface")
359+
}
360+
}
361+
362+
func TestGuardsSatisfiedConcreteTypeNotSatisfied(t *testing.T) {
363+
t.Parallel()
364+
prog := ssa.NewProgram(token.NewFileSet(), 0)
365+
366+
// Named struct type — string is not identical to it.
367+
pkg := types.NewPackage("myapp", "myapp")
368+
obj := types.NewTypeName(token.NoPos, pkg, "MyStruct", nil)
369+
_ = types.NewNamed(obj, types.NewStruct(nil, nil), nil)
370+
pkg.Scope().Insert(obj)
371+
pkg.MarkComplete()
372+
prog.CreatePackage(pkg, nil, nil, false)
373+
374+
arg := ssa.NewConst(constant.MakeString("x"), types.Typ[types.String])
375+
sink := Sink{ArgTypeGuards: map[int]string{0: "myapp.MyStruct"}}
376+
// string != myapp.MyStruct and string != *myapp.MyStruct → guard not satisfied.
377+
if guardsSatisfied([]ssa.Value{arg}, sink, prog) {
378+
t.Fatal("expected false when arg type does not match required concrete type")
379+
}
380+
}
381+
382+
// ── resolveOriginalType ───────────────────────────────────────────────────────
383+
384+
func TestResolveOriginalTypeDefault(t *testing.T) {
385+
t.Parallel()
386+
// A plain Const value — no ChangeInterface or MakeInterface wrapping.
387+
val := ssa.NewConst(constant.MakeString("test"), types.Typ[types.String])
388+
got := resolveOriginalType(val)
389+
if !types.Identical(got, types.Typ[types.String]) {
390+
t.Fatalf("expected string type, got %v", got)
391+
}
392+
}
393+
394+
func TestAnalyzeSetsProgAndBuildsCallGraph(t *testing.T) {
395+
t.Parallel()
396+
397+
// Build a minimal self-contained package with a local interface W.
398+
// Function f calls w.Write() which is configured as a sink below.
399+
src := `package p
400+
401+
type W interface{ Write([]byte) (int, error) }
402+
type B struct{}
403+
404+
func (b *B) Write(p []byte) (int, error) { return 0, nil }
405+
func f(w W) { w.Write([]byte("hello")) }
406+
`
407+
fset := token.NewFileSet()
408+
parsed, err := parser.ParseFile(fset, "p.go", src, 0)
409+
if err != nil {
410+
t.Fatalf("parse: %v", err)
411+
}
412+
info := &types.Info{
413+
Types: make(map[ast.Expr]types.TypeAndValue),
414+
Defs: make(map[*ast.Ident]types.Object),
415+
Uses: make(map[*ast.Ident]types.Object),
416+
Implicits: make(map[ast.Node]types.Object),
417+
Scopes: make(map[ast.Node]*types.Scope),
418+
Selections: make(map[*ast.SelectorExpr]*types.Selection),
419+
}
420+
pkg, err := (&types.Config{}).Check("p", fset, []*ast.File{parsed}, info)
421+
if err != nil {
422+
t.Fatalf("type-check: %v", err)
423+
}
424+
prog := ssa.NewProgram(fset, ssa.BuilderMode(0))
425+
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
426+
prog.Build()
427+
428+
fn := ssaPkg.Func("f")
429+
if fn == nil {
430+
t.Fatal("SSA function f not found")
431+
}
432+
433+
// Sink matches the invoke call w.Write inside f; ArgTypeGuards left empty
434+
// so guardsSatisfied is reached and returns true without further work.
435+
analyzer := New(&Config{
436+
Sinks: []Sink{
437+
{Package: "p", Receiver: "W", Method: "Write"},
438+
},
439+
})
440+
441+
_ = analyzer.Analyze(prog, []*ssa.Function{fn})
442+
}
443+
444+
func TestResolveOriginalTypeMakeInterface(t *testing.T) {
445+
t.Parallel()
446+
// Build a minimal, self-contained SSA program (no external imports) that
447+
// boxes a concrete *B value into interface W. This exercises the
448+
// *ssa.MakeInterface branch of resolveOriginalType.
449+
src := `package p
450+
451+
type W interface{ Write([]byte) (int, error) }
452+
type B struct{}
453+
454+
func (b *B) Write(p []byte) (int, error) { return 0, nil }
455+
func f() W { return &B{} }
456+
`
457+
fset := token.NewFileSet()
458+
parsed, err := parser.ParseFile(fset, "p.go", src, 0)
459+
if err != nil {
460+
t.Fatalf("parse: %v", err)
461+
}
462+
info := &types.Info{
463+
Types: make(map[ast.Expr]types.TypeAndValue),
464+
Defs: make(map[*ast.Ident]types.Object),
465+
Uses: make(map[*ast.Ident]types.Object),
466+
Implicits: make(map[ast.Node]types.Object),
467+
Scopes: make(map[ast.Node]*types.Scope),
468+
Selections: make(map[*ast.SelectorExpr]*types.Selection),
469+
}
470+
pkg, err := (&types.Config{}).Check("p", fset, []*ast.File{parsed}, info)
471+
if err != nil {
472+
t.Fatalf("type-check: %v", err)
473+
}
474+
prog := ssa.NewProgram(fset, ssa.BuilderMode(0))
475+
ssaPkg := prog.CreatePackage(pkg, []*ast.File{parsed}, info, true)
476+
prog.Build()
477+
478+
fn := ssaPkg.Func("f")
479+
if fn == nil {
480+
t.Fatal("SSA function f not found")
481+
}
482+
for _, blk := range fn.Blocks {
483+
for _, instr := range blk.Instrs {
484+
mi, ok := instr.(*ssa.MakeInterface)
485+
if !ok {
486+
continue
487+
}
488+
got := resolveOriginalType(mi)
489+
if got == nil {
490+
t.Fatal("resolveOriginalType returned nil for MakeInterface")
491+
}
492+
return
493+
}
494+
}
495+
t.Fatal("no MakeInterface instruction found in function f")
496+
}

0 commit comments

Comments
 (0)