From e925d3c34794f158471980ff05df9dd63601e16b Mon Sep 17 00:00:00 2001 From: Grant Murphy Date: Thu, 28 Dec 2017 16:54:10 +1000 Subject: [PATCH] Migrated old test cases. --- call_list.go | 1 + rules/blacklist.go | 22 ++- rules/rules_test.go | 72 ++++++- rules/templates.go | 3 +- rules/tls.go | 45 ++--- testutils/source.go | 459 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 562 insertions(+), 40 deletions(-) diff --git a/call_list.go b/call_list.go index 909ae32..b0e9855 100644 --- a/call_list.go +++ b/call_list.go @@ -60,6 +60,7 @@ func (c CallList) ContainsCallExpr(n ast.Node, ctx *Context) *ast.CallExpr { if err != nil { return nil } + // Try direct resolution if c.Contains(selector, ident) { return n.(*ast.CallExpr) diff --git a/rules/blacklist.go b/rules/blacklist.go index d6699ae..fbcfff0 100644 --- a/rules/blacklist.go +++ b/rules/blacklist.go @@ -16,6 +16,7 @@ package rules import ( "go/ast" + "strings" "github.com/GoASTScanner/gas" ) @@ -25,11 +26,16 @@ type blacklistedImport struct { Blacklisted map[string]string } -func (r *blacklistedImport) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) { +func unquote(original string) string { + copy := strings.TrimSpace(original) + copy = strings.TrimLeft(copy, `"`) + return strings.TrimRight(copy, `"`) +} + +func (r *blacklistedImport) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { if node, ok := n.(*ast.ImportSpec); ok { - description, ok := r.Blacklisted[node.Path.Value] - if ok && node.Name.String() != "_" { - return gas.NewIssue(c, n, description, r.Severity, r.Confidence), nil + if description, ok := r.Blacklisted[unquote(node.Path.Value)]; ok { + return gas.NewIssue(c, node, description, r.Severity, r.Confidence), nil } } return nil, nil @@ -50,27 +56,27 @@ func NewBlacklistedImports(conf gas.Config, blacklist map[string]string) (gas.Ru // NewBlacklistedImportMD5 fails if MD5 is imported func NewBlacklistedImportMD5(conf gas.Config) (gas.Rule, []ast.Node) { return NewBlacklistedImports(conf, map[string]string{ - "crypto/md5": "Use of weak cryptographic primitive", + "crypto/md5": "Blacklisted import crypto/md5: weak cryptographic primitive", }) } // NewBlacklistedImportDES fails if DES is imported func NewBlacklistedImportDES(conf gas.Config) (gas.Rule, []ast.Node) { return NewBlacklistedImports(conf, map[string]string{ - "crypto/des": "Use of weak cryptographic primitive", + "crypto/des": "Blacklisted import crypto/des: weak cryptographic primitive", }) } // NewBlacklistedImportRC4 fails if DES is imported func NewBlacklistedImportRC4(conf gas.Config) (gas.Rule, []ast.Node) { return NewBlacklistedImports(conf, map[string]string{ - "crypto/rc4": "Use of weak cryptographic primitive", + "crypto/rc4": "Blacklisted import crypto/rc4: weak cryptographic primitive", }) } // NewBlacklistedImportCGI fails if CGI is imported func NewBlacklistedImportCGI(conf gas.Config) (gas.Rule, []ast.Node) { return NewBlacklistedImports(conf, map[string]string{ - "net/http/cgi": "Go versions < 1.6.3 are vulnerable to Httpoxy attack: (CVE-2016-5386)", + "net/http/cgi": "Blacklisted import net/http/cgi: Go versions < 1.6.3 are vulnerable to Httpoxy attack: (CVE-2016-5386)", }) } diff --git a/rules/rules_test.go b/rules/rules_test.go index e0fd6ca..6bc0d7d 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -47,22 +47,86 @@ var _ = Describe("gas rules", func() { }) Context("report correct errors for all samples", func() { - It("should work for G101 samples", func() { + It("should detect hardcoded credentials", func() { runner("G101", testutils.SampleCodeG101) }) - It("should work for G102 samples", func() { + It("should detect binding to all network interfaces", func() { runner("G102", testutils.SampleCodeG102) }) - It("should work for G103 samples", func() { + It("should use of unsafe block", func() { runner("G103", testutils.SampleCodeG103) }) - It("should work for G104 samples", func() { + It("should errors not being checked", func() { runner("G104", testutils.SampleCodeG104) }) + It("should detect of big.Exp function", func() { + runner("G105", testutils.SampleCodeG105) + }) + + It("should detect sql injection via format strings", func() { + runner("G201", testutils.SampleCodeG201) + }) + + It("should detect sql injection via string concatenation", func() { + runner("G202", testutils.SampleCodeG202) + }) + + It("should detect unescaped html in templates", func() { + runner("G203", testutils.SampleCodeG203) + }) + + It("should detect command execution", func() { + runner("G204", testutils.SampleCodeG204) + }) + + It("should detect poor file permissions on mkdir", func() { + runner("G301", testutils.SampleCodeG301) + }) + + It("should detect poor permissions when creating or chmod a file", func() { + runner("G302", testutils.SampleCodeG302) + }) + + It("should detect insecure temp file creation", func() { + runner("G303", testutils.SampleCodeG303) + }) + + It("should detect weak crypto algorithms", func() { + runner("G401", testutils.SampleCodeG401) + }) + + It("should find insecure tls settings", func() { + runner("G402", testutils.SampleCodeG402) + }) + + It("should detect weak creation of weak rsa keys", func() { + runner("G403", testutils.SampleCodeG403) + }) + + It("should find non cryptographically secure random number sources", func() { + runner("G404", testutils.SampleCodeG404) + }) + + It("should detect blacklisted imports - MD5", func() { + runner("G501", testutils.SampleCodeG501) + }) + + It("should detect blacklisted imports - DES", func() { + runner("G502", testutils.SampleCodeG502) + }) + + It("should detect blacklisted imports - RC4", func() { + runner("G503", testutils.SampleCodeG503) + }) + + It("should detect blacklisted imports - CGI (httpoxy)", func() { + runner("G504", testutils.SampleCodeG504) + }) + }) }) diff --git a/rules/templates.go b/rules/templates.go index 78bbd92..eae3503 100644 --- a/rules/templates.go +++ b/rules/templates.go @@ -44,8 +44,9 @@ func NewTemplateCheck(conf gas.Config) (gas.Rule, []ast.Node) { calls.Add("template", "HTML") calls.Add("template", "HTMLAttr") calls.Add("template", "JS") + calls.Add("template", "URL") return &templateCheck{ - calls: gas.NewCallList(), + calls: calls, MetaData: gas.MetaData{ Severity: gas.Medium, Confidence: gas.Low, diff --git a/rules/tls.go b/rules/tls.go index 00a13f5..c0971b2 100644 --- a/rules/tls.go +++ b/rules/tls.go @@ -38,23 +38,13 @@ func stringInSlice(a string, list []string) bool { } func (t *insecureConfigTLS) processTLSCipherSuites(n ast.Node, c *gas.Context) *gas.Issue { - tlsConfig := gas.MatchCompLit(n, c, t.requiredType) - if tlsConfig == nil { - return nil - } - for _, expr := range tlsConfig.Elts { - if keyvalExpr, ok := expr.(*ast.KeyValueExpr); ok { - if keyname, ok := keyvalExpr.Key.(*ast.Ident); ok && keyname.Name == "CipherSuites" { - if ciphers, ok := keyvalExpr.Value.(*ast.CompositeLit); ok { - for _, cipher := range ciphers.Elts { - if ident, ok := cipher.(*ast.SelectorExpr); ok { - if !stringInSlice(ident.Sel.Name, t.goodCiphers) { - str := fmt.Sprintf("TLS Bad Cipher Suite: %s", ident.Sel.Name) - return gas.NewIssue(c, n, str, gas.High, gas.High) - } - } - } + if ciphers, ok := n.(*ast.CompositeLit); ok { + for _, cipher := range ciphers.Elts { + if ident, ok := cipher.(*ast.SelectorExpr); ok { + if !stringInSlice(ident.Sel.Name, t.goodCiphers) { + err := fmt.Sprintf("TLS Bad Cipher Suite: %s", ident.Sel.Name) + return gas.NewIssue(c, ident, err, gas.High, gas.High) } } } @@ -65,6 +55,7 @@ func (t *insecureConfigTLS) processTLSCipherSuites(n ast.Node, c *gas.Context) * func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gas.Context) *gas.Issue { if ident, ok := n.Key.(*ast.Ident); ok { switch ident.Name { + case "InsecureSkipVerify": if node, ok := n.Value.(*ast.Ident); ok { if node.Name != "false" { @@ -104,7 +95,7 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gas.Contex } case "CipherSuites": - if ret := t.processTLSCipherSuites(n, c); ret != nil { + if ret := t.processTLSCipherSuites(n.Value, c); ret != nil { return ret } @@ -114,24 +105,24 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gas.Contex return nil } -func (t *insecureConfigTLS) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) { - if node := gas.MatchCompLit(n, c, t.requiredType); node != nil { - for _, elt := range node.Elts { +func (t *insecureConfigTLS) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { + if complit, ok := n.(*ast.CompositeLit); ok && c.Info.TypeOf(complit.Type).String() == t.requiredType { + for _, elt := range complit.Elts { if kve, ok := elt.(*ast.KeyValueExpr); ok { - gi = t.processTLSConfVal(kve, c) - if gi != nil { - break + issue := t.processTLSConfVal(kve, c) + if issue != nil { + return issue, nil } } } } - return + return nil, nil } // NewModernTLSCheck see: https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility func NewModernTLSCheck(conf gas.Config) (gas.Rule, []ast.Node) { return &insecureConfigTLS{ - requiredType: "tls.Config", + requiredType: "crypto/tls.Config", MinVersion: 0x0303, // TLS 1.2 only MaxVersion: 0x0303, goodCiphers: []string{ @@ -146,7 +137,7 @@ func NewModernTLSCheck(conf gas.Config) (gas.Rule, []ast.Node) { // NewIntermediateTLSCheck see: https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28default.29 func NewIntermediateTLSCheck(conf gas.Config) (gas.Rule, []ast.Node) { return &insecureConfigTLS{ - requiredType: "tls.Config", + requiredType: "crypto/tls.Config", MinVersion: 0x0301, // TLS 1.2, 1.1, 1.0 MaxVersion: 0x0303, goodCiphers: []string{ @@ -172,7 +163,7 @@ func NewIntermediateTLSCheck(conf gas.Config) (gas.Rule, []ast.Node) { // NewCompatTLSCheck see: https://wiki.mozilla.org/Security/Server_Side_TLS#Old_compatibility_.28default.29 func NewCompatTLSCheck(conf gas.Config) (gas.Rule, []ast.Node) { return &insecureConfigTLS{ - requiredType: "tls.Config", + requiredType: "crypto/tls.Config", MinVersion: 0x0301, // TLS 1.2, 1.1, 1.0 MaxVersion: 0x0303, goodCiphers: []string{ diff --git a/testutils/source.go b/testutils/source.go index 91a0837..9ba1b04 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -166,6 +166,269 @@ func main() { fmt.Println(e) }`, 0}} + // SampleCodeG105 - bignum overflow + SampleCodeG105 = []CodeSample{{` +package main +import ( + "math/big" +) +func main() { + z := new(big.Int) + x := new(big.Int) + x = x.SetUint64(2) + y := new(big.Int) + y = y.SetUint64(4) + m := new(big.Int) + m = m.SetUint64(0) + z = z.Exp(x, y, m) +}`, 1}} + + // SampleCodeG201 - SQL injection via format string + SampleCodeG201 = []CodeSample{ + {` +// Format string without proper quoting +package main +import ( + "database/sql" + "fmt" + "os" + //_ "github.com/mattn/go-sqlite3" +) + +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + q := fmt.Sprintf("SELECT * FROM foo where name = '%s'", os.Args[1]) + rows, err := db.Query(q) + if err != nil { + panic(err) + } + defer rows.Close() +}`, 1}, { + ` +// Format string false positive +package main +import ( + "database/sql" + //_ "github.com/mattn/go-sqlite3" +) +var staticQuery = "SELECT * FROM foo WHERE age < 32" +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + rows, err := db.Query(staticQuery) + if err != nil { + panic(err) + } + defer rows.Close() +}`, 0}} + + // SampleCodeG202 - SQL query string building via string concatenation + SampleCodeG202 = []CodeSample{ + {` +package main +import ( + "database/sql" + //_ "github.com/mattn/go-sqlite3" + "os" +) +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + rows, err := db.Query("SELECT * FROM foo WHERE name = " + os.Args[1]) + if err != nil { + panic(err) + } + defer rows.Close() +}`, 1}, {` +// false positive +package main +import ( + "database/sql" + //_ "github.com/mattn/go-sqlite3" +) +var staticQuery = "SELECT * FROM foo WHERE age < " +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + rows, err := db.Query(staticQuery + "32") + if err != nil { + panic(err) + } + defer rows.Close() +}`, 0}, {` +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(staticQuery + age) + if err != nil { + panic(err) + } + defer rows.Close() +} +`, 0}} + + // SampleCodeG203 - Template checks + SampleCodeG203 = []CodeSample{ + {` +// We assume that hardcoded template strings are safe as the programmer would +// need to be explicitly shooting themselves in the foot (as below) +package main +import ( + "html/template" + "os" +) +const tmpl = "" +func main() { + t := template.Must(template.New("ex").Parse(tmpl)) + v := map[string]interface{}{ + "Title": "Test World", + "Body": template.HTML(""), + } + t.Execute(os.Stdout, v) +}`, 0}, { + ` +// Using a variable to initialize could potentially be dangerous. Under the +// current model this will likely produce some false positives. +package main +import ( + "html/template" + "os" +) +const tmpl = "" +func main() { + a := "something from another place" + t := template.Must(template.New("ex").Parse(tmpl)) + v := map[string]interface{}{ + "Title": "Test World", + "Body": template.HTML(a), + } + t.Execute(os.Stdout, v) +}`, 1}, { + ` +package main +import ( + "html/template" + "os" +) +const tmpl = "" +func main() { + a := "something from another place" + t := template.Must(template.New("ex").Parse(tmpl)) + v := map[string]interface{}{ + "Title": "Test World", + "Body": template.JS(a), + } + t.Execute(os.Stdout, v) +}`, 1}, { + ` +package main +import ( + "html/template" + "os" +) +const tmpl = "" +func main() { + a := "something from another place" + t := template.Must(template.New("ex").Parse(tmpl)) + v := map[string]interface{}{ + "Title": "Test World", + "Body": template.URL(a), + } + t.Execute(os.Stdout, v) +}`, 1}} + + // SampleCodeG204 - Subprocess auditing + SampleCodeG204 = []CodeSample{{` +package main +import "syscall" +func main() { + syscall.Exec("/bin/cat", []string{ "/etc/passwd" }, nil) +}`, 1}, {` +package main +import ( + "log" + "os/exec" +) +func main() { + cmd := exec.Command("sleep", "5") + err := cmd.Start() + if err != nil { + log.Fatal(err) + } + log.Printf("Waiting for command to finish...") + err = cmd.Wait() + log.Printf("Command finished with error: %v", err) +}`, 1}, {` +package main +import ( + "log" + "os" + "os/exec" +) +func main() { + run := "sleep" + os.Getenv("SOMETHING") + cmd := exec.Command(run, "5") + err := cmd.Start() + if err != nil { + log.Fatal(err) + } + log.Printf("Waiting for command to finish...") + err = cmd.Wait() + log.Printf("Command finished with error: %v", err) +}`, 1}} + + // SampleCodeG301 - mkdir permission check + SampleCodeG301 = []CodeSample{{` +package main +import "os" +func main() { + os.Mkdir("/tmp/mydir", 0777) + os.Mkdir("/tmp/mydir", 0600) + os.MkdirAll("/tmp/mydir/mysubidr", 0775) +}`, 2}} + + // SampleCodeG302 - file create / chmod permissions check + SampleCodeG302 = []CodeSample{{` +package main +import "os" +func main() { + os.Chmod("/tmp/somefile", 0777) + 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}} + + // SampleCodeG303 - bad tempfile permissions & hardcoded shared path + SampleCodeG303 = []CodeSample{{` +package samples +import ( + "io/ioutil" + "os" +) +func main() { + file1, _ := os.Create("/tmp/demo1") + defer file1.Close() + ioutil.WriteFile("/tmp/demo2", []byte("This is some data"), 0644) +}`, 2}} + // SampleCodeG401 - Use of weak crypto MD5 SampleCodeG401 = []CodeSample{ {` @@ -190,4 +453,200 @@ func main() { } fmt.Printf("%x", h.Sum(nil)) }`, 1}} + + // SampleCodeG402 - TLS settings + SampleCodeG402 = []CodeSample{{` +// InsecureSkipVerify +package main +import ( + "crypto/tls" + "fmt" + "net/http" +) +func main() { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + } + + client := &http.Client{Transport: tr} + _, err := client.Get("https://golang.org/") + if err != nil { + fmt.Println(err) + } +}`, 1}, { + ` +// Insecure minimum version +package main +import ( + "crypto/tls" + "fmt" + "net/http" +) +func main() { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{MinVersion: 0}, + } + client := &http.Client{Transport: tr} + _, err := client.Get("https://golang.org/") + if err != nil { + fmt.Println(err) + } +}`, 1}, {` +// Insecure max version +package main +import ( + "crypto/tls" + "fmt" + "net/http" +) +func main() { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{MaxVersion: 0}, + } + client := &http.Client{Transport: tr} + _, err := client.Get("https://golang.org/") + if err != nil { + fmt.Println(err) + } +} +`, 1}, { + ` +// Insecure ciphersuite selection +package main +import ( + "crypto/tls" + "fmt" + "net/http" +) +func main() { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{CipherSuites: []uint16{ + tls.TLS_RSA_WITH_RC4_128_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + },}, + } + client := &http.Client{Transport: tr} + _, err := client.Get("https://golang.org/") + if err != nil { + fmt.Println(err) + } +}`, 1}} + + // SampleCodeG403 - weak key strength + SampleCodeG403 = []CodeSample{ + {` +package main +import ( + "crypto/rand" + "crypto/rsa" + "fmt" +) +func main() { + //Generate Private Key + pvk, err := rsa.GenerateKey(rand.Reader, 1024) + if err != nil { + fmt.Println(err) + } + fmt.Println(pvk) +}`, 1}} + + // SampleCodeG404 - weak random number + SampleCodeG404 = []CodeSample{ + {` +package main +import "crypto/rand" +func main() { + good, _ := rand.Read(nil) + println(good) +}`, 0}, {` +package main +import "math/rand" +func main() { + bad := rand.Int() + println(bad) +}`, 1}, {` +package main +import ( + "crypto/rand" + mrand "math/rand" +) +func main() { + good, _ := rand.Read(nil) + println(good) + i := mrand.Int31() + println(i) +}`, 0}} + + // SampleCode501 - Blacklisted import MD5 + SampleCodeG501 = []CodeSample{ + {` +package main +import ( + "crypto/md5" + "fmt" + "os" +) +func main() { + for _, arg := range os.Args { + fmt.Printf("%x - %s\n", md5.Sum([]byte(arg)), arg) + } +}`, 1}} + + // SampleCode502 - Blacklisted import DES + SampleCodeG502 = []CodeSample{ + {` +package main +import ( + "crypto/cipher" + "crypto/des" + "crypto/rand" + "encoding/hex" + "fmt" + "io" +) +func main() { + block, err := des.NewCipher([]byte("sekritz")) + if err != nil { + panic(err) + } + plaintext := []byte("I CAN HAZ SEKRIT MSG PLZ") + ciphertext := make([]byte, des.BlockSize+len(plaintext)) + iv := ciphertext[:des.BlockSize] + if _, err := io.ReadFull(rand.Reader, iv); err != nil { + panic(err) + } + stream := cipher.NewCFBEncrypter(block, iv) + stream.XORKeyStream(ciphertext[des.BlockSize:], plaintext) + fmt.Println("Secret message is: %s", hex.EncodeToString(ciphertext)) +}`, 1}} + + // SampleCodeG503 - Blacklisted import RC4 + SampleCodeG503 = []CodeSample{{` +package main +import ( + "crypto/rc4" + "encoding/hex" + "fmt" +) +func main() { + cipher, err := rc4.NewCipher([]byte("sekritz")) + if err != nil { + panic(err) + } + plaintext := []byte("I CAN HAZ SEKRIT MSG PLZ") + ciphertext := make([]byte, len(plaintext)) + cipher.XORKeyStream(ciphertext, plaintext) + fmt.Println("Secret message is: %s", hex.EncodeToString(ciphertext)) +}`, 1}} + + // SampleCodeG504 - Blacklisted import CGI + SampleCodeG504 = []CodeSample{{` +package main +import ( + "net/http/cgi" + "net/http" + ) +func main() { + cgi.Serve(http.FileServer(http.Dir("/usr/share/doc"))) +}`, 1}} )