Skip to content

Commit

Permalink
Update to latest gosec.
Browse files Browse the repository at this point in the history
  • Loading branch information
alecthomas committed Sep 6, 2018
1 parent 3917f98 commit 2b0b5f3
Show file tree
Hide file tree
Showing 8 changed files with 258 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type goTLSConfiguration struct {

// getTLSConfFromURL retrieves the json containing the TLS configurations from the specified URL.
func getTLSConfFromURL(url string) (*ServerSideTLSJson, error) {
r, err := http.Get(url)
r, err := http.Get(url) // #nosec G107
if err != nil {
return nil, err
}
Expand Down
30 changes: 30 additions & 0 deletions _linters/src/github.com/securego/gosec/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,3 +281,33 @@ func ConcatString(n *ast.BinaryExpr) (string, bool) {
}
return s, true
}

// FindVarIdentities returns array of all variable identities in a given binary expression
func FindVarIdentities(n *ast.BinaryExpr, c *Context) ([]*ast.Ident, bool) {
identities := []*ast.Ident{}
// sub expressions are found in X object, Y object is always the last term
if rightOperand, ok := n.Y.(*ast.Ident); ok {
obj := c.Info.ObjectOf(rightOperand)
if _, ok := obj.(*types.Var); ok && !TryResolve(rightOperand, c) {
identities = append(identities, rightOperand)
}
}
if leftOperand, ok := n.X.(*ast.BinaryExpr); ok {
if leftIdentities, ok := FindVarIdentities(leftOperand, c); ok {
identities = append(identities, leftIdentities...)
}
} else {
if leftOperand, ok := n.X.(*ast.Ident); ok {
obj := c.Info.ObjectOf(leftOperand)
if _, ok := obj.(*types.Var); ok && !TryResolve(leftOperand, c) {
identities = append(identities, leftOperand)
}
}
}

if len(identities) > 0 {
return identities, true
}
// if nil or error, return false
return nil, false
}
43 changes: 43 additions & 0 deletions _linters/src/github.com/securego/gosec/rules/readfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,57 @@ import (
type readfile struct {
gosec.MetaData
gosec.CallList
pathJoin gosec.CallList
}

// ID returns the identifier for this rule
func (r *readfile) ID() string {
return r.MetaData.ID
}

// isJoinFunc checks if there is a filepath.Join or other join function
func (r *readfile) isJoinFunc(n ast.Node, c *gosec.Context) bool {
if call := r.pathJoin.ContainsCallExpr(n, c); call != nil {
for _, arg := range call.Args {
// edge case: check if one of the args is a BinaryExpr
if binExp, ok := arg.(*ast.BinaryExpr); ok {
// iterate and resolve all found identites from the BinaryExpr
if _, ok := gosec.FindVarIdentities(binExp, c); ok {
return true
}
}

// try and resolve identity
if ident, ok := arg.(*ast.Ident); ok {
obj := c.Info.ObjectOf(ident)
if _, ok := obj.(*types.Var); ok && !gosec.TryResolve(ident, c) {
return true
}
}
}
}
return false
}

// Match inspects AST nodes to determine if the match the methods `os.Open` or `ioutil.ReadFile`
func (r *readfile) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
if node := r.ContainsCallExpr(n, c); node != nil {
for _, arg := range node.Args {
// handles path joining functions in Arg
// eg. os.Open(filepath.Join("/tmp/", file))
if callExpr, ok := arg.(*ast.CallExpr); ok {
if r.isJoinFunc(callExpr, c) {
return gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
}
}
// handles binary string concatenation eg. ioutil.Readfile("/tmp/" + file + "/blob")
if binExp, ok := arg.(*ast.BinaryExpr); ok {
// resolve all found identites from the BinaryExpr
if _, ok := gosec.FindVarIdentities(binExp, c); ok {
return gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
}
}

if ident, ok := arg.(*ast.Ident); ok {
obj := c.Info.ObjectOf(ident)
if _, ok := obj.(*types.Var); ok && !gosec.TryResolve(ident, c) {
Expand All @@ -49,6 +89,7 @@ func (r *readfile) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
// NewReadFile detects cases where we read files
func NewReadFile(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
rule := &readfile{
pathJoin: gosec.NewCallList(),
CallList: gosec.NewCallList(),
MetaData: gosec.MetaData{
ID: id,
Expand All @@ -57,6 +98,8 @@ func NewReadFile(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
Confidence: gosec.High,
},
}
rule.pathJoin.Add("path/filepath", "Join")
rule.pathJoin.Add("path", "Join")
rule.Add("io/ioutil", "ReadFile")
rule.Add("os", "Open")
return rule, []ast.Node{(*ast.CallExpr)(nil)}
Expand Down
1 change: 1 addition & 0 deletions _linters/src/github.com/securego/gosec/rules/rulelist.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ func Generate(filters ...RuleFilter) RuleList {
{"G104", "Audit errors not checked", NewNoErrorCheck},
{"G105", "Audit the use of big.Exp function", NewUsingBigExp},
{"G106", "Audit the use of ssh.InsecureIgnoreHostKey function", NewSSHHostKey},
{"G107", "Url provided to HTTP request as taint input", NewSSRFCheck},

// injection
{"G201", "SQL query construction using format string", NewSQLStrFormat},
Expand Down
31 changes: 26 additions & 5 deletions _linters/src/github.com/securego/gosec/rules/sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,35 @@ func NewSQLStrConcat(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {

type sqlStrFormat struct {
sqlStatement
calls gosec.CallList
calls gosec.CallList
noIssue gosec.CallList
}

// Looks for "fmt.Sprintf("SELECT * FROM foo where '%s', userInput)"
func (s *sqlStrFormat) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {

// argIndex changes the function argument which gets matched to the regex
argIndex := 0

// TODO(gm) improve confidence if database/sql is being used
if node := s.calls.ContainsCallExpr(n, c); node != nil {
// if the function is fmt.Fprintf, search for SQL statement in Args[1] instead
if sel, ok := node.Fun.(*ast.SelectorExpr); ok {
if sel.Sel.Name == "Fprintf" {
// if os.Stderr or os.Stdout is in Arg[0], mark as no issue
if arg, ok := node.Args[0].(*ast.SelectorExpr); ok {
if ident, ok := arg.X.(*ast.Ident); ok {
if s.noIssue.Contains(ident.Name, arg.Sel.Name) {
return nil, nil
}
}
}
// the function is Fprintf so set argIndex = 1
argIndex = 1
}
}
// concats callexpr arg strings together if needed before regex evaluation
if argExpr, ok := node.Args[0].(*ast.BinaryExpr); ok {
if argExpr, ok := node.Args[argIndex].(*ast.BinaryExpr); ok {
if fullStr, ok := gosec.ConcatString(argExpr); ok {
if s.MatchPatterns(fullStr) {
return gosec.NewIssue(c, n, s.ID(), s.What, s.Severity, s.Confidence),
Expand All @@ -116,7 +135,7 @@ func (s *sqlStrFormat) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error)
}
}

if arg, e := gosec.GetString(node.Args[0]); s.MatchPatterns(arg) && e == nil {
if arg, e := gosec.GetString(node.Args[argIndex]); s.MatchPatterns(arg) && e == nil {
return gosec.NewIssue(c, n, s.ID(), s.What, s.Severity, s.Confidence), nil
}
}
Expand All @@ -126,7 +145,8 @@ func (s *sqlStrFormat) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error)
// NewSQLStrFormat looks for cases where we're building SQL query strings using format strings
func NewSQLStrFormat(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
rule := &sqlStrFormat{
calls: gosec.NewCallList(),
calls: gosec.NewCallList(),
noIssue: gosec.NewCallList(),
sqlStatement: sqlStatement{
patterns: []*regexp.Regexp{
regexp.MustCompile("(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) "),
Expand All @@ -140,6 +160,7 @@ func NewSQLStrFormat(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
},
},
}
rule.calls.AddAll("fmt", "Sprint", "Sprintf", "Sprintln")
rule.calls.AddAll("fmt", "Sprint", "Sprintf", "Sprintln", "Fprintf")
rule.noIssue.AddAll("os", "Stdout", "Stderr")
return rule, []ast.Node{(*ast.CallExpr)(nil)}
}
59 changes: 59 additions & 0 deletions _linters/src/github.com/securego/gosec/rules/ssrf.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package rules

import (
"go/ast"
"go/types"

"github.com/securego/gosec"
)

type ssrf struct {
gosec.MetaData
gosec.CallList
}

// ID returns the identifier for this rule
func (r *ssrf) ID() string {
return r.MetaData.ID
}

// ResolveVar tries to resolve the first argument of a call expression
// The first argument is the url
func (r *ssrf) ResolveVar(n *ast.CallExpr, c *gosec.Context) bool {
if len(n.Args) > 0 {
arg := n.Args[0]
if ident, ok := arg.(*ast.Ident); ok {
obj := c.Info.ObjectOf(ident)
if _, ok := obj.(*types.Var); ok && !gosec.TryResolve(ident, c) {
return true
}
}
}
return false
}

// Match inspects AST nodes to determine if certain net/http methods are called with variable input
func (r *ssrf) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
// Call expression is using http package directly
if node := r.ContainsCallExpr(n, c); node != nil {
if r.ResolveVar(node, c) {
return gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
}
}
return nil, nil
}

// NewSSRFCheck detects cases where HTTP requests are sent
func NewSSRFCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
rule := &ssrf{
CallList: gosec.NewCallList(),
MetaData: gosec.MetaData{
ID: id,
What: "Potential HTTP request made with variable url",
Severity: gosec.Medium,
Confidence: gosec.Medium,
},
}
rule.AddAll("net/http", "Do", "Get", "Head", "Post", "PostForm", "RoundTrip")
return rule, []ast.Node{(*ast.CallExpr)(nil)}
}
98 changes: 97 additions & 1 deletion _linters/src/github.com/securego/gosec/testutils/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,43 @@ import (
func main() {
_ = ssh.InsecureIgnoreHostKey()
}`, 1}}

// SampleCodeG107 - SSRF via http requests with variable url
SampleCodeG107 = []CodeSample{{`
package main
import (
"net/http"
"io/ioutil"
"fmt"
"os"
)
func main() {
url := os.Getenv("tainted_url")
resp, err := http.Get(url)
if err != nil {
panic(err)
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
panic(err)
}
fmt.Printf("%s", body)
}`, 1}, {`
package main
import (
"fmt"
"net/http"
)
const url = "http://127.0.0.1"
func main() {
resp, err := http.Get(url)
if err != nil {
fmt.Println(err)
}
fmt.Println(resp.Status)
}`, 0}}
// SampleCodeG201 - SQL injection via format string
SampleCodeG201 = []CodeSample{
{`
Expand Down Expand Up @@ -500,7 +537,7 @@ import (
func main() {
http.HandleFunc("/bar", func(w http.ResponseWriter, r *http.Request) {
title := r.URL.Query().Get("title")
title := r.URL.Query().Get("title")
f, err := os.Open(title)
if err != nil {
fmt.Printf("Error: %v\n", err)
Expand All @@ -512,6 +549,65 @@ func main() {
fmt.Fprintf(w, "%s", body)
})
log.Fatal(http.ListenAndServe(":3000", nil))
}`, 1}, {`
package main
import (
"log"
"os"
"io/ioutil"
)
func main() {
f2 := os.Getenv("tainted_file2")
body, err := ioutil.ReadFile("/tmp/" + f2)
if err != nil {
log.Printf("Error: %v\n", err)
}
log.Print(body)
}`, 1}, {`
package main
import (
"bufio"
"fmt"
"os"
"path/filepath"
)
func main() {
reader := bufio.NewReader(os.Stdin)
fmt.Print("Please enter file to read: ")
file, _ := reader.ReadString('\n')
file = file[:len(file)-1]
f, err := os.Open(filepath.Join("/tmp/service/", file))
if err != nil {
fmt.Printf("Error: %v\n", err)
}
contents := make([]byte, 15)
if _, err = f.Read(contents); err != nil {
fmt.Printf("Error: %v\n", err)
}
fmt.Println(string(contents))
}`, 1}, {`
package main
import (
"log"
"os"
"io/ioutil"
"path/filepath"
)
func main() {
dir := os.Getenv("server_root")
f3 := os.Getenv("tainted_file3")
// edge case where both a binary expression and file Join are used.
body, err := ioutil.ReadFile(filepath.Join("/var/"+dir, f3))
if err != nil {
log.Printf("Error: %v\n", err)
}
log.Print(body)
}`, 1}}

// SampleCodeG305 - File path traversal when extracting zip archives
Expand Down
2 changes: 1 addition & 1 deletion _linters/src/manifest
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
"importpath": "github.com/securego/gosec",
"repository": "https://github.com/securego/gosec",
"vcs": "git",
"revision": "a7cff91312e4ea979b7ec590792a289b9d199391",
"revision": "145f1a0bf413089fb73a26c7859887afe39aadfa",
"branch": "master",
"notests": true
},
Expand Down

0 comments on commit 2b0b5f3

Please sign in to comment.