From d3f1980e7aa33e56dd33d6dbe6c66bf4ed057513 Mon Sep 17 00:00:00 2001 From: Delon Wong Her Laang Date: Fri, 28 Sep 2018 15:46:59 +0800 Subject: [PATCH] Fix false positives for SQL string concatenation with constants from another file (#247) * Allow for SQL concatenation of nodes that resolve to literals If node.Y resolves to a literal, it will not be considered as an issue. * Fix typo in comment. * Go through all files in package to resolve that identifier * Refactor code and added comments. * Changed checking to not var or func. * Allow for supporting code for test cases. * Resolve merge conflict changes. --- analyzer.go | 2 + resolve.go | 2 +- rules/rules_test.go | 14 ++++ rules/sql.go | 11 +++- testutils/source.go | 152 ++++++++++++++++++++++++++------------------ 5 files changed, 115 insertions(+), 66 deletions(-) diff --git a/analyzer.go b/analyzer.go index 231b718..ef8bae9 100644 --- a/analyzer.go +++ b/analyzer.go @@ -39,6 +39,7 @@ type Context struct { Comments ast.CommentMap Info *types.Info Pkg *types.Package + PkgFiles []*ast.File Root *ast.File Config map[string]interface{} Imports *ImportTracker @@ -139,6 +140,7 @@ func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error gosec.context.Root = file gosec.context.Info = &pkg.Info gosec.context.Pkg = pkg.Pkg + gosec.context.PkgFiles = pkg.Files gosec.context.Imports = NewImportTracker() gosec.context.Imports.TrackPackages(gosec.context.Pkg.Imports()...) ast.Walk(gosec, file) diff --git a/resolve.go b/resolve.go index b563e7d..3c20dd3 100644 --- a/resolve.go +++ b/resolve.go @@ -56,7 +56,7 @@ func resolveCallExpr(n *ast.CallExpr, c *Context) bool { // TryResolve will attempt, given a subtree starting at some ATS node, to resolve // all values contained within to a known constant. It is used to check for any -// unkown values in compound expressions. +// unknown values in compound expressions. func TryResolve(n ast.Node, c *Context) bool { switch node := n.(type) { case *ast.BasicLit: diff --git a/rules/rules_test.go b/rules/rules_test.go index 0bd0ce9..d617a60 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -28,10 +28,24 @@ var _ = Describe("gosec rules", func() { analyzer = gosec.NewAnalyzer(config, logger) runner = func(rule string, samples []testutils.CodeSample) { analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, rule)).Builders()) + + supportingFiles := []string{} + for _, sample := range samples { + if sample.SupportingCode { + supportingFiles = append(supportingFiles, sample.Code) + } + } + for n, sample := range samples { + if sample.SupportingCode { + continue + } analyzer.Reset() pkg := testutils.NewTestPackage() defer pkg.Close() + for n, supportingCode := range supportingFiles { + pkg.AddFile(fmt.Sprintf("supporting_sample_%d.go", n), supportingCode) + } pkg.AddFile(fmt.Sprintf("sample_%d.go", n), sample.Code) err := pkg.Build() Expect(err).ShouldNot(HaveOccurred()) diff --git a/rules/sql.go b/rules/sql.go index 18d8937..8214047 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -51,10 +51,17 @@ func (s *sqlStrConcat) ID() string { } // see if we can figure out what it is -func (s *sqlStrConcat) checkObject(n *ast.Ident) bool { +func (s *sqlStrConcat) checkObject(n *ast.Ident, c *gosec.Context) bool { if n.Obj != nil { return n.Obj.Kind != ast.Var && n.Obj.Kind != ast.Fun } + + // Try to resolve unresolved identifiers using other files in same package + for _, file := range c.PkgFiles { + if node, ok := file.Scope.Objects[n.String()]; ok { + return node.Kind != ast.Var && node.Kind != ast.Fun + } + } return false } @@ -69,7 +76,7 @@ func (s *sqlStrConcat) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) if _, ok := node.Y.(*ast.BasicLit); ok { return nil, nil // string cat OK } - if second, ok := node.Y.(*ast.Ident); ok && s.checkObject(second) { + if second, ok := node.Y.(*ast.Ident); ok && s.checkObject(second, c) { return nil, nil } return gosec.NewIssue(c, n, s.ID(), s.What, s.Severity, s.Confidence), nil diff --git a/testutils/source.go b/testutils/source.go index 4e502b6..4a692dd 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1,9 +1,13 @@ package testutils // CodeSample encapsulates a snippet of source code that compiles, and how many errors should be detected +// Setting SupportingCode to true means that that snippet will not be scanned against +// the rules, but will be added to the same package with the other snippets. +// See SampleCodeG202 for an example. type CodeSample struct { - Code string - Errors int + Code string + Errors int + SupportingCode bool } var ( @@ -15,7 +19,7 @@ func main() { username := "admin" password := "f62e5bcda4fae4f82370da0c6f20697b8f8447ef" fmt.Println("Doing something with: ", username, password) -}`, 1}, {` +}`, 1, false}, {` // Entropy check should not report this error by default package main import "fmt" @@ -23,21 +27,21 @@ func main() { username := "admin" password := "secret" fmt.Println("Doing something with: ", username, password) -}`, 0}, {` +}`, 0, false}, {` package main import "fmt" var password = "f62e5bcda4fae4f82370da0c6f20697b8f8447ef" func main() { username := "admin" fmt.Println("Doing something with: ", username, password) -}`, 1}, {` +}`, 1, false}, {` package main import "fmt" const password = "f62e5bcda4fae4f82370da0c6f20697b8f8447ef" func main() { username := "admin" fmt.Println("Doing something with: ", username, password) -}`, 1}, {` +}`, 1, false}, {` package main import "fmt" const ( @@ -46,12 +50,12 @@ const ( ) func main() { fmt.Println("Doing something with: ", username, password) -}`, 1}, {` +}`, 1, false}, {` package main var password string func init() { password = "f62e5bcda4fae4f82370da0c6f20697b8f8447ef" -}`, 1}, {` +}`, 1, false}, {` package main const ( ATNStateSomethingElse = 1 @@ -59,14 +63,14 @@ const ( ) func main() { println(ATNStateTokenStart) -}`, 0}, {` +}`, 0, false}, {` package main const ( ATNStateTokenStart = "f62e5bcda4fae4f82370da0c6f20697b8f8447ef" ) func main() { println(ATNStateTokenStart) -}`, 1}} +}`, 1, false}} // SampleCodeG102 code snippets for network binding SampleCodeG102 = []CodeSample{ @@ -83,7 +87,7 @@ func main() { log.Fatal(err) } defer l.Close() -}`, 1}, +}`, 1, false}, // Bind to all networks implicitly (default if host omitted) {` @@ -98,7 +102,7 @@ func main() { log.Fatal(err) } defer l.Close() -}`, 1}, +}`, 1, false}, } // SampleCodeG103 find instances of unsafe blocks for auditing purposes SampleCodeG103 = []CodeSample{ @@ -120,7 +124,7 @@ func main() { addressHolder := uintptr(unsafe.Pointer(intPtr)) + unsafe.Sizeof(intArray[0]) intPtr = (*int)(unsafe.Pointer(addressHolder)) fmt.Printf("\nintPtr=%p, *intPtr=%d.\n\n", intPtr, *intPtr) -}`, 3}} +}`, 3, false}} // SampleCodeG104 finds errors that aren't being handled SampleCodeG104 = []CodeSample{ @@ -133,7 +137,7 @@ func test() (int,error) { func main() { v, _ := test() fmt.Println(v) -}`, 1}, {` +}`, 1, false}, {` package main import ( "io/ioutil" @@ -155,7 +159,7 @@ func main() { a() b() c() -}`, 3}, {` +}`, 3, false}, {` package main import "fmt" func test() error { @@ -164,7 +168,7 @@ func test() error { func main() { e := test() fmt.Println(e) -}`, 0}} +}`, 0, false}} // SampleCodeG105 - bignum overflow SampleCodeG105 = []CodeSample{{` @@ -181,7 +185,7 @@ func main() { m := new(big.Int) m = m.SetUint64(0) z = z.Exp(x, y, m) -}`, 1}} +}`, 1, false}} // SampleCodeG106 - ssh InsecureIgnoreHostKey SampleCodeG106 = []CodeSample{{` @@ -191,7 +195,7 @@ import ( ) func main() { _ = ssh.InsecureIgnoreHostKey() -}`, 1}} +}`, 1, false}} // SampleCodeG107 - SSRF via http requests with variable url SampleCodeG107 = []CodeSample{{` @@ -214,7 +218,7 @@ func main() { panic(err) } fmt.Printf("%s", body) -}`, 1}, {` +}`, 1, false}, {` package main import ( @@ -228,7 +232,7 @@ func main() { fmt.Println(err) } fmt.Println(resp.Status) -}`, 0}} +}`, 0, false}} // SampleCodeG201 - SQL injection via format string SampleCodeG201 = []CodeSample{ {` @@ -252,7 +256,7 @@ func main(){ panic(err) } defer rows.Close() -}`, 1}, {` +}`, 1, false}, {` // Format string false positive, safe string spec. package main import ( @@ -273,7 +277,7 @@ func main(){ panic(err) } defer rows.Close() -}`, 0}, { +}`, 0, false}, { ` // Format string false positive package main @@ -292,7 +296,7 @@ func main(){ panic(err) } defer rows.Close() -}`, 0}, {` +}`, 0, false}, {` // Format string false positive, quoted formatter argument. package main import ( @@ -313,7 +317,7 @@ func main(){ panic(err) } defer rows.Close() -}`, 0}} +}`, 0, false}} // SampleCodeG202 - SQL query string building via string concatenation SampleCodeG202 = []CodeSample{ @@ -334,7 +338,7 @@ func main(){ panic(err) } defer rows.Close() -}`, 1}, {` +}`, 1, false}, {` // false positive package main import ( @@ -344,15 +348,15 @@ import ( var staticQuery = "SELECT * FROM foo WHERE age < " func main(){ db, err := sql.Open("sqlite3", ":memory:") - if err != nil { + if err != nil { panic(err) } rows, err := db.Query(staticQuery + "32") if err != nil { panic(err) } - defer rows.Close() -}`, 0}, {` + defer rows.Close() +}`, 0, false}, {` package main import ( "database/sql" @@ -371,7 +375,29 @@ func main(){ } defer rows.Close() } -`, 0}} +`, 0, false}, {` +package main +const gender = "M" +`, 0, true}, {` +package main +import ( + "database/sql" + //_ "github.com/mattn/go-sqlite3" +) +const age = "32" +var staticQuery = "SELECT * FROM foo WHERE age < " +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + rows, err := db.Query("SELECT * FROM foo WHERE gender = " + gender) + if err != nil { + panic(err) + } + defer rows.Close() +} +`, 0, false}} // SampleCodeG203 - Template checks SampleCodeG203 = []CodeSample{ @@ -391,7 +417,7 @@ func main() { "Body": template.HTML(""), } t.Execute(os.Stdout, v) -}`, 0}, { +}`, 0, false}, { ` // Using a variable to initialize could potentially be dangerous. Under the // current model this will likely produce some false positives. @@ -409,7 +435,7 @@ func main() { "Body": template.HTML(a), } t.Execute(os.Stdout, v) -}`, 1}, { +}`, 1, false}, { ` package main import ( @@ -425,7 +451,7 @@ func main() { "Body": template.JS(a), } t.Execute(os.Stdout, v) -}`, 1}, { +}`, 1, false}, { ` package main import ( @@ -441,7 +467,7 @@ func main() { "Body": template.URL(a), } t.Execute(os.Stdout, v) -}`, 1}} +}`, 1, false}} // SampleCodeG204 - Subprocess auditing SampleCodeG204 = []CodeSample{{` @@ -449,7 +475,7 @@ package main import "syscall" func main() { syscall.Exec("/bin/cat", []string{ "/etc/passwd" }, nil) -}`, 1}, {` +}`, 1, false}, {` package main import ( "log" @@ -464,7 +490,7 @@ func main() { log.Printf("Waiting for command to finish...") err = cmd.Wait() log.Printf("Command finished with error: %v", err) -}`, 1}, {` +}`, 1, false}, {` package main import ( "log" @@ -477,7 +503,7 @@ func main() { log.Fatal(err) } log.Printf("Command finished with error: %v", err) -}`, 1}, {` +}`, 1, false}, {` package main import ( "log" @@ -494,7 +520,7 @@ func main() { log.Printf("Waiting for command to finish...") err = cmd.Wait() log.Printf("Command finished with error: %v", err) -}`, 1}} +}`, 1, false}} // SampleCodeG301 - mkdir permission check SampleCodeG301 = []CodeSample{{` @@ -504,7 +530,7 @@ func main() { os.Mkdir("/tmp/mydir", 0777) os.Mkdir("/tmp/mydir", 0600) os.MkdirAll("/tmp/mydir/mysubidr", 0775) -}`, 2}} +}`, 2, false}} // SampleCodeG302 - file create / chmod permissions check SampleCodeG302 = []CodeSample{{` @@ -515,7 +541,7 @@ func main() { os.Chmod("/tmp/someotherfile", 0600) os.OpenFile("/tmp/thing", os.O_CREATE|os.O_WRONLY, 0666) os.OpenFile("/tmp/thing", os.O_CREATE|os.O_WRONLY, 0600) -}`, 2}} +}`, 2, false}} // SampleCodeG303 - bad tempfile permissions & hardcoded shared path SampleCodeG303 = []CodeSample{{` @@ -528,7 +554,7 @@ func main() { file1, _ := os.Create("/tmp/demo1") defer file1.Close() ioutil.WriteFile("/tmp/demo2", []byte("This is some data"), 0644) -}`, 2}} +}`, 2, false}} // SampleCodeG304 - potential file inclusion vulnerability SampleCodeG304 = []CodeSample{{` @@ -546,7 +572,7 @@ if err != nil { } log.Print(body) -}`, 1}, {` +}`, 1, false}, {` package main import ( @@ -570,7 +596,7 @@ func main() { fmt.Fprintf(w, "%s", body) }) log.Fatal(http.ListenAndServe(":3000", nil)) -}`, 1}, {` +}`, 1, false}, {` package main import ( @@ -586,7 +612,7 @@ import ( log.Printf("Error: %v\n", err) } log.Print(body) - }`, 1}, {` + }`, 1, false}, {` package main import ( @@ -610,7 +636,7 @@ func main() { fmt.Printf("Error: %v\n", err) } fmt.Println(string(contents)) -}`, 1}, {` +}`, 1, false}, {` package main import ( @@ -629,7 +655,7 @@ func main() { log.Printf("Error: %v\n", err) } log.Print(body) -}`, 1}} +}`, 1, false}} // SampleCodeG305 - File path traversal when extracting zip archives SampleCodeG305 = []CodeSample{{` @@ -677,7 +703,7 @@ func unzip(archive, target string) error { } return nil -}`, 1}, {` +}`, 1, false}, {` package unzip import ( @@ -723,7 +749,7 @@ func unzip(archive, target string) error { } return nil -}`, 1}} +}`, 1, false}} // SampleCodeG401 - Use of weak crypto MD5 SampleCodeG401 = []CodeSample{ @@ -748,7 +774,7 @@ func main() { log.Fatal(err) } fmt.Printf("%x", h.Sum(nil)) -}`, 1}} +}`, 1, false}} // SampleCodeG401b - Use of weak crypto SHA1 SampleCodeG401b = []CodeSample{ @@ -773,7 +799,7 @@ func main() { log.Fatal(err) } fmt.Printf("%x", h.Sum(nil)) -}`, 1}} +}`, 1, false}} // SampleCodeG402 - TLS settings SampleCodeG402 = []CodeSample{{` @@ -794,7 +820,7 @@ func main() { if err != nil { fmt.Println(err) } -}`, 1}, { +}`, 1, false}, { ` // Insecure minimum version package main @@ -812,7 +838,7 @@ func main() { if err != nil { fmt.Println(err) } -}`, 1}, {` +}`, 1, false}, {` // Insecure max version package main import ( @@ -830,7 +856,7 @@ func main() { fmt.Println(err) } } -`, 1}, { +`, 1, false}, { ` // Insecure ciphersuite selection package main @@ -851,7 +877,7 @@ func main() { if err != nil { fmt.Println(err) } -}`, 1}} +}`, 1, false}} // SampleCodeG403 - weak key strength SampleCodeG403 = []CodeSample{ @@ -869,7 +895,7 @@ func main() { fmt.Println(err) } fmt.Println(pvk) -}`, 1}} +}`, 1, false}} // SampleCodeG404 - weak random number SampleCodeG404 = []CodeSample{ @@ -879,13 +905,13 @@ import "crypto/rand" func main() { good, _ := rand.Read(nil) println(good) -}`, 0}, {` +}`, 0, false}, {` package main import "math/rand" func main() { bad := rand.Int() println(bad) -}`, 1}, {` +}`, 1, false}, {` package main import ( "crypto/rand" @@ -896,7 +922,7 @@ func main() { println(good) i := mrand.Int31() println(i) -}`, 0}} +}`, 0, false}} // SampleCodeG501 - Blacklisted import MD5 SampleCodeG501 = []CodeSample{ @@ -911,7 +937,7 @@ func main() { for _, arg := range os.Args { fmt.Printf("%x - %s\n", md5.Sum([]byte(arg)), arg) } -}`, 1}} +}`, 1, false}} // SampleCodeG502 - Blacklisted import DES SampleCodeG502 = []CodeSample{ @@ -939,7 +965,7 @@ func main() { stream := cipher.NewCFBEncrypter(block, iv) stream.XORKeyStream(ciphertext[des.BlockSize:], plaintext) fmt.Println("Secret message is: %s", hex.EncodeToString(ciphertext)) -}`, 1}} +}`, 1, false}} // SampleCodeG503 - Blacklisted import RC4 SampleCodeG503 = []CodeSample{{` @@ -958,7 +984,7 @@ func main() { ciphertext := make([]byte, len(plaintext)) cipher.XORKeyStream(ciphertext, plaintext) fmt.Println("Secret message is: %s", hex.EncodeToString(ciphertext)) -}`, 1}} +}`, 1, false}} // SampleCodeG504 - Blacklisted import CGI SampleCodeG504 = []CodeSample{{` @@ -969,7 +995,7 @@ import ( ) func main() { cgi.Serve(http.FileServer(http.Dir("/usr/share/doc"))) -}`, 1}} +}`, 1, false}} // SampleCodeG505 - Blacklisted import SHA1 SampleCodeG505 = []CodeSample{ {` @@ -983,7 +1009,7 @@ func main() { for _, arg := range os.Args { fmt.Printf("%x - %s\n", sha1.Sum([]byte(arg)), arg) } -}`, 1}} +}`, 1, false}} // SampleCode601 - Go build tags SampleCode601 = []CodeSample{{` // +build test @@ -991,5 +1017,5 @@ func main() { package main func main() { fmt.Println("no package imported error") -}`, 1}} +}`, 1, false}} )