Improved config mgmt and fixed out of bounds panic (#5)

* Improved config mgmt and fixed out of bounds panic Changed the way the config is loaded to use defined structs instead of nested maps of interface{}. Now, the config is loaded into a struct during initialization and the values are referenced by the struct fields as appropriate. This refrains from reading and parsing the config file twice (once when loading the default sources and again when loading any analyzers. Additionally, this change adds a length check to fix the panic shown in issue #2. * add check for old config format
praetorian-inc · Aug 19, 2021 · 1f3eb7e · 1f3eb7e
1 parent 067e3fd
commit 1f3eb7e
Show file tree

Hide file tree

Showing 5 changed files with 179 additions and 203 deletions.
diff --git a/analyzers/generic.go b/analyzers/generic.go
@@ -15,13 +15,9 @@
 package analyzers
 
 import (
-	"io/ioutil"
-	"log"
-
 	"github.com/praetorian-inc/gokart/util"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/buildssa"
-	"gopkg.in/yaml.v3"
 )
 
 // Creates generic taint analyzer based on Sources and Sinks defined in analyzers.yaml file
@@ -66,53 +62,20 @@ func genericFunctionRun(pass *analysis.Pass, vulnPathFuncs map[string][]string,
 // LoadGenericAnalyzers creates generic taint anlalyzers from custom Sources and Sinks defined in analyzers.yaml
 // converts all variables to SSA form to construct a call graph and performs
 // recursive taint analysis to search for input sources of user-controllable data
+func LoadGenericAnalyzers() []*analysis.Analyzer {
+	var analyzers []*analysis.Analyzer
 
-func LoadGenericAnalyzers(yaml_path string) []*analysis.Analyzer {
-	yfile, err := ioutil.ReadFile(yaml_path)
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	data := make(map[interface{}]map[interface{}]map[interface{}]interface{})
-	err = yaml.Unmarshal(yfile, &data)
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	// Load analyzers from the interface
-	analyzers := []*analysis.Analyzer{}
-	m := data["analyzers"]
-	for analyzerName, analyzerDict := range m {
-		// Get the vulnerability message
-		message := ""
-		if analyzerDict["message"] != nil {
-			message = analyzerDict["message"].(string)
-		}
-
-		// Load the map of vulnerable functions
-		vulnCalls := make(map[string][]string)
-		yamlCallsMap := analyzerDict["vuln_calls"].(map[string]interface{})
-		for pkgName, packageVulnFuncs := range yamlCallsMap {
-			var newList []string
-			vulnCalls[pkgName] = newList
-			packageVulnFuncsList := packageVulnFuncs.([]interface{})
-			for _, val := range packageVulnFuncsList {
-				vulnCalls[pkgName] = append(vulnCalls[pkgName], val.(string))
-			}
-		}
-
-		// Wrap generic_function_run with a function that the analyze package can use
+	for analyzerName, analyzerDict := range util.ScanConfig.Analyzers {
 		analyzerFunc := func(pass *analysis.Pass) (interface{}, error) {
-			return genericFunctionRun(pass, vulnCalls, analyzerName.(string), message)
+			return genericFunctionRun(pass, analyzerDict.VulnCalls, analyzerName, analyzerDict.Message)
 		}
-
-		// Form the analyzer object and append to the analyzer list
-		analysisRun := new(analysis.Analyzer)
-		analysisRun.Name = "path_traversal"
-		analysisRun.Doc = analyzerDict["doc"].(string)
-		analysisRun.Run = analyzerFunc
-		analysisRun.Requires = []*analysis.Analyzer{buildssa.Analyzer}
-		analyzers = append(analyzers, analysisRun)
+		analysisRun := analysis.Analyzer{
+			Name:     analyzerName,
+			Doc:      analyzerDict.Doc,
+			Run:      analyzerFunc,
+			Requires: []*analysis.Analyzer{buildssa.Analyzer},
+		}
+		analyzers = append(analyzers, &analysisRun)
 	}
 
 	return analyzers

diff --git a/analyzers/scan.go b/analyzers/scan.go
@@ -58,7 +58,7 @@ func Scan(args []string) {
 		}
 
 		// Fix up the path to make sure it is pointed at a directory (even if given a file)
-		fileInfo, err := os.Stat(strings.TrimRight(target_path, "..."))
+		fileInfo, err := os.Stat(strings.TrimRight(target_path, "."))
 		if err != nil {
 			log.Fatal(err)
 		}
@@ -76,7 +76,7 @@ func Scan(args []string) {
 			}
 		}
 
-		err = os.Chdir(strings.TrimRight(target_path, "..."))
+		err = os.Chdir(strings.TrimRight(target_path, "."))
 		if err != nil {
 			log.Fatal(err)
 		}
@@ -86,7 +86,7 @@ func Scan(args []string) {
 		}
 	}
 
-	generic_analyzers := LoadGenericAnalyzers(util.Config.YMLPath)
+	generic_analyzers := LoadGenericAnalyzers()
 	Analyzers = append(Analyzers, generic_analyzers[:]...)
 
 	// Begin timer

diff --git a/util/analyzers.yml b/util/analyzers.yml
@@ -33,66 +33,66 @@
 #         - "Printf"
 
 
+
+# Each entry specifies a source that should be considered untrusted
+# If the package already exists in the sources section, add the variable/function/type underneath 
+# Each package can contain multiple vulnerable sources.
 sources:
-  # Each entry specifies a source that should be considered untrusted
-  # If the package already exists in the sources section, add the variable/function/type underneath 
-  # Each package can contain multiple vulnerable sources.
-  sources:
-    # Sources that are defined in Go documentation as a "variable" go here (note: these variables will have an SSA type of "Global").
-    variables:
-      "os":
-        - "Args"
-    # Sources that are defined in Go documentation as a "function" go here.
-    functions:
-      "flag":
-        - "Arg"
-        - "Args"
-      "os":
-        - "Environ"
-        - "File"
-        - "FileInfo"
-        - "FileMode"
-        - "Readdir"
-        - "Readdirnames"
-        - "OpenFile"
-      "crypto/tls":
-        - "LoadX509KeyPair"
-        - "X509KeyPair"
-      "os/user":
-        - "Lookup"
-        - "LookupId"
-        - "Current"
-      "crypto/x509":
-        - "Subjects"
-      "io":
-        - "ReadAtLeast"
-        - "ReadFull"
-      "database/sql":
-        - "Query"
-        - "QueryRow"
-      "bytes":
-        - "String"
-        - "ReadBytes"
-        - "ReadByte"
-      "bufio":
-        - "Text"
-        - "Bytes"
-        - "ReadString"
-        - "ReadSlice"
-        - "ReadRune"
-        - "ReadLine"
-        - "ReadBytes"
-        - "ReadByte"
-      "archive/tar":
-        - "Next"
-        - "FileInfo"
-        - "Header"
-      "net/url":
-        - "ParseQuery"
-        - "ParseUriRequest"
-        - "Parse"
-        - "Query"
-    # Sources that are defined in Go documentation as a "type" go here (note: adding types will consider all functions that use that type to be tainted).
-    types:
-      "net/http":
-        - "Request"
+  # Sources that are defined in Go documentation as a "variable" go here (note: these variables will have an SSA type of "Global").
+  variables:
+    "os":
+      - "Args"
+  # Sources that are defined in Go documentation as a "function" go here.
+  functions:
+    "flag":
+      - "Arg"
+      - "Args"
+    "os":
+      - "Environ"
+      - "File"
+      - "FileInfo"
+      - "FileMode"
+      - "Readdir"
+      - "Readdirnames"
+      - "OpenFile"
+    "crypto/tls":
+      - "LoadX509KeyPair"
+      - "X509KeyPair"
+    "os/user":
+      - "Lookup"
+      - "LookupId"
+      - "Current"
+    "crypto/x509":
+      - "Subjects"
+    "io":
+      - "ReadAtLeast"
+      - "ReadFull"
+    "database/sql":
+      - "Query"
+      - "QueryRow"
+    "bytes":
+      - "String"
+      - "ReadBytes"
+      - "ReadByte"
+    "bufio":
+      - "Text"
+      - "Bytes"
+      - "ReadString"
+      - "ReadSlice"
+      - "ReadRune"
+      - "ReadLine"
+      - "ReadBytes"
+      - "ReadByte"
+    "archive/tar":
+      - "Next"
+      - "FileInfo"
+      - "Header"
+    "net/url":
+      - "ParseQuery"
+      - "ParseUriRequest"
+      - "Parse"
+      - "Query"
+  # Sources that are defined in Go documentation as a "type" go here (note: adding types will consider all functions that use that type to be tainted).
+  types:
+    "net/http":
+      - "Request"