Merge pull request securego#97 from GoASTScanner/experimental

Address unhandled error conditions

core/analyzer.go

@@ -23,6 +23,7 @@ import (
 	"go/types"
 	"log"
 	"os"
+	"path"
 	"reflect"
 	"strings"
 )
@@ -222,8 +223,9 @@ func (gas *Analyzer) Visit(n ast.Node) ast.Visitor {
 			for _, rule := range val {
 				ret, err := rule.Match(n, &gas.context)
 				if err != nil {
-					// will want to give more info than this ...
-					gas.logger.Println("internal error running rule:", err)
+					file, line := GetLocation(n, &gas.context)
+					file = path.Base(file)
+					gas.logger.Printf("Rule error: %v => %s (%s:%d)\n", reflect.TypeOf(rule), err, file, line)
 				}
 				if ret != nil {
 					gas.Issues = append(gas.Issues, *ret)

core/helpers.go

@@ -212,3 +212,9 @@ func GetImportPath(name string, ctx *Context) (string, bool) {
 	}
 	return "", false
 }
+
+// GetLocation returns the filename and line number of an ast.Node
+func GetLocation(n ast.Node, ctx *Context) (string, int) {
+	fobj := ctx.FileSet.File(n.Pos())
+	return fobj.Name(), fobj.Line(n.Pos())
+}

filelist.go

@@ -15,6 +15,8 @@
 package main
 
 import (
+	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 )
@@ -32,7 +34,10 @@ func newFileList(paths ...string) *filelist {
 	}
 
 	for _, path := range paths {
-		f.Set(path)
+		if e := f.Set(path); e != nil {
+			// #nosec
+			fmt.Fprintf(os.Stderr, "Unable to add %s to filelist: %s\n", path, e)
+		}
 	}
 	return f
 }

main.go

@@ -117,7 +117,9 @@ func buildConfig(incRules string, excRules string) map[string]interface{} {
 	return config
 }
 
+// #nosec
 func usage() {
+
 	fmt.Fprintln(os.Stderr, usageText)
 	fmt.Fprint(os.Stderr, "OPTIONS:\n\n")
 	flag.PrintDefaults()

rules/fileperms.go

@@ -36,7 +36,11 @@ func getConfiguredMode(conf map[string]interface{}, configKey string, defaultMod
 		case int64:
 			mode = value.(int64)
 		case string:
-			mode, _ = strconv.ParseInt(value.(string), 0, 64)
+			if m, e := strconv.ParseInt(value.(string), 0, 64); e != nil {
+				mode = defaultMode
+			} else {
+				mode = m
+			}
 		}
 	}
 	return mode

rules/sql.go

@@ -42,7 +42,7 @@ func (s *SqlStrConcat) checkObject(n *ast.Ident) bool {
 func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	if node, ok := n.(*ast.BinaryExpr); ok {
 		if start, ok := node.X.(*ast.BasicLit); ok {
-			if str, _ := gas.GetString(start); s.pattern.MatchString(str) {
+			if str, e := gas.GetString(start); s.pattern.MatchString(str) && e == nil {
 				if _, ok := node.Y.(*ast.BasicLit); ok {
 					return nil, nil // string cat OK
 				}
@@ -77,7 +77,7 @@ type SqlStrFormat struct {
 // Looks for "fmt.Sprintf("SELECT * FROM foo where '%s', userInput)"
 func (s *SqlStrFormat) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if node := gas.MatchCall(n, s.call); node != nil {
-		if arg, _ := gas.GetString(node.Args[0]); s.pattern.MatchString(arg) {
+		if arg, e := gas.GetString(node.Args[0]); s.pattern.MatchString(arg) && e == nil {
 			return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil
 		}
 	}

rules/tempfiles.go

@@ -29,7 +29,7 @@ type BadTempFile struct {
 
 func (t *BadTempFile) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if node := gas.MatchCall(n, t.call); node != nil {
-		if arg, _ := gas.GetString(node.Args[0]); t.args.MatchString(arg) {
+		if arg, e := gas.GetString(node.Args[0]); t.args.MatchString(arg) && e == nil {
 			return gas.NewIssue(c, n, t.What, t.Severity, t.Confidence), nil
 		}
 	}

tools.go

@@ -74,10 +74,12 @@ func (u *utilities) run(args ...string) {
 func shouldSkip(path string) bool {
 	st, e := os.Stat(path)
 	if e != nil {
+		// #nosec
 		fmt.Fprintf(os.Stderr, "Skipping: %s - %s\n", path, e)
 		return true
 	}
 	if st.IsDir() {
+		// #nosec
 		fmt.Fprintf(os.Stderr, "Skipping: %s - directory\n", path)
 		return true
 	}
@@ -95,11 +97,12 @@ func dumpAst(files ...string) {
 		fset := token.NewFileSet() // positions are relative to fset
 		f, err := parser.ParseFile(fset, arg, nil, 0)
 		if err != nil {
+			// #nosec
 			fmt.Fprintf(os.Stderr, "Unable to parse file %s\n", err)
 			continue
 		}
 
-		// Print the AST.
+		// Print the AST. #nosec
 		ast.Print(fset, f)
 	}
 }
@@ -115,7 +118,12 @@ type context struct {
 
 func createContext(filename string) *context {
 	fileset := token.NewFileSet()
-	root, _ := parser.ParseFile(fileset, filename, nil, parser.ParseComments)
+	root, e := parser.ParseFile(fileset, filename, nil, parser.ParseComments)
+	if e != nil {
+		// #nosec
+		fmt.Fprintf(os.Stderr, "Unable to parse file: %s. Reason: %s\n", filename, e)
+		return nil
+	}
 	comments := ast.NewCommentMap(fileset, root, root.Comments)
 	info := &types.Info{
 		Types:      make(map[ast.Expr]types.TypeAndValue),
@@ -126,7 +134,12 @@ func createContext(filename string) *context {
 		Implicits:  make(map[ast.Node]types.Object),
 	}
 	config := types.Config{Importer: importer.Default()}
-	pkg, _ := config.Check("main.go", fileset, []*ast.File{root}, info)
+	pkg, e := config.Check("main.go", fileset, []*ast.File{root}, info)
+	if e != nil {
+		// #nosec
+		fmt.Fprintf(os.Stderr, "Type check failed for file: %s. Reason: %s\n", filename, e)
+		return nil
+	}
 	return &context{fileset, comments, info, pkg, &config, root}
 }
 
@@ -147,13 +160,25 @@ func printObject(obj types.Object) {
 	fmt.Printf("   Id = %v\n", obj.Id())
 }
 
+func checkContext(ctx *context, file string) bool {
+	// #nosec
+	if ctx == nil {
+		fmt.Fprintln(os.Stderr, "Failed to create context for file: ", file)
+		return false
+	}
+	return true
+}
+
 func dumpCallObj(files ...string) {
 
 	for _, file := range files {
 		if shouldSkip(file) {
 			continue
 		}
 		context := createContext(file)
+		if !checkContext(context, file) {
+			return
+		}
 		ast.Inspect(context.root, func(n ast.Node) bool {
 			var obj types.Object
 			switch node := n.(type) {
@@ -178,6 +203,9 @@ func dumpUses(files ...string) {
 			continue
 		}
 		context := createContext(file)
+		if !checkContext(context, file) {
+			return
+		}
 		for ident, obj := range context.info.Uses {
 			fmt.Printf("IDENT: %v, OBJECT: %v\n", ident, obj)
 		}
@@ -190,6 +218,9 @@ func dumpTypes(files ...string) {
 			continue
 		}
 		context := createContext(file)
+		if !checkContext(context, file) {
+			return
+		}
 		for expr, tv := range context.info.Types {
 			fmt.Printf("EXPR: %v, TYPE: %v\n", expr, tv)
 		}
@@ -202,6 +233,9 @@ func dumpDefs(files ...string) {
 			continue
 		}
 		context := createContext(file)
+		if !checkContext(context, file) {
+			return
+		}
 		for ident, obj := range context.info.Defs {
 			fmt.Printf("IDENT: %v, OBJ: %v\n", ident, obj)
 		}
@@ -214,6 +248,9 @@ func dumpComments(files ...string) {
 			continue
 		}
 		context := createContext(file)
+		if !checkContext(context, file) {
+			return
+		}
 		for _, group := range context.comments.Comments() {
 			fmt.Println(group.Text())
 		}
@@ -226,6 +263,9 @@ func dumpImports(files ...string) {
 			continue
 		}
 		context := createContext(file)
+		if !checkContext(context, file) {
+			return
+		}
 		for _, pkg := range context.pkg.Imports() {
 			fmt.Println(pkg.Path(), pkg.Name())
 			for _, name := range pkg.Scope().Names() {