From fd280360cd689f09803c15501157be5900ba10cd Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Mon, 9 Jan 2023 15:10:44 +0100 Subject: [PATCH] Fix the TLS config rule when parsing the settings from a variable (#911) --- cmd/tlsconfig/rule_template.go | 2 +- rules/tls.go | 68 ++++++++++++++++++++++++---------- rules/tls_config.go | 6 +-- testutils/source.go | 12 ++++++ 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/cmd/tlsconfig/rule_template.go b/cmd/tlsconfig/rule_template.go index 952be29..0c65560 100644 --- a/cmd/tlsconfig/rule_template.go +++ b/cmd/tlsconfig/rule_template.go @@ -15,6 +15,6 @@ func New{{.Name}}TLSCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {{range $cipherName := .Ciphers }} "{{$cipherName}}", {{end}} }, - }, []ast.Node{(*ast.CompositeLit)(nil)} + }, []ast.Node{(*ast.CompositeLit)(nil), (*ast.AssignStmt)(nil)} } `)) diff --git a/rules/tls.go b/rules/tls.go index 9e6e3e1..1cc3a29 100644 --- a/rules/tls.go +++ b/rules/tls.go @@ -63,31 +63,51 @@ func (t *insecureConfigTLS) processTLSCipherSuites(n ast.Node, c *gosec.Context) return nil } -func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Context) *gosec.Issue { - if ident, ok := n.Key.(*ast.Ident); ok { +func (t *insecureConfigTLS) processTLSConf(n ast.Node, c *gosec.Context) *gosec.Issue { + if kve, ok := n.(*ast.KeyValueExpr); ok { + issue := t.processTLSConfVal(kve.Key, kve.Value, c) + if issue != nil { + return issue + } + } else if assign, ok := n.(*ast.AssignStmt); ok { + if len(assign.Lhs) < 1 || len(assign.Rhs) < 1 { + return nil + } + if selector, ok := assign.Lhs[0].(*ast.SelectorExpr); ok { + issue := t.processTLSConfVal(selector.Sel, assign.Rhs[0], c) + if issue != nil { + return issue + } + } + } + return nil +} + +func (t *insecureConfigTLS) processTLSConfVal(key ast.Expr, value ast.Expr, c *gosec.Context) *gosec.Issue { + if ident, ok := key.(*ast.Ident); ok { switch ident.Name { case "InsecureSkipVerify": - if node, ok := n.Value.(*ast.Ident); ok { + if node, ok := value.(*ast.Ident); ok { if node.Name != "false" { - return gosec.NewIssue(c, n, t.ID(), "TLS InsecureSkipVerify set true.", gosec.High, gosec.High) + return gosec.NewIssue(c, value, t.ID(), "TLS InsecureSkipVerify set true.", gosec.High, gosec.High) } } else { // TODO(tk): symbol tab look up to get the actual value - return gosec.NewIssue(c, n, t.ID(), "TLS InsecureSkipVerify may be true.", gosec.High, gosec.Low) + return gosec.NewIssue(c, value, t.ID(), "TLS InsecureSkipVerify may be true.", gosec.High, gosec.Low) } case "PreferServerCipherSuites": - if node, ok := n.Value.(*ast.Ident); ok { + if node, ok := value.(*ast.Ident); ok { if node.Name == "false" { - return gosec.NewIssue(c, n, t.ID(), "TLS PreferServerCipherSuites set false.", gosec.Medium, gosec.High) + return gosec.NewIssue(c, value, t.ID(), "TLS PreferServerCipherSuites set false.", gosec.Medium, gosec.High) } } else { // TODO(tk): symbol tab look up to get the actual value - return gosec.NewIssue(c, n, t.ID(), "TLS PreferServerCipherSuites may be false.", gosec.Medium, gosec.Low) + return gosec.NewIssue(c, value, t.ID(), "TLS PreferServerCipherSuites may be false.", gosec.Medium, gosec.Low) } case "MinVersion": - if d, ok := n.Value.(*ast.Ident); ok { + if d, ok := value.(*ast.Ident); ok { obj := d.Obj if obj == nil { for _, f := range c.PkgFiles { @@ -118,10 +138,10 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Cont t.actualMinVersion = ival } } - } else if ival, ierr := gosec.GetInt(n.Value); ierr == nil { + } else if ival, ierr := gosec.GetInt(value); ierr == nil { t.actualMinVersion = ival } else { - if se, ok := n.Value.(*ast.SelectorExpr); ok { + if se, ok := value.(*ast.SelectorExpr); ok { if pkg, ok := se.X.(*ast.Ident); ok { if ip, ok := gosec.GetImportPath(pkg.Name, c); ok && ip == "crypto/tls" { t.actualMinVersion = t.mapVersion(se.Sel.Name) @@ -131,10 +151,10 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Cont } case "MaxVersion": - if ival, ierr := gosec.GetInt(n.Value); ierr == nil { + if ival, ierr := gosec.GetInt(value); ierr == nil { t.actualMaxVersion = ival } else { - if se, ok := n.Value.(*ast.SelectorExpr); ok { + if se, ok := value.(*ast.SelectorExpr); ok { if pkg, ok := se.X.(*ast.Ident); ok { if ip, ok := gosec.GetImportPath(pkg.Name, c); ok && ip == "crypto/tls" { t.actualMaxVersion = t.mapVersion(se.Sel.Name) @@ -144,7 +164,7 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Cont } case "CipherSuites": - if ret := t.processTLSCipherSuites(n.Value, c); ret != nil { + if ret := t.processTLSCipherSuites(value, c); ret != nil { return ret } @@ -192,17 +212,27 @@ func (t *insecureConfigTLS) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, e actualType := c.Info.TypeOf(complit.Type) if actualType != nil && actualType.String() == t.requiredType { for _, elt := range complit.Elts { - if kve, ok := elt.(*ast.KeyValueExpr); ok { - issue := t.processTLSConfVal(kve, c) - if issue != nil { - return issue, nil - } + issue := t.processTLSConf(elt, c) + if issue != nil { + return issue, nil } } issue := t.checkVersion(complit, c) t.resetVersion() return issue, nil } + } else { + if assign, ok := n.(*ast.AssignStmt); ok && len(assign.Lhs) > 0 { + if selector, ok := assign.Lhs[0].(*ast.SelectorExpr); ok { + actualType := c.Info.TypeOf(selector.X) + if actualType != nil && actualType.String() == t.requiredType { + issue := t.processTLSConf(assign, c) + if issue != nil { + return issue, nil + } + } + } + } } return nil, nil } diff --git a/rules/tls_config.go b/rules/tls_config.go index 5d68593..9bb17c2 100644 --- a/rules/tls_config.go +++ b/rules/tls_config.go @@ -19,7 +19,7 @@ func NewModernTLSCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256", }, - }, []ast.Node{(*ast.CompositeLit)(nil)} + }, []ast.Node{(*ast.CompositeLit)(nil), (*ast.AssignStmt)(nil)} } // NewIntermediateTLSCheck creates a check for Intermediate TLS ciphers @@ -45,7 +45,7 @@ func NewIntermediateTLSCheck(id string, conf gosec.Config) (gosec.Rule, []ast.No "TLS_DHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_DHE_RSA_WITH_AES_256_GCM_SHA384", }, - }, []ast.Node{(*ast.CompositeLit)(nil)} + }, []ast.Node{(*ast.CompositeLit)(nil), (*ast.AssignStmt)(nil)} } // NewOldTLSCheck creates a check for Old TLS ciphers @@ -88,5 +88,5 @@ func NewOldTLSCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { "TLS_RSA_WITH_AES_256_CBC_SHA", "TLS_RSA_WITH_3DES_EDE_CBC_SHA", }, - }, []ast.Node{(*ast.CompositeLit)(nil)} + }, []ast.Node{(*ast.CompositeLit)(nil), (*ast.AssignStmt)(nil)} } diff --git a/testutils/source.go b/testutils/source.go index fdd555d..0683bae 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -2859,6 +2859,18 @@ func main() { if err != nil { fmt.Println(err) } +}`}, 1, gosec.NewConfig()}, + {[]string{` +// InsecureSkipVerify from variable +package main + +import ( + "crypto/tls" +) + +func main() { + var conf tls.Config + conf.InsecureSkipVerify = true }`}, 1, gosec.NewConfig()}, {[]string{ `