From d92962cca3d47c77f8954e686736e1d9f6c45a2f Mon Sep 17 00:00:00 2001 From: Martin Vrachev Date: Mon, 25 Feb 2019 14:13:56 +0200 Subject: [PATCH 1/4] 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. I fix this problem in this patch and I added those Golang errors in the output. Signed-off-by: Martin Vrachev --- analyzer.go | 27 +++++++++++++++++++++++++-- analyzer_test.go | 16 ++++++++-------- cmd/gosec/main.go | 12 ++++++------ errors.go | 34 ++++++++++++++++++++++++++++++++++ output/formatter.go | 10 +++++++++- rules/rules_test.go | 2 +- 6 files changed, 83 insertions(+), 18 deletions(-) create mode 100644 errors.go diff --git a/analyzer.go b/analyzer.go index 62d7bc83c3..6148b07586 100644 --- a/analyzer.go +++ b/analyzer.go @@ -64,6 +64,7 @@ type Analyzer struct { logger *log.Logger issues []*Issue stats *Metrics + errors map[string][]Error } // NewAnalyzer builds a new analyzer. @@ -83,6 +84,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 +131,27 @@ 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(), ":") + newErr := NewError(infoErr[1], infoErr[2], strings.TrimSpace(infoErr[3])) + filePath := infoErr[0] + + 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 +256,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 index 49e97a105c..f6e7437c8f 100644 --- 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,7 +106,7 @@ 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)) }) @@ -124,7 +124,7 @@ var _ = Describe("Analyzer", func() { nosecPackage.Build() analyzer.Process(buildTags, nosecPackage.Path) - nosecIssues, _ := analyzer.Report() + nosecIssues, _, _ := analyzer.Report() Expect(nosecIssues).Should(BeEmpty()) }) @@ -141,7 +141,7 @@ var _ = Describe("Analyzer", func() { nosecPackage.Build() analyzer.Process(buildTags, nosecPackage.Path) - nosecIssues, _ := analyzer.Report() + nosecIssues, _, _ := analyzer.Report() Expect(nosecIssues).Should(BeEmpty()) }) @@ -158,7 +158,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 +175,7 @@ var _ = Describe("Analyzer", func() { nosecPackage.Build() analyzer.Process(buildTags, nosecPackage.Path) - nosecIssues, _ := analyzer.Report() + nosecIssues, _, _ := analyzer.Report() Expect(nosecIssues).Should(BeEmpty()) }) @@ -212,7 +212,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 index bab1e933d1..09e65da29b 100644 --- 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,7 +344,7 @@ 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) } @@ -352,7 +352,7 @@ func main() { 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 new file mode 100644 index 0000000000..5868ac66e1 --- /dev/null +++ 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 string `json:"line"` + Column string `json:"column"` + Err string `json:"error"` +} + +// NewError creates Error object +func NewError(line, column, 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 index eee693f94c..1301af9adb 100644 --- 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 index 48acf360a8..19dc329dc4 100644 --- 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) } From 9b01bbe517e026ad597a4387239d9712d2677b81 Mon Sep 17 00:00:00 2001 From: Martin Vrachev Date: Mon, 25 Feb 2019 15:11:16 +0200 Subject: [PATCH 2/4] Add comment for the map Signed-off-by: Martin Vrachev --- analyzer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzer.go b/analyzer.go index 6148b07586..2981c58ed9 100644 --- a/analyzer.go +++ b/analyzer.go @@ -64,7 +64,7 @@ type Analyzer struct { logger *log.Logger issues []*Issue stats *Metrics - errors map[string][]Error + errors map[string][]Error // keys are file paths; values are the golang errors in those files } // NewAnalyzer builds a new analyzer. From 77304046be38bfe1c393f10b5716a22f7fa8f0d6 Mon Sep 17 00:00:00 2001 From: Martin Vrachev Date: Mon, 25 Feb 2019 17:02:59 +0200 Subject: [PATCH 3/4] Change a little the format Signed-off-by: Martin Vrachev --- output/formatter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/output/formatter.go b/output/formatter.go index 1301af9adb..61810fc0c5 100644 --- a/output/formatter.go +++ b/output/formatter.go @@ -47,7 +47,7 @@ var text = `Results: {{range $filePath,$fileErrors := .Errors}} Golang errors in file: [{{ $filePath }}]: {{range $index, $error := $fileErrors}} - > [line{{$error.Line}} : column{{$error.Column}}] - {{$error.Err}} + > [line {{$error.Line}} : column {{$error.Column}}] - {{$error.Err}} {{end}} {{end}} {{ range $index, $issue := .Issues }} From 759baed7c089bf6f6d339b707ba23b942c34efe3 Mon Sep 17 00:00:00 2001 From: Martin Vrachev Date: Mon, 25 Feb 2019 19:24:53 +0200 Subject: [PATCH 4/4] Add unit test + error struct changes I added unit tests where I give an invalid file to Gosec. Also, I realized that is more logical if the line and the column attributes in the errors struct are integers. Signed-off-by: Martin Vrachev --- analyzer.go | 11 ++++++++++- analyzer_test.go | 25 +++++++++++++++++++++++++ errors.go | 6 +++--- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/analyzer.go b/analyzer.go index 2981c58ed9..3ab3119f68 100644 --- a/analyzer.go +++ b/analyzer.go @@ -26,6 +26,7 @@ import ( "path" "reflect" "regexp" + "strconv" "strings" "golang.org/x/tools/go/loader" @@ -139,8 +140,16 @@ func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error // at index 1 is the line; index 2 is for column // at index 3 is the actual error infoErr := strings.Split(packErr.Error(), ":") - newErr := NewError(infoErr[1], infoErr[2], strings.TrimSpace(infoErr[3])) 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) diff --git a/analyzer_test.go b/analyzer_test.go index f6e7437c8f..69516c6f14 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -111,6 +111,31 @@ var _ = Describe("Analyzer", func() { }) + 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] diff --git a/errors.go b/errors.go index 5868ac66e1..3e0edfbe1e 100644 --- a/errors.go +++ b/errors.go @@ -6,13 +6,13 @@ import ( // Error is used when there are golang errors while parsing the AST type Error struct { - Line string `json:"line"` - Column string `json:"column"` + Line int `json:"line"` + Column int `json:"column"` Err string `json:"error"` } // NewError creates Error object -func NewError(line, column, err string) *Error { +func NewError(line, column int, err string) *Error { return &Error{ Line: line, Column: column,