From f61230de99f6482499309a6e94ddcc37f41b4bc5 Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Wed, 14 Aug 2024 17:07:59 +0200 Subject: [PATCH 01/13] Added new rule G407(hardcoded IV/nonce) The rule is supposed to detect for the usage of hardcoded or static nonce/Iv in many encryption algorithms: * The different modes of AES (mainly tested here) * It should be able to work with ascon Currently the rules doesn't check when constant variables are used. TODO: Improve the rule, to detected for constatant variable usage --- README.md | 1 + analyzer_test.go | 307 ++++++++++++++++++++++++++++++ cwe/data.go | 5 + issue/issue.go | 1 + report/formatter_test.go | 4 +- rules/hardcodedIV.go | 132 +++++++++++++ rules/rulelist.go | 1 + rules/rules_test.go | 60 ++++++ testutils/g407_samples.go | 380 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 889 insertions(+), 2 deletions(-) create mode 100644 rules/hardcodedIV.go create mode 100644 testutils/g407_samples.go diff --git a/README.md b/README.md index a9384c970c..7b8b526a83 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,7 @@ directory you can supply `./...` as the input argument. - G404: Insecure random number source (rand) - G405: Detect the usage of DES or RC4 - G406: Detect the usage of MD4 or RIPEMD160 +- G407: Detect the usage of hardcoded Initialization Vector(IV)/Nonce - G501: Import blocklist: crypto/md5 - G502: Import blocklist: crypto/des - G503: Import blocklist: crypto/rc4 diff --git a/analyzer_test.go b/analyzer_test.go index 30ca17094a..301f194c40 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -187,6 +187,22 @@ var _ = Describe("Analyzer", func() { Expect(controlIssues).Should(HaveLen(sample.Errors)) }) + It("should find errors when nosec is not in use", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + controlPackage := testutils.NewTestPackage() + defer controlPackage.Close() + controlPackage.AddFile("aesOFB.go", source) + err := controlPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, controlPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + controlIssues, _, _ := analyzer.Report() + Expect(controlIssues).Should(HaveLen(sample.Errors)) + }) + It("should report Go build errors and invalid files", func() { analyzer.LoadRules(rules.Generate(false).RulesInfo()) pkg := testutils.NewTestPackage() @@ -266,6 +282,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should not report errors when a nosec line comment is present", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should not report errors when a nosec block comment is present", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -317,6 +350,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should not report errors when a nosec block comment is present", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) /* #nosec */", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should not report errors when an exclude comment is present for the correct rule", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -371,6 +421,24 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should not report errors when an exclude comment is present for the correct rule", func() { + // Rule for hardcoded nonce/IV + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G407", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should not report errors when a nosec block and line comment are present", func() { sample := testutils.SampleCodeG101[23] source := sample.Code[0] @@ -468,6 +536,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) + It("should report errors when an exclude comment is present for a different rule", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G301", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(HaveLen(sample.Errors)) + }) + It("should not report errors when an exclude comment is present for multiple rules, including the correct rule", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -525,6 +610,25 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should not report errors when an exclude comment is present for multiple rules, including the correct rule", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G301 G407", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should pass the build tags", func() { sample := testutils.SampleCodeBuildTag[0] source := sample.Code[0] @@ -621,6 +725,29 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) + It("should be possible to overwrite nosec comments, and report issues", func() { + // Rule for hardcoded IV/nonce + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + + // overwrite nosec option + nosecIgnoreConfig := gosec.NewConfig() + nosecIgnoreConfig.SetGlobal(gosec.Nosec, "true") + customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) + customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = customAnalyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := customAnalyzer.Report() + Expect(nosecIssues).Should(HaveLen(sample.Errors)) + }) + It("should be possible to overwrite nosec comments, and report issues but they should not be counted", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -699,6 +826,32 @@ var _ = Describe("Analyzer", func() { Expect(metrics.NumNosec).Should(Equal(1)) }) + It("should be possible to overwrite nosec comments, and report issues but they should not be counted", func() { + // Rule for hardcoded nonce/IV + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + + // overwrite nosec option + nosecIgnoreConfig := gosec.NewConfig() + nosecIgnoreConfig.SetGlobal(gosec.Nosec, "mynosec") + nosecIgnoreConfig.SetGlobal(gosec.ShowIgnored, "true") + customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) + customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) // #mynosec", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = customAnalyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, metrics, _ := customAnalyzer.Report() + Expect(nosecIssues).Should(HaveLen(sample.Errors)) + Expect(metrics.NumFound).Should(Equal(0)) + Expect(metrics.NumNosec).Should(Equal(1)) + }) + It("should not report errors when nosec tag is in front of a line", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -750,6 +903,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should not report errors when nosec tag is in front of a line", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//Some description\n//#nosec G407\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should report errors when nosec tag is not in front of a line", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -801,6 +971,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) + It("should report errors when nosec tag is not in front of a line", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//Some description\n//Another description #nosec G407\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(HaveLen(sample.Errors)) + }) + It("should not report errors when rules are in front of nosec tag even rules are wrong", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -852,6 +1039,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should not report errors when rules are in front of nosec tag even rules are wrong", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//G301\n//#nosec\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should report errors when there are nosec tags after a #nosec WrongRuleList annotation", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -903,6 +1107,23 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) + It("should report errors when there are nosec tags after a #nosec WrongRuleList annotation", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//#nosec\n//G301\n//#nosec\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := analyzer.Report() + Expect(nosecIssues).Should(HaveLen(sample.Errors)) + }) + It("should be possible to use an alternative nosec tag", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -972,6 +1193,29 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should be possible to use an alternative nosec tag", func() { + // Rule for hardcoded nonce/IV + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + + // overwrite nosec option + nosecIgnoreConfig := gosec.NewConfig() + nosecIgnoreConfig.SetGlobal(gosec.NoSecAlternative, "falsePositive") + customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) + customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) // #falsePositive", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = customAnalyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := customAnalyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should ignore vulnerabilities when the default tag is found", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -1041,6 +1285,29 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) + It("should ignore vulnerabilities when the default tag is found", func() { + // Rule for hardcoded nonce/IV + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + + // overwrite nosec option + nosecIgnoreConfig := gosec.NewConfig() + nosecIgnoreConfig.SetGlobal(gosec.NoSecAlternative, "falsePositive") + customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) + customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = customAnalyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + nosecIssues, _, _ := customAnalyzer.Report() + Expect(nosecIssues).Should(BeEmpty()) + }) + It("should be able to analyze Go test package", func() { customAnalyzer := gosec.NewAnalyzer(nil, true, false, false, 1, logger) customAnalyzer.LoadRules(rules.Generate(false).RulesInfo()) @@ -1404,6 +1671,26 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("Justification")) }) + It("should not report an error if the violation is suppressed", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G407 -- Justification", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + issues, _, _ := analyzer.Report() + Expect(issues).To(HaveLen(sample.Errors)) + Expect(issues[0].Suppressions).To(HaveLen(1)) + Expect(issues[0].Suppressions[0].Kind).To(Equal("inSource")) + Expect(issues[0].Suppressions[0].Justification).To(Equal("Justification")) + }) + It("should not report an error if the violation is suppressed without certain rules", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -1464,6 +1751,26 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("")) }) + It("should not report an error if the violation is suppressed without certain rules", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) + + nosecPackage := testutils.NewTestPackage() + defer nosecPackage.Close() + nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) + nosecPackage.AddFile("aesOFB.go", nosecSource) + err := nosecPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, nosecPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + issues, _, _ := analyzer.Report() + Expect(issues).To(HaveLen(sample.Errors)) + Expect(issues[0].Suppressions).To(HaveLen(1)) + Expect(issues[0].Suppressions[0].Kind).To(Equal("inSource")) + Expect(issues[0].Suppressions[0].Justification).To(Equal("")) + }) + It("should not report an error if the rule is not included", func() { sample := testutils.SampleCodeG101[0] source := sample.Code[0] diff --git a/cwe/data.go b/cwe/data.go index b472626086..a9568ba4d5 100644 --- a/cwe/data.go +++ b/cwe/data.go @@ -133,6 +133,11 @@ var idWeaknesses = map[string]*Weakness{ Description: "The software contains hard-coded credentials, such as a password or cryptographic key, which it uses for its own inbound authentication, outbound communication to external components, or encryption of internal data.", Name: "Use of Hard-coded Credentials", }, + "1204": { + ID: "1204", + Description: "The product uses a cryptographic primitive that uses an Initialization Vector (IV), but the product does not generate IVs that are sufficiently unpredictable or unique according to the expected cryptographic requirements for that primitive.", + Name: "Generation of Weak Initialization Vector (IV)", + }, } // Get Retrieves a CWE weakness by it's id diff --git a/issue/issue.go b/issue/issue.go index aad093422f..6227db670e 100644 --- a/issue/issue.go +++ b/issue/issue.go @@ -84,6 +84,7 @@ var ruleToCWE = map[string]string{ "G404": "338", "G405": "327", "G406": "328", + "G407": "1204", "G501": "327", "G502": "327", "G503": "327", diff --git a/report/formatter_test.go b/report/formatter_test.go index 03d086cc22..ad22ee063a 100644 --- a/report/formatter_test.go +++ b/report/formatter_test.go @@ -281,8 +281,8 @@ var _ = Describe("Formatter", func() { "G101", "G102", "G103", "G104", "G106", "G107", "G109", "G110", "G111", "G112", "G113", "G201", "G202", "G203", "G204", "G301", "G302", "G303", "G304", "G305", "G401", - "G402", "G403", "G404", "G405", "G406", "G501", "G502", - "G503", "G504", "G505", "G506", "G507", "G601", + "G402", "G403", "G404", "G405", "G406", "G407", "G501", + "G502", "G503", "G504", "G505", "G506", "G507", "G601", } It("csv formatted report should contain the CWE mapping", func() { diff --git a/rules/hardcodedIV.go b/rules/hardcodedIV.go new file mode 100644 index 0000000000..2c7f5ab0c9 --- /dev/null +++ b/rules/hardcodedIV.go @@ -0,0 +1,132 @@ +package rules + +import ( + "go/ast" + + "github.com/securego/gosec/v2" + "github.com/securego/gosec/v2/issue" +) + +type usesHardcodedIV struct { + issue.MetaData + trackedFunctions map[string][]int +} + +func (r *usesHardcodedIV) ID() string { + return r.MetaData.ID +} + +// The code is a little bit spaghetti and there are things that repeat +// Can be improved +func (r *usesHardcodedIV) Match(n ast.Node, c *gosec.Context) (*issue.Issue, error) { + // cast n to a call expression, we can do that safely, because this match method gets only called when CallExpr node is found + funcCall := n.(*ast.CallExpr) + + // cast to a function call from an object and get the function part; example: a.doSomething() + funcSelector, exists := funcCall.Fun.(*ast.SelectorExpr) + if exists { + //Iterate trough the wanted functions + for functionName, functionNumArgsAndNoncePosArr := range r.trackedFunctions { + // Check if the call is actually made from an object + if _, hasX := funcSelector.X.(*ast.Ident); hasX { + + // Check if the function name matches with the one we look for, and if the function accepts an exact number of arguments(Function signature) + if funcSelector.Sel.Name == functionName && len(funcCall.Args) == functionNumArgsAndNoncePosArr[0] { + + // Check the type of the passed argument to the function + switch funcCall.Args[functionNumArgsAndNoncePosArr[1]].(type) { + + case *ast.CompositeLit: + // Check if the argument is static array + if _, isArray := funcCall.Args[functionNumArgsAndNoncePosArr[1]].(*ast.CompositeLit).Type.(*ast.ArrayType); isArray { + return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array", r.Severity, r.Confidence), nil + } + + case *ast.CallExpr: + + // Check if it's a function call, because []byte() is a function call, and also check if the number of arguments to this call is only 1 + switch funcCall.Args[functionNumArgsAndNoncePosArr[1]].(*ast.CallExpr).Fun.(type) { + case *ast.ArrayType: + return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array", r.Severity, r.Confidence), nil + + // Check if it's an anonymous function + case *ast.FuncLit: + functionCalled, _ := funcCall.Args[functionNumArgsAndNoncePosArr[1]].(*ast.CallExpr).Fun.(*ast.FuncLit) + + // Check the type of the last statement in the anonymous function + switch functionCalled.Body.List[len(functionCalled.Body.List)-1].(type) { + + case *ast.IfStmt: + + ifStatementContent := functionCalled.Body.List[len(functionCalled.Body.List)-1].(*ast.IfStmt).Body.List + + // check if the if statement has return statement + if retStatement, isReturn := ifStatementContent[len(ifStatementContent)-1].(*ast.ReturnStmt); isReturn { + argInNestedFunc := retStatement.Results[0] + + // check the type of the returned value + switch argInNestedFunc.(type) { + case *ast.CompositeLit: + // Check if the argument is static array + if _, isArray := argInNestedFunc.(*ast.CompositeLit).Type.(*ast.ArrayType); isArray { + return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array in a function call", r.Severity, r.Confidence), nil + } + + case *ast.CallExpr: + if _, ok := argInNestedFunc.(*ast.CallExpr).Fun.(*ast.ArrayType); ok { + return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array in a function call", r.Severity, r.Confidence), nil + } + } + } + case *ast.ReturnStmt: + + argInNestedFunc := functionCalled.Body.List[len(functionCalled.Body.List)-1].(*ast.ReturnStmt).Results[0] + switch argInNestedFunc.(type) { + case *ast.CompositeLit: + // Check if the argument is static array + if _, isArray := argInNestedFunc.(*ast.CompositeLit).Type.(*ast.ArrayType); isArray { + return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array in a function call", r.Severity, r.Confidence), nil + } + + case *ast.CallExpr: + if _, ok := argInNestedFunc.(*ast.CallExpr).Fun.(*ast.ArrayType); ok { + return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array in a function call", r.Severity, r.Confidence), nil + } + } + } + } + } + } + } + } + } + // loop through the functions we are checking + + return nil, nil +} + +func NewUsesHardCodedIV(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { + calls := make(map[string][]int) + // Holds the function name as key, the number of arguments that the function accepts, and at which index of those accepted arguments is the nonce/IV + // Example "Test" 3, 1 -- means the function "Test" which accepts 3 arguments, and has the nonce arg as second argument + + calls["Seal"] = []int{4, 1} + calls["Open"] = []int{4, 1} + calls["NewCBCDecrypter"] = []int{2, 1} // + calls["NewCBCEncrypter"] = []int{2, 1} // + calls["NewCFBDecrypter"] = []int{2, 1} + calls["NewCFBEncrypter"] = []int{2, 1} + calls["NewCTR"] = []int{2, 1} // + calls["NewOFB"] = []int{2, 1} // + + rule := &usesHardcodedIV{ + trackedFunctions: calls, + MetaData: issue.MetaData{ + ID: id, + Severity: issue.High, + Confidence: issue.Medium, + What: "Use of hardcoded IV/nonce for encryption", + }, + } + return rule, []ast.Node{(*ast.CallExpr)(nil)} +} diff --git a/rules/rulelist.go b/rules/rulelist.go index 13f29f71aa..889413cda7 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -100,6 +100,7 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { {"G404", "Insecure random number source (rand)", NewWeakRandCheck}, {"G405", "Detect the usage of DES or RC4", NewUsesWeakCryptographyEncryption}, {"G406", "Detect the usage of deprecated MD4 or RIPEMD160", NewUsesWeakDeprecatedCryptographyHash}, + {"G407", "Detect the usage of hardcoded Initialization Vector(IV)/Nonce", NewUsesHardCodedIV}, // blocklist {"G501", "Import blocklist: crypto/md5", NewBlocklistedImportMD5}, diff --git a/rules/rules_test.go b/rules/rules_test.go index 9a7d65a2f1..783ab878b3 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -187,6 +187,66 @@ var _ = Describe("gosec rules", func() { runner("G406", testutils.SampleCodeG406b) }) + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407b) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407c) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407d) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407e) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407f) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407g) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407h) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407i) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407j) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407k) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407l) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407m) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407n) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407o) + }) + It("should detect blocklisted imports - MD5", func() { runner("G501", testutils.SampleCodeG501) }) diff --git a/testutils/g407_samples.go b/testutils/g407_samples.go new file mode 100644 index 0000000000..675aa06752 --- /dev/null +++ b/testutils/g407_samples.go @@ -0,0 +1,380 @@ +package testutils + +import "github.com/securego/gosec/v2" + +var ( + // SampleCodeG407 - Use of hardcoded nonce/IV + SampleCodeG407 = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesOFB := cipher.NewOFB(block, []byte("ILoveMyNonceAlot")) + var output = make([]byte, 16) + aesOFB.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + +}`}, 1, gosec.NewConfig()}, + } + + // SampleCodeG407b - Use of hardcoded nonce/IV + SampleCodeG407b = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesOFB := cipher.NewOFB(block, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + var output = make([]byte, 16) + aesOFB.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + +}`}, 1, gosec.NewConfig()}, + } + + // SampleCodeG407c - Use of hardcoded nonce/IV + SampleCodeG407c = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCTR := cipher.NewCTR(block, []byte("ILoveMyNonceAlot")) + var output = make([]byte, 16) + aesCTR.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + +}`}, 1, gosec.NewConfig()}, + } + + // SampleCodeG407d - Use of hardcoded nonce/IV + SampleCodeG407d = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCTR := cipher.NewCTR(block, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + var output = make([]byte, 16) + aesCTR.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + +} +`}, 1, gosec.NewConfig()}, + } + + // SampleCodeG407e - Use of hardcoded nonce/IV + SampleCodeG407e = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + + cipherText := aesGCM.Seal(nil, []byte("ILoveMyNonce"), []byte("My secret message"), nil) + fmt.Println(string(cipherText)) + cipherText, _ = aesGCM.Open(nil, []byte("ILoveMyNonce"), cipherText, nil) + fmt.Println(string(cipherText)) +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407f - Use of hardcoded nonce/IV + SampleCodeG407f = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + cipherText := aesGCM.Seal(nil, []byte{}, []byte("My secret message"), nil) + fmt.Println(string(cipherText)) + + cipherText, _ = aesGCM.Open(nil, []byte{}, cipherText, nil) + fmt.Println(string(cipherText)) +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407g - Use of hardcoded nonce/IV + SampleCodeG407g = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + + cipherText := aesGCM.Seal(nil, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}, []byte("My secret message"), nil) + fmt.Println(string(cipherText)) + + cipherText, _ = aesGCM.Open(nil, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}, cipherText, nil) + fmt.Println(string(cipherText)) +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407h - Use of hardcoded nonce/IV + SampleCodeG407h = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher( []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + + cipherText := aesGCM.Seal(nil, func() []byte { + if true { + return []byte("ILoveMyNonce") + } else { + return []byte("IDont'Love..") + } + }(), []byte("My secret message"), nil) + fmt.Println(string(cipherText)) + + cipherText, _ = aesGCM.Open(nil, func() []byte { + if true { + return []byte("ILoveMyNonce") + } else { + return []byte("IDont'Love..") + } + }(), cipherText, nil) + + fmt.Println(string(cipherText)) +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407I - Use of hardcoded nonce/IV + SampleCodeG407i = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + + cipherText := aesGCM.Seal(nil, func() []byte { + if true { + return []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} + } else { + return []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} + } + }(), []byte("My secret message"), nil) + fmt.Println(string(cipherText)) + + cipherText, _ = aesGCM.Open(nil, func() []byte { + if true { + return []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} + } else { + return []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} + } + }(), cipherText, nil) + fmt.Println(string(cipherText)) +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407j - Use of hardcoded nonce/IV + SampleCodeG407j = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + cipheredText := aesGCM.Seal(nil, func() []byte { return []byte("ILoveMyNonce") }(), []byte("My secret message"), nil) + fmt.Println(string(cipheredText)) + cipheredText, _ = aesGCM.Open(nil, func() []byte { return []byte("ILoveMyNonce") }(), cipheredText, nil) + fmt.Println(string(cipheredText)) + +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407k - Use of hardcoded nonce/IV + SampleCodeG407k = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + cipheredText := aesGCM.Seal(nil, func() []byte { return []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} }(), []byte("My secret message"), nil) + fmt.Println(string(cipheredText)) + cipheredText, _ = aesGCM.Open(nil, func() []byte { return []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} }(), cipheredText, nil) + fmt.Println(string(cipheredText)) + +} +`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407l - Use of hardcoded nonce/IV + SampleCodeG407l = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCFB := cipher.NewCFBEncrypter(block, []byte("ILoveMyNonceAlot")) + var output = make([]byte, 16) + aesCFB.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + aesCFB = cipher.NewCFBDecrypter(block, []byte("ILoveMyNonceAlot")) + aesCFB.XORKeyStream(output, output) + fmt.Println(string(output)) + +}`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407m - Use of hardcoded nonce/IV + SampleCodeG407m = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCFB := cipher.NewCFBEncrypter(block, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + var output = make([]byte, 16) + aesCFB.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + aesCFB = cipher.NewCFBDecrypter(block, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCFB.XORKeyStream(output, output) + fmt.Println(string(output)) + +}`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407n - Use of hardcoded nonce/IV + SampleCodeG407n = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCBC := cipher.NewCBCEncrypter(block, []byte("ILoveMyNonceAlot")) + + var output = make([]byte, 16) + aesCBC.CryptBlocks(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + + aesCBC = cipher.NewCBCDecrypter(block, []byte("ILoveMyNonceAlot")) + aesCBC.CryptBlocks(output, output) + fmt.Println(string(output)) + +}`}, 2, gosec.NewConfig()}, + } + + // SampleCodeG407o - Use of hardcoded nonce/IV + SampleCodeG407o = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCBC := cipher.NewCBCEncrypter(block, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + + var output = make([]byte, 16) + aesCBC.CryptBlocks(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + + aesCBC = cipher.NewCBCDecrypter(block, []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCBC.CryptBlocks(output, output) + fmt.Println(string(output)) + +} +`}, 2, gosec.NewConfig()}, + } +) From f5c50ede3249745c70d94a6791751a1ccd4bca5d Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Wed, 21 Aug 2024 15:29:34 +0200 Subject: [PATCH 02/13] Refractored code a little bit --- rules/hardcodedIV.go | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/rules/hardcodedIV.go b/rules/hardcodedIV.go index 2c7f5ab0c9..2acda68087 100644 --- a/rules/hardcodedIV.go +++ b/rules/hardcodedIV.go @@ -23,42 +23,46 @@ func (r *usesHardcodedIV) Match(n ast.Node, c *gosec.Context) (*issue.Issue, err funcCall := n.(*ast.CallExpr) // cast to a function call from an object and get the function part; example: a.doSomething() - funcSelector, exists := funcCall.Fun.(*ast.SelectorExpr) - if exists { - //Iterate trough the wanted functions - for functionName, functionNumArgsAndNoncePosArr := range r.trackedFunctions { - // Check if the call is actually made from an object - if _, hasX := funcSelector.X.(*ast.Ident); hasX { - - // Check if the function name matches with the one we look for, and if the function accepts an exact number of arguments(Function signature) + + if funcSelector, isSelector := funcCall.Fun.(*ast.SelectorExpr); isSelector { + + // Check if the call is actually made from an object + if _, hasX := funcSelector.X.(*ast.Ident); hasX { + + //Iterate trough the wanted functions + for functionName, functionNumArgsAndNoncePosArr := range r.trackedFunctions { + + // Check if the function name matches with the one we look for, and if the function accepts an exact number of arguments(rough function signature check) if funcSelector.Sel.Name == functionName && len(funcCall.Args) == functionNumArgsAndNoncePosArr[0] { // Check the type of the passed argument to the function - switch funcCall.Args[functionNumArgsAndNoncePosArr[1]].(type) { + switch trackedFunctionPassedArgType := funcCall.Args[functionNumArgsAndNoncePosArr[1]].(type) { + // {} used case *ast.CompositeLit: // Check if the argument is static array - if _, isArray := funcCall.Args[functionNumArgsAndNoncePosArr[1]].(*ast.CompositeLit).Type.(*ast.ArrayType); isArray { + if _, isArray := trackedFunctionPassedArgType.Type.(*ast.ArrayType); isArray { return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array", r.Severity, r.Confidence), nil } + // () used case *ast.CallExpr: // Check if it's a function call, because []byte() is a function call, and also check if the number of arguments to this call is only 1 - switch funcCall.Args[functionNumArgsAndNoncePosArr[1]].(*ast.CallExpr).Fun.(type) { + switch trackedFunctionPassedArgType.Fun.(type) { case *ast.ArrayType: return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array", r.Severity, r.Confidence), nil - // Check if it's an anonymous function + // Check if the argument passed is another function case *ast.FuncLit: - functionCalled, _ := funcCall.Args[functionNumArgsAndNoncePosArr[1]].(*ast.CallExpr).Fun.(*ast.FuncLit) + functionCalled, _ := trackedFunctionPassedArgType.Fun.(*ast.FuncLit) // Check the type of the last statement in the anonymous function - switch functionCalled.Body.List[len(functionCalled.Body.List)-1].(type) { + switch calledFunctionLastInstructionType := functionCalled.Body.List[len(functionCalled.Body.List)-1].(type) { case *ast.IfStmt: - ifStatementContent := functionCalled.Body.List[len(functionCalled.Body.List)-1].(*ast.IfStmt).Body.List + ifStatementContent := calledFunctionLastInstructionType.Body.List // check if the if statement has return statement if retStatement, isReturn := ifStatementContent[len(ifStatementContent)-1].(*ast.ReturnStmt); isReturn { @@ -80,7 +84,7 @@ func (r *usesHardcodedIV) Match(n ast.Node, c *gosec.Context) (*issue.Issue, err } case *ast.ReturnStmt: - argInNestedFunc := functionCalled.Body.List[len(functionCalled.Body.List)-1].(*ast.ReturnStmt).Results[0] + argInNestedFunc := calledFunctionLastInstructionType.Results[0] switch argInNestedFunc.(type) { case *ast.CompositeLit: // Check if the argument is static array From f6bd7b73e8fa593f610814d96e35aa179d628631 Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Thu, 29 Aug 2024 17:40:14 +0200 Subject: [PATCH 03/13] Migrated the rule to the analyzers folder --- analyzers/hardcodedNonce.go | 231 ++++++++++++++++++++++++++++++++++++ analyzers/util.go | 9 ++ rules/hardcodedIV.go | 136 --------------------- rules/rulelist.go | 1 - rules/rules_test.go | 16 +++ testutils/g407_samples.go | 94 +++++++++++++++ 6 files changed, 350 insertions(+), 137 deletions(-) create mode 100644 analyzers/hardcodedNonce.go delete mode 100644 rules/hardcodedIV.go diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go new file mode 100644 index 0000000000..2d1d343542 --- /dev/null +++ b/analyzers/hardcodedNonce.go @@ -0,0 +1,231 @@ +// (c) Copyright gosec's authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package analyzers + +import ( + "errors" + "fmt" + "go/token" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" + "golang.org/x/tools/go/ssa" + + "github.com/securego/gosec/v2/issue" +) + +const defaultWhat = "Use of hardcoded IV/nonce for encryption" + +func newHardCodedNonce(id string, description string) *analysis.Analyzer { + return &analysis.Analyzer{ + Name: id, + Doc: description, + Run: runHardCodedNonce, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, + } +} + +func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { + ssaResult, err := getSSAResult(pass) + if err != nil { + return nil, fmt.Errorf("building ssa representation: %w", err) + } + + // Holds the function name as key, the number of arguments that the function accepts, and at which index of those accepted arguments is the nonce/IV + // Example "Test" 3, 1 -- means the function "Test" which accepts 3 arguments, and has the nonce arg as second argument + calls := map[string][]int{ + "(crypto/cipher.AEAD).Seal": {4, 1}, + "(crypto/cipher.AEAD).Open": {4, 1}, + "crypto/cipher.NewCBCDecrypter": {2, 1}, + "crypto/cipher.NewCBCEncrypter": {2, 1}, + "crypto/cipher.NewCFBDecrypter": {2, 1}, + "crypto/cipher.NewCFBEncrypter": {2, 1}, + "crypto/cipher.NewCTR": {2, 1}, + "crypto/cipher.NewOFB": {2, 1}, + } + var issues []*issue.Issue + var ssaPkgFunctions = ssaResult.SSA.SrcFuncs + var savedArgsFromFunctions = *iterateAndGetArgsFromTrackedFunctions(ssaPkgFunctions, &calls) + + for _, savedArg := range savedArgsFromFunctions { + tmp, err := raiseIssue(savedArg, &calls, ssaPkgFunctions, pass, "") + issues = append(issues, tmp...) + if err != nil { + return nil, err + } + } + return issues, nil +} + +func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa.Function, pass *analysis.Pass, issueDescription string) ([]*issue.Issue, error) { + var err error = nil + var gosecIssue []*issue.Issue = nil + + if issueDescription == "" { + issueDescription = defaultWhat + } + + switch valType := (*val).(type) { + case *ssa.Slice: + issueDescription += " by passing hardcoded slice/array" + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.High) + gosecIssue = append(gosecIssue, tmp...) + err = hasErr + + case *ssa.UnOp: + // Check if it's a dereference operation (a.k.a pointer) + if valType.Op == token.MUL { + issueDescription += " by passing pointer which points to hardcoded variable" + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.Low) + gosecIssue = append(gosecIssue, tmp...) + err = hasErr + } + + // When the value assigned to a variable is a function call. + // It goes and check if this function contains call to crypto/rand.Read in it's body(Assuming that calling crypto/rand.Read in a function, is used for the generation of nonce/iv ) + case *ssa.Call: + if calledFunction, ok := valType.Call.Value.(*ssa.Function); ok { + if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr != nil { + issueDescription += " by passing a value from function which doesn't use crypto/rand" + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.Medium) + gosecIssue = append(gosecIssue, tmp...) + err = hasErr + } + } + + // only checks from strings->[]byte + // might need to add additional types + case *ssa.Convert: + if valType.Type().String() == "[]byte" && valType.X.Type().String() == "string" { + issueDescription += " by passing converted string" + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.High) + gosecIssue = append(gosecIssue, tmp...) + err = hasErr + } + + case *ssa.Parameter: + //arg given to tracked function is wrapped in another function, example: + // func encrypt(..,nonce,...){ + // aesgcm.Seal() + //} + // save parameter position, by checking the name of the variable used in tracked functions and comparing it with the name of the arg + if valType.Parent() != nil { + trackedFunctions := make(map[string][]int) + for index, funcArgs := range valType.Parent().Params { + if funcArgs.Name() == valType.Name() && funcArgs.Type() == valType.Type() { + trackedFunctions[valType.Parent().String()] = []int{len(valType.Parent().Params), index} + } + } + result := iterateAndGetArgsFromTrackedFunctions(ssaFuncs, &trackedFunctions) + + issueDescription += " by passing a parameter to a function and" + // recursively backtrack to where the origin of a variable passed to multiple functions is + for _, trackedVariable := range *result { + + tmp, hasErr := raiseIssue(trackedVariable, &trackedFunctions, ssaFuncs, pass, issueDescription) + gosecIssue = append(gosecIssue, tmp...) + err = hasErr + } + } + } + return gosecIssue, err +} + +// Iterate through all places that use the `variable` argument and check if it's used in one of the tracked functions +func iterateThroughReferrers(variable *ssa.Value, funcsToTrack *map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueSeverity issue.Score, issueConfidence issue.Score) ([]*issue.Issue, error) { + + if funcsToTrack == nil || variable == nil || analyzerID == "" || issueDescription == "" || fileSet == nil { + return nil, errors.New("received a nil object") + } + var gosecIssues []*issue.Issue = nil + // Go trough all functions that use the given arg variable + for _, referrer := range *(*variable).Referrers() { + + // Iterate trough the functions we are interested + for trackedFunc := range *funcsToTrack { + + // Split the functions we are interested in, by the '.' because we will use the function name to do the comparison + // MIGHT GIVE SOME FALSE POSITIVES THIS WAY + trackedFuncParts := strings.Split(trackedFunc, ".") + trackedFuncPartsName := trackedFuncParts[len(trackedFuncParts)-1] + if strings.Contains(referrer.String(), trackedFuncPartsName) { + gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, referrer.Pos(), issueSeverity, issueConfidence)) + } + } + } + return gosecIssues, nil +} + +// Check whether a function contains a call to crypto/rand.Read in it's function body +func isFuncContainsCryptoRand(funcCall *ssa.Function) (bool, error) { + if funcCall == nil { + return false, errors.New("passed ssa.Function object is nil") + } + for _, block := range funcCall.Blocks { + for _, instr := range block.Instrs { + if call, ok := instr.(*ssa.Call); ok { + if calledFunction, ok := call.Call.Value.(*ssa.Function); ok { + if calledFunction.Pkg != nil && calledFunction.Pkg.Pkg.Path() == "crypto/rand" && calledFunction.Name() == "Read" { + return true, nil + } + } + } + } + } + return false, nil +} + +func addToVarsMap(value *ssa.Value, mapToAddTo *map[string]*ssa.Value) { + key := (*value).Name() + (*value).Type().String() + (*value).String() + (*value).Parent().String() + (*mapToAddTo)[key] = value +} + +func isContainedInMap(value *ssa.Value, mapToCheck *map[string]*ssa.Value) bool { + key := (*value).Name() + (*value).Type().String() + (*value).String() + (*value).Parent().String() + _, contained := (*mapToCheck)[key] + return contained +} + +func iterateAndGetArgsFromTrackedFunctions(ssaFuncs []*ssa.Function, trackedFunc *map[string][]int) *map[string]*ssa.Value { + values := make(map[string]*ssa.Value) + for _, pkgFunc := range ssaFuncs { + for _, funcBlock := range pkgFunc.Blocks { + for _, funcBlocInstr := range funcBlock.Instrs { + if funcCall, ok := funcBlocInstr.(*ssa.Call); ok { + for trackedFuncName, trackedFuncArgsInfo := range *trackedFunc { + // only process functions that have the same number of arguments as the ones we track + if len(funcCall.Call.Args) == trackedFuncArgsInfo[0] { + tmpArg := funcCall.Call.Args[trackedFuncArgsInfo[1]] + // check if the function is called from an object or directly from the package + if funcCall.Call.Method != nil { + if methodFullname := funcCall.Call.Method.FullName(); methodFullname == trackedFuncName { + if !isContainedInMap(&tmpArg, &values) { + addToVarsMap(&tmpArg, &values) + } + } + } else if funcCall.Call.Value.String() == trackedFuncName { + if !isContainedInMap(&tmpArg, &values) { + addToVarsMap(&tmpArg, &values) + } + } + } + } + } + } + } + } + return &values +} diff --git a/analyzers/util.go b/analyzers/util.go index e88afe7bdb..e7961a56b7 100644 --- a/analyzers/util.go +++ b/analyzers/util.go @@ -35,6 +35,15 @@ type SSAAnalyzerResult struct { SSA *buildssa.SSA } +// BuildDefaultAnalyzers returns the default list of analyzers +func BuildDefaultAnalyzers() []*analysis.Analyzer { + return []*analysis.Analyzer{ + newConversionOverflowAnalyzer("G115", "Type conversion which leads to integer overflow"), + newSliceBoundsAnalyzer("G602", "Possible slice bounds out of range"), + newHardCodedNonce("G407", "Use of hardcoded IV/nonce for encryption"), + } +} + // getSSAResult retrieves the SSA result from analysis pass func getSSAResult(pass *analysis.Pass) (*SSAAnalyzerResult, error) { result, ok := pass.ResultOf[buildssa.Analyzer] diff --git a/rules/hardcodedIV.go b/rules/hardcodedIV.go deleted file mode 100644 index 2acda68087..0000000000 --- a/rules/hardcodedIV.go +++ /dev/null @@ -1,136 +0,0 @@ -package rules - -import ( - "go/ast" - - "github.com/securego/gosec/v2" - "github.com/securego/gosec/v2/issue" -) - -type usesHardcodedIV struct { - issue.MetaData - trackedFunctions map[string][]int -} - -func (r *usesHardcodedIV) ID() string { - return r.MetaData.ID -} - -// The code is a little bit spaghetti and there are things that repeat -// Can be improved -func (r *usesHardcodedIV) Match(n ast.Node, c *gosec.Context) (*issue.Issue, error) { - // cast n to a call expression, we can do that safely, because this match method gets only called when CallExpr node is found - funcCall := n.(*ast.CallExpr) - - // cast to a function call from an object and get the function part; example: a.doSomething() - - if funcSelector, isSelector := funcCall.Fun.(*ast.SelectorExpr); isSelector { - - // Check if the call is actually made from an object - if _, hasX := funcSelector.X.(*ast.Ident); hasX { - - //Iterate trough the wanted functions - for functionName, functionNumArgsAndNoncePosArr := range r.trackedFunctions { - - // Check if the function name matches with the one we look for, and if the function accepts an exact number of arguments(rough function signature check) - if funcSelector.Sel.Name == functionName && len(funcCall.Args) == functionNumArgsAndNoncePosArr[0] { - - // Check the type of the passed argument to the function - switch trackedFunctionPassedArgType := funcCall.Args[functionNumArgsAndNoncePosArr[1]].(type) { - - // {} used - case *ast.CompositeLit: - // Check if the argument is static array - if _, isArray := trackedFunctionPassedArgType.Type.(*ast.ArrayType); isArray { - return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array", r.Severity, r.Confidence), nil - } - - // () used - case *ast.CallExpr: - - // Check if it's a function call, because []byte() is a function call, and also check if the number of arguments to this call is only 1 - switch trackedFunctionPassedArgType.Fun.(type) { - case *ast.ArrayType: - return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array", r.Severity, r.Confidence), nil - - // Check if the argument passed is another function - case *ast.FuncLit: - functionCalled, _ := trackedFunctionPassedArgType.Fun.(*ast.FuncLit) - - // Check the type of the last statement in the anonymous function - switch calledFunctionLastInstructionType := functionCalled.Body.List[len(functionCalled.Body.List)-1].(type) { - - case *ast.IfStmt: - - ifStatementContent := calledFunctionLastInstructionType.Body.List - - // check if the if statement has return statement - if retStatement, isReturn := ifStatementContent[len(ifStatementContent)-1].(*ast.ReturnStmt); isReturn { - argInNestedFunc := retStatement.Results[0] - - // check the type of the returned value - switch argInNestedFunc.(type) { - case *ast.CompositeLit: - // Check if the argument is static array - if _, isArray := argInNestedFunc.(*ast.CompositeLit).Type.(*ast.ArrayType); isArray { - return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array in a function call", r.Severity, r.Confidence), nil - } - - case *ast.CallExpr: - if _, ok := argInNestedFunc.(*ast.CallExpr).Fun.(*ast.ArrayType); ok { - return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array in a function call", r.Severity, r.Confidence), nil - } - } - } - case *ast.ReturnStmt: - - argInNestedFunc := calledFunctionLastInstructionType.Results[0] - switch argInNestedFunc.(type) { - case *ast.CompositeLit: - // Check if the argument is static array - if _, isArray := argInNestedFunc.(*ast.CompositeLit).Type.(*ast.ArrayType); isArray { - return c.NewIssue(n, r.ID(), r.What+" by passing hardcoded byte array in a function call", r.Severity, r.Confidence), nil - } - - case *ast.CallExpr: - if _, ok := argInNestedFunc.(*ast.CallExpr).Fun.(*ast.ArrayType); ok { - return c.NewIssue(n, r.ID(), r.What+" by converting static string to a byte array in a function call", r.Severity, r.Confidence), nil - } - } - } - } - } - } - } - } - } - // loop through the functions we are checking - - return nil, nil -} - -func NewUsesHardCodedIV(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { - calls := make(map[string][]int) - // Holds the function name as key, the number of arguments that the function accepts, and at which index of those accepted arguments is the nonce/IV - // Example "Test" 3, 1 -- means the function "Test" which accepts 3 arguments, and has the nonce arg as second argument - - calls["Seal"] = []int{4, 1} - calls["Open"] = []int{4, 1} - calls["NewCBCDecrypter"] = []int{2, 1} // - calls["NewCBCEncrypter"] = []int{2, 1} // - calls["NewCFBDecrypter"] = []int{2, 1} - calls["NewCFBEncrypter"] = []int{2, 1} - calls["NewCTR"] = []int{2, 1} // - calls["NewOFB"] = []int{2, 1} // - - rule := &usesHardcodedIV{ - trackedFunctions: calls, - MetaData: issue.MetaData{ - ID: id, - Severity: issue.High, - Confidence: issue.Medium, - What: "Use of hardcoded IV/nonce for encryption", - }, - } - return rule, []ast.Node{(*ast.CallExpr)(nil)} -} diff --git a/rules/rulelist.go b/rules/rulelist.go index 889413cda7..13f29f71aa 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -100,7 +100,6 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { {"G404", "Insecure random number source (rand)", NewWeakRandCheck}, {"G405", "Detect the usage of DES or RC4", NewUsesWeakCryptographyEncryption}, {"G406", "Detect the usage of deprecated MD4 or RIPEMD160", NewUsesWeakDeprecatedCryptographyHash}, - {"G407", "Detect the usage of hardcoded Initialization Vector(IV)/Nonce", NewUsesHardCodedIV}, // blocklist {"G501", "Import blocklist: crypto/md5", NewBlocklistedImportMD5}, diff --git a/rules/rules_test.go b/rules/rules_test.go index 783ab878b3..036342aea4 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -247,6 +247,22 @@ var _ = Describe("gosec rules", func() { runner("G407", testutils.SampleCodeG407o) }) + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407p) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407q) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407r) + }) + + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407s) + }) + It("should detect blocklisted imports - MD5", func() { runner("G501", testutils.SampleCodeG501) }) diff --git a/testutils/g407_samples.go b/testutils/g407_samples.go index 675aa06752..6bd6c2ac8d 100644 --- a/testutils/g407_samples.go +++ b/testutils/g407_samples.go @@ -377,4 +377,98 @@ func main() { } `}, 2, gosec.NewConfig()}, } + + // SampleCodeG407p - Use of hardcoded nonce/IV + SampleCodeG407p = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + var nonce = []byte("ILoveMyNonce") + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + fmt.Println(string(aesGCM.Seal(nil, nonce, []byte("My secret message"), nil))) +} +`}, 1, gosec.NewConfig()}, + } + + // SampleCodeG407q - Use of hardcoded nonce/IV + SampleCodeG407q = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + var nonce = []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCTR := cipher.NewCTR(block, nonce) + var output = make([]byte, 16) + aesCTR.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) +} +`}, 1, gosec.NewConfig()}, + } + + // SampleCodeG407r - Use of hardcoded nonce/IV + SampleCodeG407r = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "crypto/rand" + "fmt" +) + +func coolFunc(size int) []byte{ + buf := make([]byte, size) + rand.Read(buf) + return buf +} + +func main() { + + var nonce = coolFunc(16) + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesCTR := cipher.NewCTR(block, nonce) + var output = make([]byte, 16) + aesCTR.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) +} +`}, 0, gosec.NewConfig()}, + } + // SampleCodeG407s - Use of hardcoded nonce/IV + SampleCodeG407s = []CodeSample{ + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "crypto/rand" + "fmt" +) + +var nonce = []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + cipherText := aesGCM.Seal(nil, nonce, []byte("My secret message"), nil) + fmt.Println(string(cipherText)) + +} +`}, 1, gosec.NewConfig()}, + } ) From 4566ab8530ea313ca6f86fd5ef6a1912eddd3643 Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Fri, 30 Aug 2024 11:39:24 +0200 Subject: [PATCH 04/13] Updated analyzer to use new way of initialization * Removed old way of initializing analyzers * Added the new analyzer to the rest of the default analyzers * Fixed small bug in the rule * Removed the test for the new analyzer from the file responsible for testing the rules * Merged the diffrent examples into 1 variable * Added tests for the analyzer * Removed code that was used for testing rules, but it was used to test the analyzer --- analyzer_test.go | 346 ++++-------------------------------- analyzers/analyzers_test.go | 4 + analyzers/analyzerslist.go | 1 + analyzers/hardcodedNonce.go | 4 +- rules/rules_test.go | 76 -------- testutils/g407_samples.go | 114 ++++++------ 6 files changed, 105 insertions(+), 440 deletions(-) diff --git a/analyzer_test.go b/analyzer_test.go index 301f194c40..2e5591b3f1 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -187,22 +187,6 @@ var _ = Describe("Analyzer", func() { Expect(controlIssues).Should(HaveLen(sample.Errors)) }) - It("should find errors when nosec is not in use", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - controlPackage := testutils.NewTestPackage() - defer controlPackage.Close() - controlPackage.AddFile("aesOFB.go", source) - err := controlPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, controlPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - controlIssues, _, _ := analyzer.Report() - Expect(controlIssues).Should(HaveLen(sample.Errors)) - }) - It("should report Go build errors and invalid files", func() { analyzer.LoadRules(rules.Generate(false).RulesInfo()) pkg := testutils.NewTestPackage() @@ -282,23 +266,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should not report errors when a nosec line comment is present", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should not report errors when a nosec block comment is present", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -350,23 +317,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should not report errors when a nosec block comment is present", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) /* #nosec */", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should not report errors when an exclude comment is present for the correct rule", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -421,24 +371,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should not report errors when an exclude comment is present for the correct rule", func() { - // Rule for hardcoded nonce/IV - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G407", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should not report errors when a nosec block and line comment are present", func() { sample := testutils.SampleCodeG101[23] source := sample.Code[0] @@ -536,23 +468,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) - It("should report errors when an exclude comment is present for a different rule", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G301", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(HaveLen(sample.Errors)) - }) - It("should not report errors when an exclude comment is present for multiple rules, including the correct rule", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -610,25 +525,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should not report errors when an exclude comment is present for multiple rules, including the correct rule", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G301 G407", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should pass the build tags", func() { sample := testutils.SampleCodeBuildTag[0] source := sample.Code[0] @@ -725,29 +621,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) - It("should be possible to overwrite nosec comments, and report issues", func() { - // Rule for hardcoded IV/nonce - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - - // overwrite nosec option - nosecIgnoreConfig := gosec.NewConfig() - nosecIgnoreConfig.SetGlobal(gosec.Nosec, "true") - customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) - customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = customAnalyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := customAnalyzer.Report() - Expect(nosecIssues).Should(HaveLen(sample.Errors)) - }) - It("should be possible to overwrite nosec comments, and report issues but they should not be counted", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -826,32 +699,6 @@ var _ = Describe("Analyzer", func() { Expect(metrics.NumNosec).Should(Equal(1)) }) - It("should be possible to overwrite nosec comments, and report issues but they should not be counted", func() { - // Rule for hardcoded nonce/IV - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - - // overwrite nosec option - nosecIgnoreConfig := gosec.NewConfig() - nosecIgnoreConfig.SetGlobal(gosec.Nosec, "mynosec") - nosecIgnoreConfig.SetGlobal(gosec.ShowIgnored, "true") - customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) - customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) // #mynosec", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = customAnalyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, metrics, _ := customAnalyzer.Report() - Expect(nosecIssues).Should(HaveLen(sample.Errors)) - Expect(metrics.NumFound).Should(Equal(0)) - Expect(metrics.NumNosec).Should(Equal(1)) - }) - It("should not report errors when nosec tag is in front of a line", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -903,23 +750,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should not report errors when nosec tag is in front of a line", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//Some description\n//#nosec G407\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should report errors when nosec tag is not in front of a line", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -971,23 +801,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) - It("should report errors when nosec tag is not in front of a line", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//Some description\n//Another description #nosec G407\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(HaveLen(sample.Errors)) - }) - It("should not report errors when rules are in front of nosec tag even rules are wrong", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -1039,23 +852,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should not report errors when rules are in front of nosec tag even rules are wrong", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//G301\n//#nosec\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should report errors when there are nosec tags after a #nosec WrongRuleList annotation", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -1107,23 +903,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(HaveLen(sample.Errors)) }) - It("should report errors when there are nosec tags after a #nosec WrongRuleList annotation", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "aesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "//#nosec\n//G301\n//#nosec\naesOFB := cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := analyzer.Report() - Expect(nosecIssues).Should(HaveLen(sample.Errors)) - }) - It("should be possible to use an alternative nosec tag", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -1193,29 +972,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should be possible to use an alternative nosec tag", func() { - // Rule for hardcoded nonce/IV - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - - // overwrite nosec option - nosecIgnoreConfig := gosec.NewConfig() - nosecIgnoreConfig.SetGlobal(gosec.NoSecAlternative, "falsePositive") - customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) - customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) // #falsePositive", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = customAnalyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := customAnalyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should ignore vulnerabilities when the default tag is found", func() { // Rule for MD5 weak crypto usage sample := testutils.SampleCodeG401[0] @@ -1285,29 +1041,6 @@ var _ = Describe("Analyzer", func() { Expect(nosecIssues).Should(BeEmpty()) }) - It("should ignore vulnerabilities when the default tag is found", func() { - // Rule for hardcoded nonce/IV - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - - // overwrite nosec option - nosecIgnoreConfig := gosec.NewConfig() - nosecIgnoreConfig.SetGlobal(gosec.NoSecAlternative, "falsePositive") - customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, false, false, 1, logger) - customAnalyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = customAnalyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - nosecIssues, _, _ := customAnalyzer.Report() - Expect(nosecIssues).Should(BeEmpty()) - }) - It("should be able to analyze Go test package", func() { customAnalyzer := gosec.NewAnalyzer(nil, true, false, false, 1, logger) customAnalyzer.LoadRules(rules.Generate(false).RulesInfo()) @@ -1671,26 +1404,6 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("Justification")) }) - It("should not report an error if the violation is suppressed", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec G407 -- Justification", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - issues, _, _ := analyzer.Report() - Expect(issues).To(HaveLen(sample.Errors)) - Expect(issues[0].Suppressions).To(HaveLen(1)) - Expect(issues[0].Suppressions[0].Kind).To(Equal("inSource")) - Expect(issues[0].Suppressions[0].Justification).To(Equal("Justification")) - }) - It("should not report an error if the violation is suppressed without certain rules", func() { sample := testutils.SampleCodeG401[0] source := sample.Code[0] @@ -1751,26 +1464,6 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("")) }) - It("should not report an error if the violation is suppressed without certain rules", func() { - sample := testutils.SampleCodeG407[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G407")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\"))", "cipher.NewOFB(block, []byte(\"ILoveMyNonceAlot\")) //#nosec", 1) - nosecPackage.AddFile("aesOFB.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - issues, _, _ := analyzer.Report() - Expect(issues).To(HaveLen(sample.Errors)) - Expect(issues[0].Suppressions).To(HaveLen(1)) - Expect(issues[0].Suppressions[0].Kind).To(Equal("inSource")) - Expect(issues[0].Suppressions[0].Justification).To(Equal("")) - }) - It("should not report an error if the rule is not included", func() { sample := testutils.SampleCodeG101[0] source := sample.Code[0] @@ -1809,6 +1502,45 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) }) + It("should not report an error if the analyzer is not included", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadAnalyzers(analyzers.Generate(true, analyzers.NewAnalyzerFilter(false, "G115")).AnalyzersInfo()) + + controlPackage := testutils.NewTestPackage() + defer controlPackage.Close() + controlPackage.AddFile("cipher.go", source) + err := controlPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, controlPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + controlIssues, _, _ := analyzer.Report() + Expect(controlIssues).Should(HaveLen(sample.Errors)) + Expect(controlIssues[0].Suppressions).To(HaveLen(1)) + Expect(controlIssues[0].Suppressions[0].Kind).To(Equal("external")) + Expect(controlIssues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) + }) + + + It("should not report an error if the analyzer is excluded", func() { + sample := testutils.SampleCodeG407[0] + source := sample.Code[0] + analyzer.LoadAnalyzers(analyzers.Generate(true, analyzers.NewAnalyzerFilter(true, "G407")).AnalyzersInfo()) + + controlPackage := testutils.NewTestPackage() + defer controlPackage.Close() + controlPackage.AddFile("cipher.go", source) + err := controlPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, controlPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + issues, _, _ := analyzer.Report() + Expect(issues).Should(HaveLen(sample.Errors)) + Expect(issues[0].Suppressions).To(HaveLen(1)) + Expect(issues[0].Suppressions[0].Kind).To(Equal("external")) + Expect(issues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) + }) + It("should not report an error if the analyzer is not included", func() { sample := testutils.SampleCodeG602[0] source := sample.Code[0] diff --git a/analyzers/analyzers_test.go b/analyzers/analyzers_test.go index 898040b8b9..a77445ae6c 100644 --- a/analyzers/analyzers_test.go +++ b/analyzers/analyzers_test.go @@ -55,6 +55,10 @@ var _ = Describe("gosec analyzers", func() { runner("G115", testutils.SampleCodeG115) }) + It("should detect hardcoded nonce/IV", func() { + runner("G407", testutils.SampleCodeG407) + }) + It("should detect out of bounds slice access", func() { runner("G602", testutils.SampleCodeG602) }) diff --git a/analyzers/analyzerslist.go b/analyzers/analyzerslist.go index 4bf9ca9b18..f2157442f5 100644 --- a/analyzers/analyzerslist.go +++ b/analyzers/analyzerslist.go @@ -68,6 +68,7 @@ func NewAnalyzerFilter(action bool, analyzerIDs ...string) AnalyzerFilter { var defaultAnalyzers = []AnalyzerDefinition{ {"G115", "Type conversion which leads to integer overflow", newConversionOverflowAnalyzer}, {"G602", "Possible slice bounds out of range", newSliceBoundsAnalyzer}, + {"G407", "Use of hardcoded IV/nonce for encryption", newHardCodedNonce}, } // Generate the list of analyzers to use diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index 2d1d343542..5598c33d26 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -98,11 +98,13 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. // It goes and check if this function contains call to crypto/rand.Read in it's body(Assuming that calling crypto/rand.Read in a function, is used for the generation of nonce/iv ) case *ssa.Call: if calledFunction, ok := valType.Call.Value.(*ssa.Function); ok { - if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr != nil { + if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr == nil { issueDescription += " by passing a value from function which doesn't use crypto/rand" tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.Medium) gosecIssue = append(gosecIssue, tmp...) err = hasErr + } else if funcErr != nil { + err = funcErr } } diff --git a/rules/rules_test.go b/rules/rules_test.go index 036342aea4..9a7d65a2f1 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -187,82 +187,6 @@ var _ = Describe("gosec rules", func() { runner("G406", testutils.SampleCodeG406b) }) - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407b) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407c) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407d) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407e) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407f) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407g) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407h) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407i) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407j) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407k) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407l) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407m) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407n) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407o) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407p) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407q) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407r) - }) - - It("should detect hardcoded nonce/IV", func() { - runner("G407", testutils.SampleCodeG407s) - }) - It("should detect blocklisted imports - MD5", func() { runner("G501", testutils.SampleCodeG501) }) diff --git a/testutils/g407_samples.go b/testutils/g407_samples.go index 6bd6c2ac8d..3d1b153466 100644 --- a/testutils/g407_samples.go +++ b/testutils/g407_samples.go @@ -5,6 +5,7 @@ import "github.com/securego/gosec/v2" var ( // SampleCodeG407 - Use of hardcoded nonce/IV SampleCodeG407 = []CodeSample{ + {[]string{`package main import ( @@ -21,11 +22,63 @@ func main() { aesOFB.XORKeyStream(output, []byte("Very Cool thing!")) fmt.Println(string(output)) -}`}, 1, gosec.NewConfig()}, - } +} +`}, 1, gosec.NewConfig()}, + + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesOFB := cipher.NewOFB(block, []byte("ILoveMyNonceAlot")) // #nosec G407 + var output = make([]byte, 16) + aesOFB.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) + +} + +`}, 0, gosec.NewConfig()}, + + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + +func main() { + + block, _ := aes.NewCipher( []byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesGCM, _ := cipher.NewGCM(block) + + cipherText := aesGCM.Seal(nil, func() []byte { + if true { + return []byte("ILoveMyNonce") + } else { + return []byte("IDont'Love..") + } + }(), []byte("My secret message"), nil) // #nosec G407 + fmt.Println(string(cipherText)) + + cipherText, _ = aesGCM.Open(nil, func() []byte { + if true { + return []byte("ILoveMyNonce") + } else { + return []byte("IDont'Love..") + } + }(), cipherText, nil) // #nosec G407 + + fmt.Println(string(cipherText)) +} +`}, 0, gosec.NewConfig()}, - // SampleCodeG407b - Use of hardcoded nonce/IV - SampleCodeG407b = []CodeSample{ {[]string{`package main import ( @@ -43,10 +96,7 @@ func main() { fmt.Println(string(output)) }`}, 1, gosec.NewConfig()}, - } - // SampleCodeG407c - Use of hardcoded nonce/IV - SampleCodeG407c = []CodeSample{ {[]string{`package main import ( @@ -64,10 +114,7 @@ func main() { fmt.Println(string(output)) }`}, 1, gosec.NewConfig()}, - } - // SampleCodeG407d - Use of hardcoded nonce/IV - SampleCodeG407d = []CodeSample{ {[]string{`package main import ( @@ -86,10 +133,7 @@ func main() { } `}, 1, gosec.NewConfig()}, - } - // SampleCodeG407e - Use of hardcoded nonce/IV - SampleCodeG407e = []CodeSample{ {[]string{`package main import ( @@ -109,10 +153,7 @@ func main() { fmt.Println(string(cipherText)) } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407f - Use of hardcoded nonce/IV - SampleCodeG407f = []CodeSample{ {[]string{`package main import ( @@ -132,10 +173,7 @@ func main() { fmt.Println(string(cipherText)) } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407g - Use of hardcoded nonce/IV - SampleCodeG407g = []CodeSample{ {[]string{`package main import ( @@ -156,10 +194,7 @@ func main() { fmt.Println(string(cipherText)) } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407h - Use of hardcoded nonce/IV - SampleCodeG407h = []CodeSample{ {[]string{`package main import ( @@ -193,10 +228,7 @@ func main() { fmt.Println(string(cipherText)) } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407I - Use of hardcoded nonce/IV - SampleCodeG407i = []CodeSample{ {[]string{`package main import ( @@ -229,10 +261,7 @@ func main() { fmt.Println(string(cipherText)) } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407j - Use of hardcoded nonce/IV - SampleCodeG407j = []CodeSample{ {[]string{`package main import ( @@ -252,10 +281,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407k - Use of hardcoded nonce/IV - SampleCodeG407k = []CodeSample{ {[]string{`package main import ( @@ -275,10 +301,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407l - Use of hardcoded nonce/IV - SampleCodeG407l = []CodeSample{ {[]string{`package main import ( @@ -299,10 +322,7 @@ func main() { fmt.Println(string(output)) }`}, 2, gosec.NewConfig()}, - } - // SampleCodeG407m - Use of hardcoded nonce/IV - SampleCodeG407m = []CodeSample{ {[]string{`package main import ( @@ -323,10 +343,7 @@ func main() { fmt.Println(string(output)) }`}, 2, gosec.NewConfig()}, - } - // SampleCodeG407n - Use of hardcoded nonce/IV - SampleCodeG407n = []CodeSample{ {[]string{`package main import ( @@ -349,10 +366,7 @@ func main() { fmt.Println(string(output)) }`}, 2, gosec.NewConfig()}, - } - // SampleCodeG407o - Use of hardcoded nonce/IV - SampleCodeG407o = []CodeSample{ {[]string{`package main import ( @@ -376,10 +390,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - } - // SampleCodeG407p - Use of hardcoded nonce/IV - SampleCodeG407p = []CodeSample{ {[]string{`package main import ( @@ -396,10 +407,7 @@ func main() { fmt.Println(string(aesGCM.Seal(nil, nonce, []byte("My secret message"), nil))) } `}, 1, gosec.NewConfig()}, - } - // SampleCodeG407q - Use of hardcoded nonce/IV - SampleCodeG407q = []CodeSample{ {[]string{`package main import ( @@ -418,10 +426,7 @@ func main() { fmt.Println(string(output)) } `}, 1, gosec.NewConfig()}, - } - // SampleCodeG407r - Use of hardcoded nonce/IV - SampleCodeG407r = []CodeSample{ {[]string{`package main import ( @@ -447,15 +452,12 @@ func main() { fmt.Println(string(output)) } `}, 0, gosec.NewConfig()}, - } - // SampleCodeG407s - Use of hardcoded nonce/IV - SampleCodeG407s = []CodeSample{ + {[]string{`package main import ( "crypto/aes" "crypto/cipher" - "crypto/rand" "fmt" ) From 5ed176ea1762e45ea613f7a27a9b7aac93818194 Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Fri, 30 Aug 2024 11:50:08 +0200 Subject: [PATCH 05/13] Formatting problems(CI was not passing) --- analyzer_test.go | 1 - analyzers/hardcodedNonce.go | 6 ++--- testutils/g407_samples.go | 51 +++++++++++++++++-------------------- 3 files changed, 26 insertions(+), 32 deletions(-) diff --git a/analyzer_test.go b/analyzer_test.go index 2e5591b3f1..f70b5fc475 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -1521,7 +1521,6 @@ var _ = Describe("Analyzer", func() { Expect(controlIssues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) }) - It("should not report an error if the analyzer is excluded", func() { sample := testutils.SampleCodeG407[0] source := sample.Code[0] diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index 5598c33d26..875918c0a3 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -57,8 +57,8 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { "crypto/cipher.NewOFB": {2, 1}, } var issues []*issue.Issue - var ssaPkgFunctions = ssaResult.SSA.SrcFuncs - var savedArgsFromFunctions = *iterateAndGetArgsFromTrackedFunctions(ssaPkgFunctions, &calls) + ssaPkgFunctions := ssaResult.SSA.SrcFuncs + savedArgsFromFunctions := *iterateAndGetArgsFromTrackedFunctions(ssaPkgFunctions, &calls) for _, savedArg := range savedArgsFromFunctions { tmp, err := raiseIssue(savedArg, &calls, ssaPkgFunctions, pass, "") @@ -148,14 +148,12 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. // Iterate through all places that use the `variable` argument and check if it's used in one of the tracked functions func iterateThroughReferrers(variable *ssa.Value, funcsToTrack *map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueSeverity issue.Score, issueConfidence issue.Score) ([]*issue.Issue, error) { - if funcsToTrack == nil || variable == nil || analyzerID == "" || issueDescription == "" || fileSet == nil { return nil, errors.New("received a nil object") } var gosecIssues []*issue.Issue = nil // Go trough all functions that use the given arg variable for _, referrer := range *(*variable).Referrers() { - // Iterate trough the functions we are interested for trackedFunc := range *funcsToTrack { diff --git a/testutils/g407_samples.go b/testutils/g407_samples.go index 3d1b153466..85054bbd53 100644 --- a/testutils/g407_samples.go +++ b/testutils/g407_samples.go @@ -2,11 +2,9 @@ package testutils import "github.com/securego/gosec/v2" -var ( - // SampleCodeG407 - Use of hardcoded nonce/IV - SampleCodeG407 = []CodeSample{ - - {[]string{`package main +// SampleCodeG407 - Use of hardcoded nonce/IV +var SampleCodeG407 = []CodeSample{ + {[]string{`package main import ( "crypto/aes" @@ -25,7 +23,7 @@ func main() { } `}, 1, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -45,7 +43,7 @@ func main() { `}, 0, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -79,7 +77,7 @@ func main() { } `}, 0, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -97,7 +95,7 @@ func main() { }`}, 1, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -115,7 +113,7 @@ func main() { }`}, 1, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -134,7 +132,7 @@ func main() { } `}, 1, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -154,7 +152,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -174,7 +172,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -195,7 +193,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -229,7 +227,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -262,7 +260,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -282,7 +280,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -302,7 +300,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -323,7 +321,7 @@ func main() { }`}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -344,7 +342,7 @@ func main() { }`}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -367,7 +365,7 @@ func main() { }`}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -391,7 +389,7 @@ func main() { } `}, 2, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -408,7 +406,7 @@ func main() { } `}, 1, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -427,7 +425,7 @@ func main() { } `}, 1, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -453,7 +451,7 @@ func main() { } `}, 0, gosec.NewConfig()}, - {[]string{`package main + {[]string{`package main import ( "crypto/aes" @@ -472,5 +470,4 @@ func main() { } `}, 1, gosec.NewConfig()}, - } -) +} From b0b104cf8496da5a72f39cc708dcaf40230810e6 Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Fri, 30 Aug 2024 11:54:08 +0200 Subject: [PATCH 06/13] Removed function parameter which is always the same --- analyzers/hardcodedNonce.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index 875918c0a3..4fd19e34b9 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -81,7 +81,7 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. switch valType := (*val).(type) { case *ssa.Slice: issueDescription += " by passing hardcoded slice/array" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.High) + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) gosecIssue = append(gosecIssue, tmp...) err = hasErr @@ -89,7 +89,7 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. // Check if it's a dereference operation (a.k.a pointer) if valType.Op == token.MUL { issueDescription += " by passing pointer which points to hardcoded variable" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.Low) + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Low) gosecIssue = append(gosecIssue, tmp...) err = hasErr } @@ -100,7 +100,7 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. if calledFunction, ok := valType.Call.Value.(*ssa.Function); ok { if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr == nil { issueDescription += " by passing a value from function which doesn't use crypto/rand" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.Medium) + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Medium) gosecIssue = append(gosecIssue, tmp...) err = hasErr } else if funcErr != nil { @@ -113,7 +113,7 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. case *ssa.Convert: if valType.Type().String() == "[]byte" && valType.X.Type().String() == "string" { issueDescription += " by passing converted string" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High, issue.High) + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) gosecIssue = append(gosecIssue, tmp...) err = hasErr } @@ -147,7 +147,7 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. } // Iterate through all places that use the `variable` argument and check if it's used in one of the tracked functions -func iterateThroughReferrers(variable *ssa.Value, funcsToTrack *map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueSeverity issue.Score, issueConfidence issue.Score) ([]*issue.Issue, error) { +func iterateThroughReferrers(variable *ssa.Value, funcsToTrack *map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueConfidence issue.Score) ([]*issue.Issue, error) { if funcsToTrack == nil || variable == nil || analyzerID == "" || issueDescription == "" || fileSet == nil { return nil, errors.New("received a nil object") } @@ -162,7 +162,7 @@ func iterateThroughReferrers(variable *ssa.Value, funcsToTrack *map[string][]int trackedFuncParts := strings.Split(trackedFunc, ".") trackedFuncPartsName := trackedFuncParts[len(trackedFuncParts)-1] if strings.Contains(referrer.String(), trackedFuncPartsName) { - gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, referrer.Pos(), issueSeverity, issueConfidence)) + gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, referrer.Pos(), issue.High, issueConfidence)) } } } From aa0969472f3e590d979819923ee1f29311f9568f Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Fri, 30 Aug 2024 12:08:26 +0200 Subject: [PATCH 07/13] Added another test case in order to increase code coverage --- testutils/g407_samples.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/testutils/g407_samples.go b/testutils/g407_samples.go index 85054bbd53..461fa7cacf 100644 --- a/testutils/g407_samples.go +++ b/testutils/g407_samples.go @@ -31,6 +31,29 @@ import ( "fmt" ) +func encrypt(nonce []byte) { + block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) + aesOFB := cipher.NewOFB(block, nonce) + var output = make([]byte, 16) + aesOFB.XORKeyStream(output, []byte("Very Cool thing!")) + fmt.Println(string(output)) +} + +func main() { + + var nonce = []byte("ILoveMyNonceAlot") + encrypt(nonce) +} +`}, 1, gosec.NewConfig()}, + + {[]string{`package main + +import ( + "crypto/aes" + "crypto/cipher" + "fmt" +) + func main() { block, _ := aes.NewCipher([]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}) From ae01dcd59d3f51a8f1f1ec996738959bc3358f8e Mon Sep 17 00:00:00 2001 From: Dimitar Banchev Date: Fri, 30 Aug 2024 14:23:20 +0200 Subject: [PATCH 08/13] Added suggested changes --- analyzers/hardcodedNonce.go | 148 +++++++++++++++++++++--------------- 1 file changed, 85 insertions(+), 63 deletions(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index 4fd19e34b9..c6e6f03016 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -27,7 +27,7 @@ import ( "github.com/securego/gosec/v2/issue" ) -const defaultWhat = "Use of hardcoded IV/nonce for encryption" +const defaultIssueDescription = "Use of hardcoded IV/nonce for encryption" func newHardCodedNonce(id string, description string) *analysis.Analyzer { return &analysis.Analyzer{ @@ -58,24 +58,30 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { } var issues []*issue.Issue ssaPkgFunctions := ssaResult.SSA.SrcFuncs - savedArgsFromFunctions := *iterateAndGetArgsFromTrackedFunctions(ssaPkgFunctions, &calls) + savedArgsFromFunctions := iterateAndGetArgsFromTrackedFunctions(ssaPkgFunctions, calls) + if savedArgsFromFunctions == nil { + return nil, errors.New("no tracked functions found, resulting in no variables to track") + } for _, savedArg := range savedArgsFromFunctions { - tmp, err := raiseIssue(savedArg, &calls, ssaPkgFunctions, pass, "") - issues = append(issues, tmp...) + tmp, err := raiseIssue(savedArg, calls, ssaPkgFunctions, pass, "") if err != nil { - return nil, err + return issues, fmt.Errorf("raising issue error: %w", err) } + issues = append(issues, tmp...) } return issues, nil } -func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa.Function, pass *analysis.Pass, issueDescription string) ([]*issue.Issue, error) { - var err error = nil - var gosecIssue []*issue.Issue = nil +func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Function, pass *analysis.Pass, issueDescription string) ([]*issue.Issue, error) { + if *val == nil { + return nil, errors.New("received a pointer to a ssa.Value") + } + var err error + var gosecIssue []*issue.Issue if issueDescription == "" { - issueDescription = defaultWhat + issueDescription = defaultIssueDescription } switch valType := (*val).(type) { @@ -95,16 +101,20 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. } // When the value assigned to a variable is a function call. - // It goes and check if this function contains call to crypto/rand.Read in it's body(Assuming that calling crypto/rand.Read in a function, is used for the generation of nonce/iv ) + // It goes and check if this function contains call to crypto/rand.Read + // in it's body(Assuming that calling crypto/rand.Read in a function, + // is used for the generation of nonce/iv ) case *ssa.Call: - if calledFunction, ok := valType.Call.Value.(*ssa.Function); ok { - if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr == nil { - issueDescription += " by passing a value from function which doesn't use crypto/rand" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Medium) - gosecIssue = append(gosecIssue, tmp...) - err = hasErr - } else if funcErr != nil { - err = funcErr + if callValue := valType.Call.Value; callValue != nil { + if calledFunction, ok := callValue.(*ssa.Function); ok { + if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr == nil { + issueDescription += " by passing a value from function which doesn't use crypto/rand" + tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Medium) + gosecIssue = append(gosecIssue, tmp...) + err = hasErr + } else if funcErr != nil { + err = funcErr + } } } @@ -119,11 +129,12 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. } case *ssa.Parameter: - //arg given to tracked function is wrapped in another function, example: + // arg given to tracked function is wrapped in another function, example: // func encrypt(..,nonce,...){ - // aesgcm.Seal() - //} - // save parameter position, by checking the name of the variable used in tracked functions and comparing it with the name of the arg + // aesgcm.Seal(nonce) + // } + // save parameter position, by checking the name of the variable used in + // tracked functions and comparing it with the name of the arg if valType.Parent() != nil { trackedFunctions := make(map[string][]int) for index, funcArgs := range valType.Parent().Params { @@ -131,13 +142,12 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. trackedFunctions[valType.Parent().String()] = []int{len(valType.Parent().Params), index} } } - result := iterateAndGetArgsFromTrackedFunctions(ssaFuncs, &trackedFunctions) + result := iterateAndGetArgsFromTrackedFunctions(ssaFuncs, trackedFunctions) issueDescription += " by passing a parameter to a function and" // recursively backtrack to where the origin of a variable passed to multiple functions is - for _, trackedVariable := range *result { - - tmp, hasErr := raiseIssue(trackedVariable, &trackedFunctions, ssaFuncs, pass, issueDescription) + for _, trackedVariable := range result { + tmp, hasErr := raiseIssue(trackedVariable, trackedFunctions, ssaFuncs, pass, issueDescription) gosecIssue = append(gosecIssue, tmp...) err = hasErr } @@ -147,22 +157,27 @@ func raiseIssue(val *ssa.Value, funcsToTrack *map[string][]int, ssaFuncs []*ssa. } // Iterate through all places that use the `variable` argument and check if it's used in one of the tracked functions -func iterateThroughReferrers(variable *ssa.Value, funcsToTrack *map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueConfidence issue.Score) ([]*issue.Issue, error) { +func iterateThroughReferrers(variable *ssa.Value, funcsToTrack map[string][]int, + analyzerID string, issueDescription string, + fileSet *token.FileSet, issueConfidence issue.Score, +) ([]*issue.Issue, error) { if funcsToTrack == nil || variable == nil || analyzerID == "" || issueDescription == "" || fileSet == nil { return nil, errors.New("received a nil object") } - var gosecIssues []*issue.Issue = nil + var gosecIssues []*issue.Issue // Go trough all functions that use the given arg variable - for _, referrer := range *(*variable).Referrers() { - // Iterate trough the functions we are interested - for trackedFunc := range *funcsToTrack { - - // Split the functions we are interested in, by the '.' because we will use the function name to do the comparison - // MIGHT GIVE SOME FALSE POSITIVES THIS WAY - trackedFuncParts := strings.Split(trackedFunc, ".") - trackedFuncPartsName := trackedFuncParts[len(trackedFuncParts)-1] - if strings.Contains(referrer.String(), trackedFuncPartsName) { - gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, referrer.Pos(), issue.High, issueConfidence)) + if varReferrers := (*variable).Referrers(); *varReferrers != nil { + for _, referrer := range *varReferrers { + // Iterate trough the functions we are interested + for trackedFunc := range funcsToTrack { + + // Split the functions we are interested in, by the '.' because we will use the function name to do the comparison + // MIGHT GIVE SOME FALSE POSITIVES THIS WAY + trackedFuncParts := strings.Split(trackedFunc, ".") + trackedFuncPartsName := trackedFuncParts[len(trackedFuncParts)-1] + if strings.Contains(referrer.String(), trackedFuncPartsName) { + gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, referrer.Pos(), issue.High, issueConfidence)) + } } } } @@ -188,44 +203,51 @@ func isFuncContainsCryptoRand(funcCall *ssa.Function) (bool, error) { return false, nil } -func addToVarsMap(value *ssa.Value, mapToAddTo *map[string]*ssa.Value) { - key := (*value).Name() + (*value).Type().String() + (*value).String() + (*value).Parent().String() - (*mapToAddTo)[key] = value +func addToVarsMap(value *ssa.Value, mapToAddTo map[string]*ssa.Value) { + valDerefed := *value + key := valDerefed.Name() + valDerefed.Type().String() + valDerefed.String() + valDerefed.Parent().String() + mapToAddTo[key] = value } -func isContainedInMap(value *ssa.Value, mapToCheck *map[string]*ssa.Value) bool { - key := (*value).Name() + (*value).Type().String() + (*value).String() + (*value).Parent().String() - _, contained := (*mapToCheck)[key] +func isContainedInMap(value *ssa.Value, mapToCheck map[string]*ssa.Value) bool { + valDerefed := *value + + key := valDerefed.Name() + valDerefed.Type().String() + valDerefed.String() + valDerefed.Parent().String() + _, contained := mapToCheck[key] return contained } -func iterateAndGetArgsFromTrackedFunctions(ssaFuncs []*ssa.Function, trackedFunc *map[string][]int) *map[string]*ssa.Value { +func iterateAndGetArgsFromTrackedFunctions(ssaFuncs []*ssa.Function, trackedFunc map[string][]int) map[string]*ssa.Value { values := make(map[string]*ssa.Value) for _, pkgFunc := range ssaFuncs { for _, funcBlock := range pkgFunc.Blocks { for _, funcBlocInstr := range funcBlock.Instrs { - if funcCall, ok := funcBlocInstr.(*ssa.Call); ok { - for trackedFuncName, trackedFuncArgsInfo := range *trackedFunc { - // only process functions that have the same number of arguments as the ones we track - if len(funcCall.Call.Args) == trackedFuncArgsInfo[0] { - tmpArg := funcCall.Call.Args[trackedFuncArgsInfo[1]] - // check if the function is called from an object or directly from the package - if funcCall.Call.Method != nil { - if methodFullname := funcCall.Call.Method.FullName(); methodFullname == trackedFuncName { - if !isContainedInMap(&tmpArg, &values) { - addToVarsMap(&tmpArg, &values) - } - } - } else if funcCall.Call.Value.String() == trackedFuncName { - if !isContainedInMap(&tmpArg, &values) { - addToVarsMap(&tmpArg, &values) - } - } + iterateTrackedFunctionsAndAddArgs(&funcBlocInstr, trackedFunc, values) + } + } + } + return values +} + +func iterateTrackedFunctionsAndAddArgs(funcBlocInstr *ssa.Instruction, trackedFunc map[string][]int, values map[string]*ssa.Value) { + if funcCall, ok := (*funcBlocInstr).(*ssa.Call); ok { + for trackedFuncName, trackedFuncArgsInfo := range trackedFunc { + // only process functions that have the same number of arguments as the ones we track + if len(funcCall.Call.Args) == trackedFuncArgsInfo[0] { + tmpArg := funcCall.Call.Args[trackedFuncArgsInfo[1]] + // check if the function is called from an object or directly from the package + if funcCall.Call.Method != nil { + if methodFullname := funcCall.Call.Method.FullName(); methodFullname == trackedFuncName { + if !isContainedInMap(&tmpArg, values) { + addToVarsMap(&tmpArg, values) } } + } else if funcCall.Call.Value.String() == trackedFuncName { + if !isContainedInMap(&tmpArg, values) { + addToVarsMap(&tmpArg, values) + } } } } } - return &values } From a5381af118ae0096adf14e70418c3294c80500c5 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Fri, 30 Aug 2024 16:57:50 +0000 Subject: [PATCH 09/13] Pass the value argument directly since is an interface The value doens't require to be passed as a pointer since is a interface. Change-Id: Ia21bceb5f315f4c30bd28425d62f678e9203e93f Signed-off-by: Cosmin Cojocar --- analyzers/hardcodedNonce.go | 77 ++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index c6e6f03016..f70f622003 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -64,7 +64,10 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { } for _, savedArg := range savedArgsFromFunctions { - tmp, err := raiseIssue(savedArg, calls, ssaPkgFunctions, pass, "") + if savedArg == nil { + continue + } + tmp, err := raiseIssue(*savedArg, calls, ssaPkgFunctions, pass, "") if err != nil { return issues, fmt.Errorf("raising issue error: %w", err) } @@ -73,10 +76,8 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { return issues, nil } -func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Function, pass *analysis.Pass, issueDescription string) ([]*issue.Issue, error) { - if *val == nil { - return nil, errors.New("received a pointer to a ssa.Value") - } +func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Function, + pass *analysis.Pass, issueDescription string) ([]*issue.Issue, error) { var err error var gosecIssue []*issue.Issue @@ -84,13 +85,12 @@ func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.F issueDescription = defaultIssueDescription } - switch valType := (*val).(type) { + switch valType := (val).(type) { case *ssa.Slice: issueDescription += " by passing hardcoded slice/array" tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) gosecIssue = append(gosecIssue, tmp...) err = hasErr - case *ssa.UnOp: // Check if it's a dereference operation (a.k.a pointer) if valType.Op == token.MUL { @@ -99,7 +99,6 @@ func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.F gosecIssue = append(gosecIssue, tmp...) err = hasErr } - // When the value assigned to a variable is a function call. // It goes and check if this function contains call to crypto/rand.Read // in it's body(Assuming that calling crypto/rand.Read in a function, @@ -117,7 +116,6 @@ func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.F } } } - // only checks from strings->[]byte // might need to add additional types case *ssa.Convert: @@ -127,7 +125,6 @@ func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.F gosecIssue = append(gosecIssue, tmp...) err = hasErr } - case *ssa.Parameter: // arg given to tracked function is wrapped in another function, example: // func encrypt(..,nonce,...){ @@ -147,7 +144,10 @@ func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.F issueDescription += " by passing a parameter to a function and" // recursively backtrack to where the origin of a variable passed to multiple functions is for _, trackedVariable := range result { - tmp, hasErr := raiseIssue(trackedVariable, trackedFunctions, ssaFuncs, pass, issueDescription) + if trackedVariable == nil { + continue + } + tmp, hasErr := raiseIssue(*trackedVariable, trackedFunctions, ssaFuncs, pass, issueDescription) gosecIssue = append(gosecIssue, tmp...) err = hasErr } @@ -157,7 +157,7 @@ func raiseIssue(val *ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.F } // Iterate through all places that use the `variable` argument and check if it's used in one of the tracked functions -func iterateThroughReferrers(variable *ssa.Value, funcsToTrack map[string][]int, +func iterateThroughReferrers(variable ssa.Value, funcsToTrack map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueConfidence issue.Score, ) ([]*issue.Issue, error) { @@ -165,19 +165,21 @@ func iterateThroughReferrers(variable *ssa.Value, funcsToTrack map[string][]int, return nil, errors.New("received a nil object") } var gosecIssues []*issue.Issue + refs := variable.Referrers() + if refs == nil { + return gosecIssues, nil + } // Go trough all functions that use the given arg variable - if varReferrers := (*variable).Referrers(); *varReferrers != nil { - for _, referrer := range *varReferrers { - // Iterate trough the functions we are interested - for trackedFunc := range funcsToTrack { - - // Split the functions we are interested in, by the '.' because we will use the function name to do the comparison - // MIGHT GIVE SOME FALSE POSITIVES THIS WAY - trackedFuncParts := strings.Split(trackedFunc, ".") - trackedFuncPartsName := trackedFuncParts[len(trackedFuncParts)-1] - if strings.Contains(referrer.String(), trackedFuncPartsName) { - gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, referrer.Pos(), issue.High, issueConfidence)) - } + for _, ref := range *refs { + // Iterate trough the functions we are interested + for trackedFunc := range funcsToTrack { + + // Split the functions we are interested in, by the '.' because we will use the function name to do the comparison + // MIGHT GIVE SOME FALSE POSITIVES THIS WAY + trackedFuncParts := strings.Split(trackedFunc, ".") + trackedFuncPartsName := trackedFuncParts[len(trackedFuncParts)-1] + if strings.Contains(ref.String(), trackedFuncPartsName) { + gosecIssues = append(gosecIssues, newIssue(analyzerID, issueDescription, fileSet, ref.Pos(), issue.High, issueConfidence)) } } } @@ -203,16 +205,13 @@ func isFuncContainsCryptoRand(funcCall *ssa.Function) (bool, error) { return false, nil } -func addToVarsMap(value *ssa.Value, mapToAddTo map[string]*ssa.Value) { - valDerefed := *value - key := valDerefed.Name() + valDerefed.Type().String() + valDerefed.String() + valDerefed.Parent().String() - mapToAddTo[key] = value +func addToVarsMap(value ssa.Value, mapToAddTo map[string]*ssa.Value) { + key := value.Name() + value.Type().String() + value.String() + value.Parent().String() + mapToAddTo[key] = &value } -func isContainedInMap(value *ssa.Value, mapToCheck map[string]*ssa.Value) bool { - valDerefed := *value - - key := valDerefed.Name() + valDerefed.Type().String() + valDerefed.String() + valDerefed.Parent().String() +func isContainedInMap(value ssa.Value, mapToCheck map[string]*ssa.Value) bool { + key := value.Name() + value.Type().String() + value.String() + value.Parent().String() _, contained := mapToCheck[key] return contained } @@ -222,15 +221,15 @@ func iterateAndGetArgsFromTrackedFunctions(ssaFuncs []*ssa.Function, trackedFunc for _, pkgFunc := range ssaFuncs { for _, funcBlock := range pkgFunc.Blocks { for _, funcBlocInstr := range funcBlock.Instrs { - iterateTrackedFunctionsAndAddArgs(&funcBlocInstr, trackedFunc, values) + iterateTrackedFunctionsAndAddArgs(funcBlocInstr, trackedFunc, values) } } } return values } -func iterateTrackedFunctionsAndAddArgs(funcBlocInstr *ssa.Instruction, trackedFunc map[string][]int, values map[string]*ssa.Value) { - if funcCall, ok := (*funcBlocInstr).(*ssa.Call); ok { +func iterateTrackedFunctionsAndAddArgs(funcBlocInstr ssa.Instruction, trackedFunc map[string][]int, values map[string]*ssa.Value) { + if funcCall, ok := (funcBlocInstr).(*ssa.Call); ok { for trackedFuncName, trackedFuncArgsInfo := range trackedFunc { // only process functions that have the same number of arguments as the ones we track if len(funcCall.Call.Args) == trackedFuncArgsInfo[0] { @@ -238,13 +237,13 @@ func iterateTrackedFunctionsAndAddArgs(funcBlocInstr *ssa.Instruction, trackedFu // check if the function is called from an object or directly from the package if funcCall.Call.Method != nil { if methodFullname := funcCall.Call.Method.FullName(); methodFullname == trackedFuncName { - if !isContainedInMap(&tmpArg, values) { - addToVarsMap(&tmpArg, values) + if !isContainedInMap(tmpArg, values) { + addToVarsMap(tmpArg, values) } } } else if funcCall.Call.Value.String() == trackedFuncName { - if !isContainedInMap(&tmpArg, values) { - addToVarsMap(&tmpArg, values) + if !isContainedInMap(tmpArg, values) { + addToVarsMap(tmpArg, values) } } } From d9c15b950cb2070c964c4408c1d1a47ccbb3da6b Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Fri, 30 Aug 2024 17:06:36 +0000 Subject: [PATCH 10/13] Refactor to reduce some fuctions and variable names Change-Id: I7f42c1de4e39dceb8e8144037d5af9223331ff06 Signed-off-by: Cosmin Cojocar --- analyzers/hardcodedNonce.go | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index f70f622003..25a962da30 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -56,22 +56,21 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { "crypto/cipher.NewCTR": {2, 1}, "crypto/cipher.NewOFB": {2, 1}, } - var issues []*issue.Issue ssaPkgFunctions := ssaResult.SSA.SrcFuncs - savedArgsFromFunctions := iterateAndGetArgsFromTrackedFunctions(ssaPkgFunctions, calls) - if savedArgsFromFunctions == nil { + args := getArgsFromTrackedFunctions(ssaPkgFunctions, calls) + if args == nil { return nil, errors.New("no tracked functions found, resulting in no variables to track") } - - for _, savedArg := range savedArgsFromFunctions { - if savedArg == nil { + var issues []*issue.Issue + for _, arg := range args { + if arg == nil { continue } - tmp, err := raiseIssue(*savedArg, calls, ssaPkgFunctions, pass, "") + i, err := raiseIssue(*arg, calls, ssaPkgFunctions, pass, "") if err != nil { return issues, fmt.Errorf("raising issue error: %w", err) } - issues = append(issues, tmp...) + issues = append(issues, i...) } return issues, nil } @@ -139,7 +138,7 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu trackedFunctions[valType.Parent().String()] = []int{len(valType.Parent().Params), index} } } - result := iterateAndGetArgsFromTrackedFunctions(ssaFuncs, trackedFunctions) + result := getArgsFromTrackedFunctions(ssaFuncs, trackedFunctions) issueDescription += " by passing a parameter to a function and" // recursively backtrack to where the origin of a variable passed to multiple functions is @@ -156,7 +155,7 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu return gosecIssue, err } -// Iterate through all places that use the `variable` argument and check if it's used in one of the tracked functions +// iterateThroughReferrers iterates through all places that use the `variable` argument and check if it's used in one of the tracked functions. func iterateThroughReferrers(variable ssa.Value, funcsToTrack map[string][]int, analyzerID string, issueDescription string, fileSet *token.FileSet, issueConfidence issue.Score, @@ -186,7 +185,7 @@ func iterateThroughReferrers(variable ssa.Value, funcsToTrack map[string][]int, return gosecIssues, nil } -// Check whether a function contains a call to crypto/rand.Read in it's function body +// isFuncContainsCryptoRand checks whether a function contains a call to crypto/rand.Read in it's function body. func isFuncContainsCryptoRand(funcCall *ssa.Function) (bool, error) { if funcCall == nil { return false, errors.New("passed ssa.Function object is nil") @@ -216,7 +215,7 @@ func isContainedInMap(value ssa.Value, mapToCheck map[string]*ssa.Value) bool { return contained } -func iterateAndGetArgsFromTrackedFunctions(ssaFuncs []*ssa.Function, trackedFunc map[string][]int) map[string]*ssa.Value { +func getArgsFromTrackedFunctions(ssaFuncs []*ssa.Function, trackedFunc map[string][]int) map[string]*ssa.Value { values := make(map[string]*ssa.Value) for _, pkgFunc := range ssaFuncs { for _, funcBlock := range pkgFunc.Blocks { From cb3c8311ac1cd150596a2a381c36caf6307f95e0 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Fri, 30 Aug 2024 17:08:17 +0000 Subject: [PATCH 11/13] Fix formatting Change-Id: I49caeb75f1bd7ecdb9b4f99466d96ad81e2e95ac Signed-off-by: Cosmin Cojocar --- analyzers/hardcodedNonce.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index 25a962da30..f2f31b9296 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -76,7 +76,8 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { } func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Function, - pass *analysis.Pass, issueDescription string) ([]*issue.Issue, error) { + pass *analysis.Pass, issueDescription string, +) ([]*issue.Issue, error) { var err error var gosecIssue []*issue.Issue From 4c16ef5f1cafad60358dd5c26d8d00f955c66ac4 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Fri, 30 Aug 2024 17:17:14 +0000 Subject: [PATCH 12/13] Make variable names more explicity and reduce duplications Change-Id: Ifa141b70351136cfe7d0756a83e8166a24b5d538 Signed-off-by: Cosmin Cojocar --- analyzers/hardcodedNonce.go | 38 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index f2f31b9296..1eb24b5e1f 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -78,26 +78,23 @@ func runHardCodedNonce(pass *analysis.Pass) (interface{}, error) { func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Function, pass *analysis.Pass, issueDescription string, ) ([]*issue.Issue, error) { - var err error - var gosecIssue []*issue.Issue - if issueDescription == "" { issueDescription = defaultIssueDescription } - + var err error + var gosecIssue []*issue.Issue + var issues []*issue.Issue switch valType := (val).(type) { case *ssa.Slice: issueDescription += " by passing hardcoded slice/array" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) - gosecIssue = append(gosecIssue, tmp...) - err = hasErr + issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) + gosecIssue = append(gosecIssue, issues...) case *ssa.UnOp: // Check if it's a dereference operation (a.k.a pointer) if valType.Op == token.MUL { issueDescription += " by passing pointer which points to hardcoded variable" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Low) - gosecIssue = append(gosecIssue, tmp...) - err = hasErr + issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Low) + gosecIssue = append(gosecIssue, issues...) } // When the value assigned to a variable is a function call. // It goes and check if this function contains call to crypto/rand.Read @@ -108,9 +105,8 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu if calledFunction, ok := callValue.(*ssa.Function); ok { if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr == nil { issueDescription += " by passing a value from function which doesn't use crypto/rand" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Medium) - gosecIssue = append(gosecIssue, tmp...) - err = hasErr + issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Medium) + gosecIssue = append(gosecIssue, issues...) } else if funcErr != nil { err = funcErr } @@ -121,9 +117,8 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu case *ssa.Convert: if valType.Type().String() == "[]byte" && valType.X.Type().String() == "string" { issueDescription += " by passing converted string" - tmp, hasErr := iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) - gosecIssue = append(gosecIssue, tmp...) - err = hasErr + issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) + gosecIssue = append(gosecIssue, issues...) } case *ssa.Parameter: // arg given to tracked function is wrapped in another function, example: @@ -139,17 +134,16 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu trackedFunctions[valType.Parent().String()] = []int{len(valType.Parent().Params), index} } } - result := getArgsFromTrackedFunctions(ssaFuncs, trackedFunctions) + args := getArgsFromTrackedFunctions(ssaFuncs, trackedFunctions) issueDescription += " by passing a parameter to a function and" // recursively backtrack to where the origin of a variable passed to multiple functions is - for _, trackedVariable := range result { - if trackedVariable == nil { + for _, arg := range args { + if arg == nil { continue } - tmp, hasErr := raiseIssue(*trackedVariable, trackedFunctions, ssaFuncs, pass, issueDescription) - gosecIssue = append(gosecIssue, tmp...) - err = hasErr + issues, err = raiseIssue(*arg, trackedFunctions, ssaFuncs, pass, issueDescription) + gosecIssue = append(gosecIssue, issues...) } } } From d9eaa0570f2388573fc637275d1976f91cfc908d Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Fri, 30 Aug 2024 17:19:25 +0000 Subject: [PATCH 13/13] Make variable name more clear Change-Id: I5b863c0da6cc3d01efa527c60c93fdcbc8c5a53c Signed-off-by: Cosmin Cojocar --- analyzers/hardcodedNonce.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/analyzers/hardcodedNonce.go b/analyzers/hardcodedNonce.go index 1eb24b5e1f..b07363388d 100644 --- a/analyzers/hardcodedNonce.go +++ b/analyzers/hardcodedNonce.go @@ -82,19 +82,19 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu issueDescription = defaultIssueDescription } var err error - var gosecIssue []*issue.Issue + var allIssues []*issue.Issue var issues []*issue.Issue switch valType := (val).(type) { case *ssa.Slice: issueDescription += " by passing hardcoded slice/array" issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) - gosecIssue = append(gosecIssue, issues...) + allIssues = append(allIssues, issues...) case *ssa.UnOp: // Check if it's a dereference operation (a.k.a pointer) if valType.Op == token.MUL { issueDescription += " by passing pointer which points to hardcoded variable" issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Low) - gosecIssue = append(gosecIssue, issues...) + allIssues = append(allIssues, issues...) } // When the value assigned to a variable is a function call. // It goes and check if this function contains call to crypto/rand.Read @@ -106,7 +106,7 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu if contains, funcErr := isFuncContainsCryptoRand(calledFunction); !contains && funcErr == nil { issueDescription += " by passing a value from function which doesn't use crypto/rand" issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.Medium) - gosecIssue = append(gosecIssue, issues...) + allIssues = append(allIssues, issues...) } else if funcErr != nil { err = funcErr } @@ -118,7 +118,7 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu if valType.Type().String() == "[]byte" && valType.X.Type().String() == "string" { issueDescription += " by passing converted string" issues, err = iterateThroughReferrers(val, funcsToTrack, pass.Analyzer.Name, issueDescription, pass.Fset, issue.High) - gosecIssue = append(gosecIssue, issues...) + allIssues = append(allIssues, issues...) } case *ssa.Parameter: // arg given to tracked function is wrapped in another function, example: @@ -143,11 +143,11 @@ func raiseIssue(val ssa.Value, funcsToTrack map[string][]int, ssaFuncs []*ssa.Fu continue } issues, err = raiseIssue(*arg, trackedFunctions, ssaFuncs, pass, issueDescription) - gosecIssue = append(gosecIssue, issues...) + allIssues = append(allIssues, issues...) } } } - return gosecIssue, err + return allIssues, err } // iterateThroughReferrers iterates through all places that use the `variable` argument and check if it's used in one of the tracked functions.