Add sample DB-related queries

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/src/experimental/InconsistentCode/DeferInLoopGood.go

@@ -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/GORMErrorNotChecked.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.qhelp

@@ -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>

ql/src/experimental/InconsistentCode/GORMErrorNotChecked.ql

@@ -0,0 +1,35 @@
+/**
+ * @name GORM error not checked
+ * @description A call that interacts with the database using the GORM library
+ *              without checking whether there was an error.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/examples/gorm-error-not-checked
+ * @precision high
+ */
+
+import go
+import semmle.go.frameworks.SQL
+
+from DataFlow::MethodCallNode call
+where
+  exists(string name | call.getTarget().hasQualifiedName(Gorm::packagePath(), "DB", name) |
+    name != "InstantSet" and
+    name != "LogMode"
+  ) and
+  // the value from the call does not:
+  not exists(DataFlow::Node succ | TaintTracking::localTaintStep*(call, succ) |
+    // get assigned to any variables
+    succ = any(Write w).getRhs()
+    or
+    // get returned
+    succ instanceof DataFlow::ResultNode
+    or
+    // have any methods chained on it
+    exists(DataFlow::MethodCallNode m | succ = m.getReceiver())
+    or
+    // have its `Error` field read
+    exists(DataFlow::FieldReadNode fr | fr.readsField(succ, _, _, "Error"))
+  )
+select call,
+  "This call appears to interact with the database without checking whether an error was encountered."

ql/src/experimental/InconsistentCode/GORMErrorNotCheckedGood.go

@@ -0,0 +1,11 @@
+package main
+
+import "gorm.io/gorm"
+
+func getUserIdGood(db *gorm.DB, name string) int64 {
+	var user User
+	if err := db.Where("name = ?", name).First(&user).Error; err != nil {
+		// handle errors
+	}
+	return user.Id
+}

ql/test/experimental/CWE-400/DatabaseCallInLoop.expected

@@ -0,0 +1,13 @@
+edges
+| DatabaseCallInLoop.go:7:2:11:2 | range statement | DatabaseCallInLoop.go:9:3:9:41 | call to First |
+| test.go:10:1:12:1 | function declaration | test.go:11:2:11:13 | call to Take |
+| test.go:14:1:16:1 | function declaration | test.go:15:2:15:13 | call to runQuery |
+| test.go:15:2:15:13 | call to runQuery | test.go:10:1:12:1 | function declaration |
+| test.go:20:2:22:2 | for statement | test.go:21:3:21:14 | call to runQuery |
+| test.go:21:3:21:14 | call to runQuery | test.go:10:1:12:1 | function declaration |
+| test.go:24:2:26:2 | for statement | test.go:25:3:25:17 | call to runRunQuery |
+| test.go:25:3:25:17 | call to runRunQuery | test.go:14:1:16:1 | function declaration |
+#select
+| DatabaseCallInLoop.go:9:3:9:41 | call to First | DatabaseCallInLoop.go:7:2:11:2 | range statement | DatabaseCallInLoop.go:9:3:9:41 | call to First | $@ is called in $@ | DatabaseCallInLoop.go:9:3:9:41 | call to First | call to First | DatabaseCallInLoop.go:7:2:11:2 | range statement | a loop |
+| test.go:11:2:11:13 | call to Take | test.go:20:2:22:2 | for statement | test.go:11:2:11:13 | call to Take | $@ is called in $@ | test.go:11:2:11:13 | call to Take | call to Take | test.go:20:2:22:2 | for statement | a loop |
+| test.go:11:2:11:13 | call to Take | test.go:24:2:26:2 | for statement | test.go:11:2:11:13 | call to Take | $@ is called in $@ | test.go:11:2:11:13 | call to Take | call to Take | test.go:24:2:26:2 | for statement | a loop |

ql/test/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/test/experimental/CWE-400/DatabaseCallInLoop.qlref

@@ -0,0 +1 @@
+experimental/CWE-400/DatabaseCallInLoop.ql

ql/test/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/test/experimental/CWE-400/go.mod

@@ -0,0 +1,5 @@
+module query-tests/databasecallinloop
+
+go 1.16
+
+require gorm.io/gorm v1.21.12

ql/test/experimental/CWE-400/test.go

@@ -0,0 +1,27 @@
+package main
+
+import "gorm.io/gorm"
+
+type User struct {
+	Id   int64
+	Name string
+}
+
+func runQuery(db *gorm.DB) {
+	db.Take(nil)
+}
+
+func runRunQuery(db *gorm.DB) {
+	runQuery(db)
+}
+
+func main() {
+	var db *gorm.DB
+	for i := 0; i < 10; i++ {
+		runQuery(db)
+	}
+
+	for i := 10; i > 0; i-- {
+		runRunQuery(db)
+	}
+}