Add query to detect out-of-order cpp/linux privilege dropping

CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.c

@@ -0,0 +1,138 @@
+#include <stdlib.h>
+#include <sys/param.h>
+#include <unistd.h>
+#include <pwd.h>
+
+void callSetuidAndCheck(int uid) {
+    if (setuid(uid) != 0) {
+        exit(1);
+    }
+}
+
+void callSetgidAndCheck(int gid) {
+    if (setgid(gid) != 0) {
+        exit(1);
+    }
+}
+
+/// Correct ways to drop priv.
+
+void correctDropPrivInline() {
+    if (setgroups(0, NULL)) {
+        exit(1);
+    }
+
+    if (setgid(-2) != 0) {
+        exit(1);
+    }
+
+    if (setuid(-2) != 0) {
+        exit(1);
+    }
+}
+
+void correctDropPrivInScope() {
+    {
+        if (setgroups(0, NULL)) {
+            exit(1);
+        }
+    }
+
+    {
+        if (setgid(-2) != 0) {
+            exit(1);
+        }
+    }
+
+    {
+        if (setuid(-2) != 0) {
+            exit(1);
+        }
+    }
+}
+
+void correctOrderForInitgroups() {
+    struct passwd *pw = getpwuid(0);
+    if (pw) {
+        if (initgroups(pw->pw_name, -2)) {
+            exit(1);
+        }
+    } else {
+        // Unhandled.
+    }
+    int rc = setuid(-2);
+    if (rc) {
+        exit(1);
+    }
+}
+
+void correctDropPrivInScopeParent() {
+    {
+        callSetgidAndCheck(-2);
+    }
+    correctOrderForInitgroups();
+}
+
+void incorrectNoReturnCodeCheck() {
+    int user = -2;
+    if (user) {
+        if (user) {
+            int rc = setgid(user);
+            (void)rc;
+            initgroups("nobody", user);
+        }
+        if (user) {
+            setuid(user);
+        }
+    }
+}
+
+void correctDropPrivInFunctionCall() {
+    if (setgroups(0, NULL)) {
+        exit(1);
+    }
+
+    callSetgidAndCheck(-2);
+    callSetuidAndCheck(-2);
+}
+
+/// Incorrect, out of order gid and uid.
+/// Calling uid before gid will fail.
+
+void incorrectDropPrivOutOfOrderInline() {
+    if (setuid(-2) != 0) {
+        exit(1);
+    }
+
+    if (setgid(-2) != 0) {
+        exit(1);
+    }
+}
+
+void incorrectDropPrivOutOfOrderInScope() {
+    {
+        if (setuid(-2) != 0) {
+            exit(1);
+        }
+    }
+
+    setgid(-2);
+}
+
+void incorrectDropPrivOutOfOrderWithFunction() {
+    callSetuidAndCheck(-2);
+
+    if (setgid(-2) != 0) {
+        exit(1);
+    }
+}
+
+void incorrectDropPrivOutOfOrderWithFunction2() {
+    callSetuidAndCheck(-2);
+    callSetgidAndCheck(-2);
+}
+
+void incorrectDropPrivNoCheck() {
+    setgid(-2);
+    setuid(-2);
+}

CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The code attempts to drop privilege in an incorrect order by
+erroneous dropping user privilege before groups. This has security
+impact if the return codes are not checked.</p>
+
+<p>False positives include code performing negative checks, making
+sure that setgid or setgroups does not work, meaning permissions are
+dropped. Additionally, other forms of sandboxing may be present removing
+any residual risk, for example a dedicated user namespace.</p>
+
+</overview>
+<recommendation>
+
+<p>Set the new group ID, then set the target user's intended groups by
+dropping previous supplemental source groups and initializing target
+groups, and finally set the target user.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates out of order calls.</p>
+<sample src="PrivilegeDroppingOutoforder.c" />
+
+</example>
+<references>
+
+<li>CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/POS37-C.+Ensure+that+privilege+relinquishment+is+successful">POS37-C. Ensure that privilege relinquishment is successful</a>.
+</li>
+
+</references>
+</qhelp>

CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql

@@ -0,0 +1,112 @@
+/**
+ * @name PrivilegeDroppingOutoforder
+ * @kind problem
+ * @problem.severity recommendation
+ * @id cpp/drop-permissions-outoforder
+ * @tags security
+ *       external/cwe/cwe-273
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.TaintTracking
+
+class SetuidLikeFunctionCall extends FunctionCall {
+  SetuidLikeFunctionCall() {
+    // setuid/setresuid with the root user are false positives.
+    getTarget().getQualifiedName() = "setuid" or
+    getTarget().getQualifiedName() = "setresuid"
+  }
+}
+
+class CallBeforeSetuidFunctionCall extends FunctionCall {
+  CallBeforeSetuidFunctionCall() {
+    // setgid/setresgid with the root group are false positives.
+    getTarget().getQualifiedName() = "setgid" or
+    getTarget().getQualifiedName() = "setresgid" or
+    // Compatibility may require skipping initgroups and setgroups return checks.
+    // A stricter best practice is to check the result and errnor for EPERM.
+    getTarget().getQualifiedName() = "initgroups" or
+    getTarget().getQualifiedName() = "setgroups" or
+    // Find variants of CVE-2017-11747 where the low-priv user can stop the process.
+    // Feel free to extend this with other variants.
+    getTarget().getQualifiedName() = "pidfile_create"
+  }
+
+  predicate isCalledAfter(SetuidLikeFunctionCall fc) {
+    exists(ControlFlowNode cfn |
+        cfn = this.getAPredecessor+() or
+        // Expressions before enclosing function.
+        cfn = callerPredecessor(this.getEnclosingFunction().getACallToThisFunction()) |
+        fc = calls(cfn, this)
+    )
+  }
+
+  private FunctionCall calls(ControlFlowNode cfn, FunctionCall guard) {
+    result = controlFlowElementIs(cfn) or
+    result = controlFlowElementContains(cfn, guard) or
+    result = controlFlowElementCalls(cfn, guard)
+  }
+
+  private predicate callsSelf(ControlFlowNode cfn, ControlFlowNode caller) {
+    cfn = caller or
+    exists (ControlFlowNode subnode |
+        subnode = cfn.(Block).getAChild+() or
+        subnode = cfn.(FunctionCall).getTarget().getBlock().getAChild+() |
+        subnode = caller or callsSelf(subnode, caller)
+    )
+  }
+
+  private ControlFlowNode callerPredecessor(ControlFlowNode caller) {
+    exists(ControlFlowNode cfn |
+      cfn = caller.getEnclosingStmt().getAPredecessor+() |
+      not callsSelf(cfn, caller) and
+      result = cfn
+    )
+  }
+
+  private FunctionCall controlFlowElementContains(ControlFlowNode cfn, FunctionCall guard) {
+    guard.getBasicBlock() != cfn.getBasicBlock() and
+    exists (ControlFlowNode subnode |
+      subnode = cfn.getBasicBlock().getANode+() |
+      result = calls(subnode, guard)
+    )
+  }
+
+  private FunctionCall controlFlowElementCalls(ControlFlowNode cfn, FunctionCall guard) {
+    exists(ControlFlowNode subnode |
+      subnode = cfn.(FunctionCall).getTarget().getBlock().getAChild+() |
+      result = calls(subnode, guard)
+    )
+  }
+
+  private FunctionCall controlFlowElementIs(SetuidLikeFunctionCall cfn) {
+    result = cfn
+  }
+}
+
+predicate withinCondition(Expr t) {
+  exists(Expr src | src = t.getParent+() | src.isCondition()) or t.isCondition()
+}
+
+predicate flowsToCondition(Expr fc) {
+  exists(DataFlow::Node source, DataFlow::Node sink |
+    TaintTracking::localTaint(source, sink) and
+    fc = source.asExpr() and
+    withinCondition(sink.asExpr())
+  )
+}
+
+from
+  CallBeforeSetuidFunctionCall func,
+  Function funcParent,
+  SetuidLikeFunctionCall setuid
+where
+  func.isCalledAfter(setuid) and
+  // Require the call return code to be used in a condition.
+  // This introduces false negatives where the return is checked but then
+  // errno == EPERM allows execution to continue.
+  (not flowsToCondition(func) or not flowsToCondition(setuid)) and
+  funcParent = func.getEnclosingFunction()
+select func, "This function is called within " + funcParent + ", and potentially after " +
+  "setuid/setresuid, and may not succeed. Be sure to check the return code and errno",
+  setuid