Fix the TLS config rule when parsing the settings from a variable (#911)

This commit is contained in:
Cosmin Cojocar 2023-01-09 15:10:44 +01:00 committed by GitHub
parent a522ae6f5f
commit fd280360cd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 65 additions and 23 deletions

View file

@ -15,6 +15,6 @@ func New{{.Name}}TLSCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node)
{{range $cipherName := .Ciphers }} "{{$cipherName}}", {{range $cipherName := .Ciphers }} "{{$cipherName}}",
{{end}} {{end}}
}, },
}, []ast.Node{(*ast.CompositeLit)(nil)} }, []ast.Node{(*ast.CompositeLit)(nil), (*ast.AssignStmt)(nil)}
} }
`)) `))

View file

@ -63,31 +63,51 @@ func (t *insecureConfigTLS) processTLSCipherSuites(n ast.Node, c *gosec.Context)
return nil return nil
} }
func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Context) *gosec.Issue { func (t *insecureConfigTLS) processTLSConf(n ast.Node, c *gosec.Context) *gosec.Issue {
if ident, ok := n.Key.(*ast.Ident); ok { 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 { switch ident.Name {
case "InsecureSkipVerify": case "InsecureSkipVerify":
if node, ok := n.Value.(*ast.Ident); ok { if node, ok := value.(*ast.Ident); ok {
if node.Name != "false" { 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 { } else {
// TODO(tk): symbol tab look up to get the actual value // 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": case "PreferServerCipherSuites":
if node, ok := n.Value.(*ast.Ident); ok { if node, ok := value.(*ast.Ident); ok {
if node.Name == "false" { 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 { } else {
// TODO(tk): symbol tab look up to get the actual value // 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": case "MinVersion":
if d, ok := n.Value.(*ast.Ident); ok { if d, ok := value.(*ast.Ident); ok {
obj := d.Obj obj := d.Obj
if obj == nil { if obj == nil {
for _, f := range c.PkgFiles { for _, f := range c.PkgFiles {
@ -118,10 +138,10 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Cont
t.actualMinVersion = ival 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 t.actualMinVersion = ival
} else { } 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 pkg, ok := se.X.(*ast.Ident); ok {
if ip, ok := gosec.GetImportPath(pkg.Name, c); ok && ip == "crypto/tls" { if ip, ok := gosec.GetImportPath(pkg.Name, c); ok && ip == "crypto/tls" {
t.actualMinVersion = t.mapVersion(se.Sel.Name) t.actualMinVersion = t.mapVersion(se.Sel.Name)
@ -131,10 +151,10 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Cont
} }
case "MaxVersion": case "MaxVersion":
if ival, ierr := gosec.GetInt(n.Value); ierr == nil { if ival, ierr := gosec.GetInt(value); ierr == nil {
t.actualMaxVersion = ival t.actualMaxVersion = ival
} else { } 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 pkg, ok := se.X.(*ast.Ident); ok {
if ip, ok := gosec.GetImportPath(pkg.Name, c); ok && ip == "crypto/tls" { if ip, ok := gosec.GetImportPath(pkg.Name, c); ok && ip == "crypto/tls" {
t.actualMaxVersion = t.mapVersion(se.Sel.Name) t.actualMaxVersion = t.mapVersion(se.Sel.Name)
@ -144,7 +164,7 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gosec.Cont
} }
case "CipherSuites": case "CipherSuites":
if ret := t.processTLSCipherSuites(n.Value, c); ret != nil { if ret := t.processTLSCipherSuites(value, c); ret != nil {
return ret 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) actualType := c.Info.TypeOf(complit.Type)
if actualType != nil && actualType.String() == t.requiredType { if actualType != nil && actualType.String() == t.requiredType {
for _, elt := range complit.Elts { for _, elt := range complit.Elts {
if kve, ok := elt.(*ast.KeyValueExpr); ok { issue := t.processTLSConf(elt, c)
issue := t.processTLSConfVal(kve, c) if issue != nil {
if issue != nil { return issue, nil
return issue, nil
}
} }
} }
issue := t.checkVersion(complit, c) issue := t.checkVersion(complit, c)
t.resetVersion() t.resetVersion()
return issue, nil 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 return nil, nil
} }

View file

@ -19,7 +19,7 @@ func NewModernTLSCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
"TLS_AES_256_GCM_SHA384", "TLS_AES_256_GCM_SHA384",
"TLS_CHACHA20_POLY1305_SHA256", "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 // 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_128_GCM_SHA256",
"TLS_DHE_RSA_WITH_AES_256_GCM_SHA384", "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 // 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_AES_256_CBC_SHA",
"TLS_RSA_WITH_3DES_EDE_CBC_SHA", "TLS_RSA_WITH_3DES_EDE_CBC_SHA",
}, },
}, []ast.Node{(*ast.CompositeLit)(nil)} }, []ast.Node{(*ast.CompositeLit)(nil), (*ast.AssignStmt)(nil)}
} }

View file

@ -2859,6 +2859,18 @@ func main() {
if err != nil { if err != nil {
fmt.Println(err) 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()}, }`}, 1, gosec.NewConfig()},
{[]string{ {[]string{
` `