Merge pull request #558 from sauyon/add-sample-queries

Add sample DB-related queries

Author: smowton

ql/src/experimental/CWE-400/DatabaseCallInLoop.go

@@ -0,0 +1,13 @@
+package main
+
+import "gorm.io/gorm"
+
+func getUsers(db *gorm.DB, names []string) []User {
+	res := make([]User, 0, len(names))
+	for _, name := range names {
+		var user User
+		db.Where("name = ?", name).First(&user)
+		res = append(res, user)
+	}
+	return res
+}

ql/src/experimental/CWE-400/DatabaseCallInLoop.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Database calls in loops are slower than running a single query and consume more resources. This
+can lead to denial of service attacks if the loop bounds can be controlled by an attacker.</p>
+</overview>
+<recommendation>
+
+<p>Ensure that where possible, database queries are not run in a loop, instead running a single query to get all relevant data.</p>
+
+</recommendation>
+<example>
+
+<p>In the example below, users in a database are queried one by one in a loop:</p>
+
+<sample src="DatabaseCallInLoop.go" />
+
+<p>This is corrected by running a single query that selects all of the users at once:</p>
+
+<sample src="DatabaseCallInLoopGood.go" />
+
+</example>
+<references>
+
+</references>
+</qhelp>

ql/src/experimental/CWE-400/DatabaseCallInLoop.ql

@@ -0,0 +1,69 @@
+/**
+ * @name Database call in loop
+ * @description Detects database operations within loops.
+ *              Doing operations in series can be slow and lead to N+1 situations.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id go/examples/database-call-in-loop
+ */
+
+import go
+
+class DatabaseAccess extends DataFlow::MethodCallNode {
+  DatabaseAccess() {
+    exists(string name |
+      this.getTarget().hasQualifiedName(Gorm::packagePath(), "DB", name) and
+      // all terminating Gorm methods
+      name =
+        [
+          "Find", "Take", "Last", "Scan", "Row", "Rows", "ScanRows", "Pluck", "Count", "First",
+          "FirstOrInit", "FindOrCreate", "Update", "Updates", "UpdateColumn", "UpdateColumns",
+          "Save", "Create", "Delete", "Exec"
+        ]
+    )
+  }
+}
+
+class CallGraphNode extends Locatable {
+  CallGraphNode() {
+    this instanceof LoopStmt
+    or
+    this instanceof CallExpr
+    or
+    this instanceof FuncDef
+  }
+}
+
+/**
+ * Holds if `pred` calls `succ`, i.e. is an edge in the call graph,
+ * This includes explicit edges from call -> callee, to produce better paths.
+ */
+predicate callGraphEdge(CallGraphNode pred, CallGraphNode succ) {
+  // Go from a loop to an enclosed expression.
+  pred.(LoopStmt).getBody().getAChild*() = succ.(CallExpr)
+  or
+  // Go from a call to the called function.
+  pred.(CallExpr) = succ.(FuncDef).getACall().asExpr()
+  or
+  // Go from a function to an enclosed loop.
+  pred.(FuncDef) = succ.(LoopStmt).getEnclosingFunction()
+  or
+  // Go from a function to an enclosed call.
+  pred.(FuncDef) = succ.(CallExpr).getEnclosingFunction()
+}
+
+query predicate edges(CallGraphNode pred, CallGraphNode succ) {
+  callGraphEdge(pred, succ) and
+  // Limit the range of edges to only those that are relevant.
+  // This helps to speed up the query by reducing the size of the outputted path information.
+  exists(LoopStmt loop, DatabaseAccess dbAccess |
+    // is between a loop and a db access
+    callGraphEdge*(loop, pred) and
+    callGraphEdge*(succ, dbAccess.asExpr())
+  )
+}
+
+from LoopStmt loop, DatabaseAccess dbAccess
+where edges*(loop, dbAccess.asExpr())
+select dbAccess, loop, dbAccess, "$@ is called in $@", dbAccess, dbAccess.toString(), loop, "a loop"

ql/src/experimental/CWE-400/DatabaseCallInLoopGood.go

@@ -0,0 +1,9 @@
+package main
+
+import "gorm.io/gorm"
+
+func getUsersGood(db *gorm.DB, names []string) []User {
+	res := make([]User, 0, len(names))
+	db.Where("name IN ?", names).Find(&res)
+	return res
+}

ql/src/experimental/InconsistentCode/DeferInLoop.go

@@ -0,0 +1,14 @@
+package main
+
+import "os"
+
+func openFiles(filenames []string) {
+	for _, filename := range filenames {
+		file, err := os.Open(filename)
+		defer file.Close()
+		if err != nil {
+			// handle error
+		}
+		// work on file
+	}
+}

ql/src/experimental/InconsistentCode/DeferInLoop.qhelp

@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>A deferred statement in a loop will not execute until the end of the function. This can lead to unintentionally holding resources open, like file handles or database transactions.</p>
+</overview>
+<recommendation>
+
+<p>Either run the deferred function manually, or create a subroutine that contains the defer.</p>
+
+</recommendation>
+<example>
+
+<p>In the example below, the files opened in the loop are not closed until the end of the function:</p>
+
+<sample src="DeferInLoop.go" />
+
+<p>The corrected version puts the loop body into a function.</p>
+
+<sample src="DeferInLoopGood.go" />
+
+</example>
+<references>
+
+<li>
+  <a href="https://golang.org/ref/spec#Defer_statements">Defer statements</a>.
+</li>
+
+</references>
+</qhelp>

ql/examples/snippets/deferinloop.ql …rimental/InconsistentCode/DeferInLoop.qlql/examples/snippets/deferinloop.ql renamed to ql/src/experimental/InconsistentCode/DeferInLoop.ql ql/examples/snippets/deferinloop.ql renamed to ql/src/experimental/InconsistentCode/DeferInLoop.ql

@@ -0,0 +1,18 @@
+package main
+
+import "os"
+
+func openFile(filename string) {
+	file, err := os.Open(filename)
+	defer file.Close()
+	if err != nil {
+		// handle error
+	}
+	// work on file
+}
+
+func openFilesGood(filenames []string) {
+	for _, filename := range filenames {
+		openFile(filename)
+	}
+}

ql/src/experimental/InconsistentCode/DeferInLoopGood.go

@@ -0,0 +1,9 @@
+package main
+
+import "gorm.io/gorm"
+
+func getUserId(db *gorm.DB, name string) int64 {
+	var user User
+	db.Where("name = ?", name).First(&user)
+	return user.Id
+}

ql/src/experimental/InconsistentCode/GORMErrorNotChecked.go

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>GORM errors are returned as a field of the return value instead of a separate return value.</p>
+
+<p>It is therefore very easy to miss that an error may occur and omit error handling routines.</p>
+</overview>
+<recommendation>
+
+<p>Ensure that GORM errors are checked.</p>
+
+</recommendation>
+<example>
+
+<p>In the example below, the error from the database query is never checked:</p>
+
+<sample src="GORMErrorNotChecked.go" />
+
+<p>The corrected version checks and handles the error before returning.</p>
+
+<sample src="GORMErrorNotCheckedGood.go" />
+
+</example>
+<references>
+
+<li>
+  <a href="https://gorm.io/docs/error_handling.html">GORM Error Handling</a>.
+</li>
+
+</references>
+</qhelp>