Golang : Add Query To Detect PAM Authorization Bugs

Author: Porcupiney Hairs

go/ql/src/experimental/CWE-285/PamAuthBad.go

@@ -0,0 +1,16 @@
+func bad() error {
+	t, err := pam.StartFunc("", "username", func(s pam.Style, msg string) (string, error) {
+		switch s {
+		case pam.PromptEchoOff:
+			return string(pass), nil
+		}
+		return "", fmt.Errorf("unsupported message style")
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	if err := t.Authenticate(0); err != nil {
+		return nil, fmt.Errorf("Authenticate: %w", err)
+	}
+}

go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp

@@ -0,0 +1,53 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+            Using only a call to
+            <code>pam.Authenticate</code>
+            to check the validity of a login can lead to authorization bypass vulnerabilities.
+        </p>
+        <p>
+            A
+            <code>pam.Authenticate</code>
+            only verifies the credentials of a user. It does not check if a user has an
+      appropriate authorization to actually login. This means a user with an expired
+      login or a password can still access the system.
+        </p>
+
+    </overview>
+
+    <recommendation>
+        <p>
+            A call to
+            <code>pam.Authenticate</code>
+            should be followed by a call to
+            <code>pam.AcctMgmt</code>
+            to check if a user is allowed to login.
+        </p>
+    </recommendation>
+
+    <example>
+        <p>
+            In the following example, the code only checks the credentials of a user. Hence,
+      in this case, a user with expired credentials can still login. This can be
+      verified by creating a new user account, expiring it with
+            <code>chage -E0 `username` </code>
+            and then trying to log in.
+        </p>
+        <sample src="PamAuthBad.go" />
+
+        <p>
+            This can be avoided by calling
+            <code>pam.AcctMgmt</code>
+            call to verify access as has been done in the snippet shown below.
+        </p>
+        <sample src="PamAuthGood.go" />
+    </example>
+
+    <references>
+        <li>
+            Man-Page:
+            <a href="https://man7.org/linux/man-pages/man3/pam_acct_mgmt.3.html">pam_acct_mgmt</a>
+        </li>
+    </references>
+</qhelp>

go/ql/src/experimental/CWE-285/PamAuthBypass.ql

@@ -0,0 +1,69 @@
+/**
+ * @name PAM authorization bypass due to incorrect usage
+ * @description Not using `pam.AcctMgmt` after `pam.Authenticate` to check the validity of a login can lead to authorization bypass.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/unreachable-statement
+ * @tags maintainability
+ *       correctness
+ *       external/cwe/cwe-561
+ *       external/cwe/cwe-285
+ * @precision very-high
+ */
+
+import go
+
+predicate isInTestFile(Expr r) {
+  r.getFile().getAbsolutePath().matches("%test%") and
+  not r.getFile().getAbsolutePath().matches("%/ql/test/%")
+}
+
+class PamAuthenticate extends Method {
+  PamAuthenticate() {
+    this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "Authenticate")
+  }
+}
+
+class PamAcctMgmt extends Method {
+  PamAcctMgmt() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "AcctMgmt") }
+}
+
+class PamTransaction extends StructType {
+  PamTransaction() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction") }
+}
+
+class PamStartFunc extends Function {
+  PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) }
+}
+
+class PamAuthBypassConfiguration extends TaintTracking::Configuration {
+  PamAuthBypassConfiguration() { this = "PAM auth bypass" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(PamStartFunc p | p.getACall().getResult(0) = source)
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(PamAcctMgmt p | p.getACall().getReceiver() = sink)
+  }
+}
+
+class PamAuthBypassConfig extends TaintTracking::Configuration {
+  PamAuthBypassConfig() { this = "PAM auth bypass2" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(PamStartFunc p | p.getACall().getResult(0) = source)
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(PamAuthenticate p | p.getACall().getReceiver() = sink)
+  }
+}
+
+from
+  PamAuthBypassConfiguration config, PamAuthBypassConfig config2, DataFlow::Node source,
+  DataFlow::Node sink
+where
+  not isInTestFile(source.asExpr()) and
+  (config2.hasFlow(source, sink) and not config.hasFlow(source, _))
+select source, "This Pam transaction may not be secure."

go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql

@@ -0,0 +1,50 @@
+/**
+ * @name PAM authorization bypass due to incorrect usage
+ * @description Not using `pam.AcctMgmt` after `pam.Authenticate` to check the validity of a login can lead to authorization bypass.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/unreachable-statement
+ * @tags maintainability
+ *       correctness
+ *       external/cwe/cwe-561
+ *       external/cwe/cwe-285
+ * @precision very-high
+ */
+
+import go
+
+predicate isInTestFile(Expr r) {
+  r.getFile().getAbsolutePath().matches("%test%") and
+  not r.getFile().getAbsolutePath().matches("%/ql/test/%")
+}
+
+class PamAuthenticate extends Method {
+  PamAuthenticate() {
+    this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "Authenticate")
+  }
+}
+
+class PamAcctMgmt extends Method {
+  PamAcctMgmt() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "AcctMgmt") }
+}
+
+class PamTransaction extends StructType {
+  PamTransaction() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction") }
+}
+
+class PamStartFunc extends Function {
+  PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) }
+}
+
+from
+  DataFlow::Node source, DataFlow::Node sink, PamAuthenticate auth, PamStartFunc start,
+  PamAcctMgmt actmgt
+where
+  not isInTestFile(source.asExpr()) and
+  (
+    start.getACall().getResult(0) = source and
+    sink = auth.getACall().getReceiver() and
+    DataFlow::localFlow(source, sink) and
+    not DataFlow::localFlow(source, actmgt.getACall().getReceiver())
+  )
+select source, "This Pam transaction may not check authorization correctly."

go/ql/src/experimental/CWE-285/PamAuthGood.go

@@ -0,0 +1,19 @@
+func good() error {
+	t, err := pam.StartFunc("", "username", func(s pam.Style, msg string) (string, error) {
+		switch s {
+		case pam.PromptEchoOff:
+			return string(pass), nil
+		}
+		return "", fmt.Errorf("unsupported message style")
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	if err := t.Authenticate(0); err != nil {
+		return nil, fmt.Errorf("Authenticate: %w", err)
+	}
+	if err := t.AcctMgmt(0); err != nil {
+		return nil, fmt.Errorf("AcctMgmt: %w", err)
+	}
+}

go/ql/test/experimental/CWE-285/PamAuthBypass.expected

@@ -0,0 +1 @@
+| main.go:10:2:12:3 | ... := ...[0] | This Pam transaction may not be secure. |

go/ql/test/experimental/CWE-285/PamAuthBypass.qlref

@@ -0,0 +1 @@
+experimental/CWE-285/PamAuthBypass.ql

go/ql/test/experimental/CWE-285/go.mod

@@ -0,0 +1,5 @@
+module main
+
+go 1.18
+
+require github.com/msteinert/pam v1.0.0

go/ql/test/experimental/CWE-285/main.go

@@ -0,0 +1,28 @@
+package main
+
+//go:generate depstubber -vendor github.com/msteinert/pam Style,Transaction StartFunc
+
+import (
+	"github.com/msteinert/pam"
+)
+
+func bad() error {
+	t, _ := pam.StartFunc("", "", func(s pam.Style, msg string) (string, error) {
+		return "", nil
+	})
+	return t.Authenticate(0)
+
+}
+
+func good() error {
+	t, err := pam.StartFunc("", "", func(s pam.Style, msg string) (string, error) {
+		return "", nil
+	})
+	err = t.Authenticate(0)
+	if err != nil {
+		return err
+	}
+	return t.AcctMgmt(0)
+}
+
+func main() {}