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

securego · Jan 9, 2023 · fd28036 · fd28036
1 parent a522ae6
commit fd28036
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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{
 			`