Report for Golang errors (securego#284)

* Report for Golang errors Right now if you use Gosec to scan invalid go file and if you report the result in a text, JSON, CSV or another file format you will always receive 0 issues. The reason for that is that Gosec can't parse the AST of invalid go files and thus will not report anything. The real problem here is that the user will never know about the issue if he generates the output in a file. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
dangolbeeker · Feb 26, 2019 · 62b5195 · 62b5195
1 parent 9cdfec4
commit 62b5195
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 18 deletions.
diff --git a/analyzer.go b/analyzer.go
@@ -26,6 +26,7 @@ import (
 	"path"
 	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"golang.org/x/tools/go/loader"
@@ -64,6 +65,7 @@ type Analyzer struct {
 	logger      *log.Logger
 	issues      []*Issue
 	stats       *Metrics
+	errors      map[string][]Error // keys are file paths; values are the golang errors in those files
 }
 
 // NewAnalyzer builds a new analyzer.
@@ -83,6 +85,7 @@ func NewAnalyzer(conf Config, logger *log.Logger) *Analyzer {
 		logger:      logger,
 		issues:      make([]*Issue, 0, 16),
 		stats:       &Metrics{},
+		errors:      make(map[string][]Error),
 	}
 }
 
@@ -129,6 +132,35 @@ func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error
 	if err != nil {
 		return err
 	}
+	for _, packageInfo := range builtPackage.AllPackages {
+		if len(packageInfo.Errors) != 0 {
+			for _, packErr := range packageInfo.Errors {
+				// infoErr contains information about the error
+				// at index 0 is the file path
+				// at index 1 is the line; index 2 is for column
+				// at index 3 is the actual error
+				infoErr := strings.Split(packErr.Error(), ":")
+				filePath := infoErr[0]
+				line, err := strconv.Atoi(infoErr[1])
+				if err != nil {
+					return err
+				}
+				column, err := strconv.Atoi(infoErr[2])
+				if err != nil {
+					return err
+				}
+				newErr := NewError(line, column, strings.TrimSpace(infoErr[3]))
+
+				if errSlice, ok := gosec.errors[filePath]; ok {
+					gosec.errors[filePath] = append(errSlice, *newErr)
+				} else {
+					errSlice = make([]Error, 0)
+					gosec.errors[filePath] = append(errSlice, *newErr)
+				}
+			}
+		}
+	}
+	sortErrors(gosec.errors) // sorts errors by line and column in the file
 
 	for _, pkg := range builtPackage.Created {
 		gosec.logger.Println("Checking package:", pkg.String())
@@ -233,8 +265,8 @@ func (gosec *Analyzer) Visit(n ast.Node) ast.Visitor {
 }
 
 // Report returns the current issues discovered and the metrics about the scan
-func (gosec *Analyzer) Report() ([]*Issue, *Metrics) {
-	return gosec.issues, gosec.stats
+func (gosec *Analyzer) Report() ([]*Issue, *Metrics, map[string][]Error) {
+	return gosec.issues, gosec.stats, gosec.errors
 }
 
 // Reset clears state such as context, issues and metrics from the configured analyzer

diff --git a/analyzer_test.go b/analyzer_test.go
@@ -68,7 +68,7 @@ var _ = Describe("Analyzer", func() {
 			pkg.Build()
 			err := analyzer.Process(buildTags, pkg.Path)
 			Expect(err).ShouldNot(HaveOccurred())
-			_, metrics := analyzer.Report()
+			_, metrics, _ := analyzer.Report()
 			Expect(metrics.NumFiles).To(Equal(2))
 		})
 
@@ -90,7 +90,7 @@ var _ = Describe("Analyzer", func() {
 			pkg2.Build()
 			err := analyzer.Process(buildTags, pkg1.Path, pkg2.Path)
 			Expect(err).ShouldNot(HaveOccurred())
-			_, metrics := analyzer.Report()
+			_, metrics, _ := analyzer.Report()
 			Expect(metrics.NumFiles).To(Equal(2))
 		})
 
@@ -106,11 +106,36 @@ var _ = Describe("Analyzer", func() {
 			controlPackage.AddFile("md5.go", source)
 			controlPackage.Build()
 			analyzer.Process(buildTags, controlPackage.Path)
-			controlIssues, _ := analyzer.Report()
+			controlIssues, _, _ := analyzer.Report()
 			Expect(controlIssues).Should(HaveLen(sample.Errors))
 
 		})
 
+		It("should report for Golang errors and invalid files", func() {
+			analyzer.LoadRules(rules.Generate().Builders())
+			pkg := testutils.NewTestPackage()
+			defer pkg.Close()
+			pkg.AddFile("foo.go", `
+				package main
+				func main()
+				}`)
+			pkg.Build()
+			err := analyzer.Process(buildTags, pkg.Path)
+			Expect(err).ShouldNot(HaveOccurred())
+			_, _, golangErrors := analyzer.Report()
+			keys := make([]string, len(golangErrors))
+			i := 0
+			for key := range golangErrors {
+				keys[i] = key
+				i++
+			}
+			fileErr := golangErrors[keys[0]]
+			Expect(len(fileErr)).To(Equal(1))
+			Expect(fileErr[0].Line).To(Equal(4))
+			Expect(fileErr[0].Column).To(Equal(5))
+			Expect(fileErr[0].Err).Should(MatchRegexp(`expected declaration, found '}'`))
+		})
+
 		It("should not report errors when a nosec comment is present", func() {
 			// Rule for MD5 weak crypto usage
 			sample := testutils.SampleCodeG401[0]
@@ -124,7 +149,7 @@ var _ = Describe("Analyzer", func() {
 			nosecPackage.Build()
 
 			analyzer.Process(buildTags, nosecPackage.Path)
-			nosecIssues, _ := analyzer.Report()
+			nosecIssues, _, _ := analyzer.Report()
 			Expect(nosecIssues).Should(BeEmpty())
 		})
 
@@ -141,7 +166,7 @@ var _ = Describe("Analyzer", func() {
 			nosecPackage.Build()
 
 			analyzer.Process(buildTags, nosecPackage.Path)
-			nosecIssues, _ := analyzer.Report()
+			nosecIssues, _, _ := analyzer.Report()
 			Expect(nosecIssues).Should(BeEmpty())
 		})
 
@@ -158,7 +183,7 @@ var _ = Describe("Analyzer", func() {
 			nosecPackage.Build()
 
 			analyzer.Process(buildTags, nosecPackage.Path)
-			nosecIssues, _ := analyzer.Report()
+			nosecIssues, _, _ := analyzer.Report()
 			Expect(nosecIssues).Should(HaveLen(sample.Errors))
 		})
 
@@ -175,7 +200,7 @@ var _ = Describe("Analyzer", func() {
 			nosecPackage.Build()
 
 			analyzer.Process(buildTags, nosecPackage.Path)
-			nosecIssues, _ := analyzer.Report()
+			nosecIssues, _, _ := analyzer.Report()
 			Expect(nosecIssues).Should(BeEmpty())
 		})
 
@@ -212,7 +237,7 @@ var _ = Describe("Analyzer", func() {
 		nosecPackage.Build()
 
 		customAnalyzer.Process(buildTags, nosecPackage.Path)
-		nosecIssues, _ := customAnalyzer.Report()
+		nosecIssues, _, _ := customAnalyzer.Report()
 		Expect(nosecIssues).Should(HaveLen(sample.Errors))
 
 	})

diff --git a/cmd/gosec/main.go b/cmd/gosec/main.go
@@ -165,19 +165,19 @@ func loadRules(include, exclude string) rules.RuleList {
 	return rules.Generate(filters...)
 }
 
-func saveOutput(filename, format string, issues []*gosec.Issue, metrics *gosec.Metrics) error {
+func saveOutput(filename, format string, issues []*gosec.Issue, metrics *gosec.Metrics, errors map[string][]gosec.Error) error {
 	if filename != "" {
 		outfile, err := os.Create(filename)
 		if err != nil {
 			return err
 		}
 		defer outfile.Close()
-		err = output.CreateReport(outfile, format, issues, metrics)
+		err = output.CreateReport(outfile, format, issues, metrics, errors)
 		if err != nil {
 			return err
 		}
 	} else {
-		err := output.CreateReport(os.Stdout, format, issues, metrics)
+		err := output.CreateReport(os.Stdout, format, issues, metrics, errors)
 		if err != nil {
 			return err
 		}
@@ -323,7 +323,7 @@ func main() {
 	}
 
 	// Collect the results
-	issues, metrics := analyzer.Report()
+	issues, metrics, errors := analyzer.Report()
 
 	// Sort the issue by severity
 	if *flagSortIssues {
@@ -344,15 +344,15 @@ func main() {
 	}
 
 	// Create output report
-	if err := saveOutput(*flagOutput, *flagFormat, issues, metrics); err != nil {
+	if err := saveOutput(*flagOutput, *flagFormat, issues, metrics, errors); err != nil {
 		logger.Fatal(err)
 	}
 
 	// Finalize logging
 	logWriter.Close() // #nosec
 
 	// Do we have an issue? If so exit 1 unless NoFail is set
-	if issuesFound && !*flagNoFail {
+	if issuesFound && !*flagNoFail || len(errors) != 0 {
 		os.Exit(1)
 	}
 }
diff --git a/errors.go b/errors.go
@@ -0,0 +1,34 @@
+package gosec
+
+import (
+	"sort"
+)
+
+// Error is used when there are golang errors while parsing the AST
+type Error struct {
+	Line   int    `json:"line"`
+	Column int    `json:"column"`
+	Err    string `json:"error"`
+}
+
+// NewError creates Error object
+func NewError(line, column int, err string) *Error {
+	return &Error{
+		Line:   line,
+		Column: column,
+		Err:    err,
+	}
+}
+
+// sortErros sorts the golang erros by line
+func sortErrors(allErrors map[string][]Error) {
+
+	for _, errors := range allErrors {
+		sort.Slice(errors, func(i, j int) bool {
+			if errors[i].Line == errors[j].Line {
+				return errors[i].Column <= errors[j].Column
+			}
+			return errors[i].Line < errors[j].Line
+		})
+	}
+}
diff --git a/output/formatter.go b/output/formatter.go
@@ -44,6 +44,12 @@ const (
 )
 
 var text = `Results:
+{{range $filePath,$fileErrors := .Errors}}
+Golang errors in file: [{{ $filePath }}]:
+{{range $index, $error := $fileErrors}}
+  > [line {{$error.Line}} : column {{$error.Column}}] - {{$error.Err}}
+{{end}}
+{{end}}
 {{ range $index, $issue := .Issues }}
 [{{ $issue.File }}:{{ $issue.Line }}] - {{ $issue.RuleID }}: {{ $issue.What }} (Confidence: {{ $issue.Confidence}}, Severity: {{ $issue.Severity }})
   > {{ $issue.Code }}
@@ -58,14 +64,16 @@ Summary:
 `
 
 type reportInfo struct {
+	Errors map[string][]gosec.Error `json:"Golang errors"`
 	Issues []*gosec.Issue
 	Stats  *gosec.Metrics
 }
 
 // CreateReport generates a report based for the supplied issues and metrics given
 // the specified format. The formats currently accepted are: json, csv, html and text.
-func CreateReport(w io.Writer, format string, issues []*gosec.Issue, metrics *gosec.Metrics) error {
+func CreateReport(w io.Writer, format string, issues []*gosec.Issue, metrics *gosec.Metrics, errors map[string][]gosec.Error) error {
 	data := &reportInfo{
+		Errors: errors,
 		Issues: issues,
 		Stats:  metrics,
 	}

diff --git a/rules/rules_test.go b/rules/rules_test.go
@@ -47,7 +47,7 @@ var _ = Describe("gosec rules", func() {
 				Expect(err).ShouldNot(HaveOccurred())
 				err = analyzer.Process(buildTags, pkg.Path)
 				Expect(err).ShouldNot(HaveOccurred())
-				issues, _ := analyzer.Report()
+				issues, _, _ := analyzer.Report()
 				if len(issues) != sample.Errors {
 					fmt.Println(sample.Code)
 				}