From a38e7045346b4bdbc074609f8284519e76be3cd6 Mon Sep 17 00:00:00 2001 From: Ted Reed Date: Mon, 10 Feb 2020 22:28:21 -0500 Subject: [PATCH 1/2] Add query to detect outoforder cpp/linux privilege dropping --- .../PrivilegeDroppingOutoforder.c | 138 ++++++++++++++++++ .../PrivilegeDroppingOutoforder.qhelp | 35 +++++ .../PrivilegeDroppingOutoforder.ql | 112 ++++++++++++++ 3 files changed, 285 insertions(+) create mode 100644 CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.c create mode 100644 CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.qhelp create mode 100644 CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql diff --git a/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.c b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.c new file mode 100644 index 0000000..ee98be5 --- /dev/null +++ b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.c @@ -0,0 +1,138 @@ +#include +#include +#include +#include + +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); +} diff --git a/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.qhelp b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.qhelp new file mode 100644 index 0000000..ca8d8df --- /dev/null +++ b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.qhelp @@ -0,0 +1,35 @@ + + + +

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.

+ +

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.

+ + + + +

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.

+ + + +

The following example demonstrates out of order calls.

+ + + + + +
  • CERT C Coding Standard: +POS37-C. Ensure that privilege relinquishment is successful. +
    • + +     +     diff --git a/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql new file mode 100644 index 0000000..d08b9e6 --- /dev/null +++ b/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 From 4de227a5c37380bbab021cf90f267fc041fdb1ef Mon Sep 17 00:00:00 2001 From: Ted Reed Date: Thu, 13 Feb 2020 17:45:48 -0500 Subject: [PATCH 2/2] Apply code review feedback --- .../PrivilegeDroppingOutoforder.ql | 105 ++++++++---------- 1 file changed, 44 insertions(+), 61 deletions(-) diff --git a/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql index d08b9e6..3e94365 100644 --- a/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql +++ b/CodeQL_Queries/cpp/PrivilegeDropping/PrivilegeDroppingOutoforder.ql @@ -13,100 +13,83 @@ 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" + getTarget().hasGlobalName("setuid") or + getTarget().hasGlobalName("setresuid") + } +} + +class SetuidLikeWrapperCall extends FunctionCall { + SetuidLikeFunctionCall baseCall; + + SetuidLikeWrapperCall() { + this = baseCall or + exists(SetuidLikeWrapperCall fc | + this.getTarget() = fc.getEnclosingFunction() and + baseCall = fc.getBaseCall() + ) + } + + SetuidLikeFunctionCall getBaseCall() { + result = baseCall } } class CallBeforeSetuidFunctionCall extends FunctionCall { CallBeforeSetuidFunctionCall() { // setgid/setresgid with the root group are false positives. - getTarget().getQualifiedName() = "setgid" or - getTarget().getQualifiedName() = "setresgid" or + getTarget().hasGlobalName("setgid") or + getTarget().hasGlobalName("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 + getTarget().hasGlobalName("initgroups") or + getTarget().hasGlobalName("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 - ) + getTarget().hasGlobalName("pidfile_create") } +} - private FunctionCall controlFlowElementContains(ControlFlowNode cfn, FunctionCall guard) { - guard.getBasicBlock() != cfn.getBasicBlock() and - exists (ControlFlowNode subnode | - subnode = cfn.getBasicBlock().getANode+() | - result = calls(subnode, guard) - ) - } +class CallBeforeSetuidWrapperCall extends FunctionCall { + CallBeforeSetuidFunctionCall baseCall; - private FunctionCall controlFlowElementCalls(ControlFlowNode cfn, FunctionCall guard) { - exists(ControlFlowNode subnode | - subnode = cfn.(FunctionCall).getTarget().getBlock().getAChild+() | - result = calls(subnode, guard) + CallBeforeSetuidWrapperCall() { + this = baseCall or + exists(CallBeforeSetuidWrapperCall fc | + this.getTarget() = fc.getEnclosingFunction() and + baseCall = fc.getBaseCall() ) } - private FunctionCall controlFlowElementIs(SetuidLikeFunctionCall cfn) { - result = cfn + CallBeforeSetuidFunctionCall getBaseCall() { + result = baseCall } } -predicate withinCondition(Expr t) { - exists(Expr src | src = t.getParent+() | src.isCondition()) or t.isCondition() +predicate setuidBeforeSetgid( + SetuidLikeWrapperCall setuidWrapper, + CallBeforeSetuidWrapperCall setgidWrapper) { + setgidWrapper.getAPredecessor+() = setuidWrapper } predicate flowsToCondition(Expr fc) { exists(DataFlow::Node source, DataFlow::Node sink | TaintTracking::localTaint(source, sink) and fc = source.asExpr() and - withinCondition(sink.asExpr()) + (sink.asExpr().getParent*().(ControlFlowNode).isCondition() or sink.asExpr().isCondition()) ) } from - CallBeforeSetuidFunctionCall func, - Function funcParent, + Function func, + CallBeforeSetuidFunctionCall fc, SetuidLikeFunctionCall setuid where - func.isCalledAfter(setuid) and + setuidBeforeSetgid(setuid, fc) 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 " + + (not flowsToCondition(fc) or not flowsToCondition(setuid)) and + func = fc.getEnclosingFunction() +select fc, "This function is called within " + func + ", and potentially after " + "setuid/setresuid, and may not succeed. Be sure to check the return code and errno", setuid