[clang][analyzer] Add checker 'security.SetgidSetuidOrder'

clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp

@@ -30,23 +30,20 @@ enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
 
 class SetgidSetuidOrderChecker
     : public Checker<check::PostCall, check::DeadSymbols, eval::Assume> {
-  const BugType BT_WrongRevocationOrder{
-      this, "Possible wrong order of privilege revocation"};
+  const BugType BT{this, "Possible wrong order of privilege revocation"};
 
   const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
   const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
 
   const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
   const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
 
-  CallDescriptionSet OtherSetPrivilegeDesc{
+  CallDescriptionSet const OtherSetPrivilegeDesc{
       {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
       {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
       {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
 
 public:
-  SetgidSetuidOrderChecker() {}
-
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
@@ -105,7 +102,7 @@ void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper &SymReaper,
     return;
 
   State = State->set<LastSetuidCallSVal>(SymbolRef{});
-  C.addTransition(State, C.getPredecessor());
+  C.addTransition(State);
 }
 
 ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
@@ -116,16 +113,21 @@ ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
   if (!LastSetuidSym)
     return State;
 
-  // Check if the most recent call to 'setuid(getuid())' is assumed to be -1.
-  auto FailVal =
-      SVB.evalBinOpNN(State, BO_EQ, nonloc::SymbolVal(LastSetuidSym),
-                      SVB.makeIntVal(-1, false), SVB.getConditionType())
+  // Check if the most recent call to 'setuid(getuid())' is assumed to be != 0.
+  // It should be only -1 at failure, but we want to accept a "!= 0" check too.
+  // (But now an invalid failure check like "!= 1" will be recognized as correct
+  // too. The "invalid failure check" is a different bug that is not the scope
+  // of this checker.)
+  auto FailComparison =
+      SVB.evalBinOpNN(State, BO_NE, nonloc::SymbolVal(LastSetuidSym),
+                      SVB.makeIntVal(0, /*isUnsigned=*/false),
+                      SVB.getConditionType())
           .getAs<DefinedOrUnknownSVal>();
-  if (!FailVal)
+  if (!FailComparison)
     return State;
-  auto IsFailBranch = State->assume(*FailVal);
-  if (IsFailBranch.first && !IsFailBranch.second) {
-    // This is the 'setuid(getuid())' == -1 case.
+  if (auto IsFailBranch = State->assume(*FailComparison);
+      IsFailBranch.first && !IsFailBranch.second) {
+    // This is the 'setuid(getuid())' != 0 case.
     // On this branch we do not want to emit warning.
     State = State->set<LastSetuidCallSVal>(SymbolRef{});
     State = State->set<LastSetPrivilegeCall>(Irrelevant);
@@ -156,9 +158,9 @@ ProgramStateRef SetgidSetuidOrderChecker::processSetgid(
       return nullptr;
     }
     return State->set<LastSetPrivilegeCall>(Irrelevant);
-  } else
-    return State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
-                                                               : Irrelevant);
+  }
+  return State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
+                                                             : Irrelevant);
 }
 
 ProgramStateRef SetgidSetuidOrderChecker::processOther(
@@ -169,9 +171,9 @@ ProgramStateRef SetgidSetuidOrderChecker::processOther(
 
 bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
     const CallDescription &Desc, const CallEvent &Call) const {
-  if (const auto *CE =
+  if (const auto *CallInArg0 =
           dyn_cast<CallExpr>(Call.getArgExpr(0)->IgnoreParenImpCasts()))
-    return Desc.matchesAsWritten(*CE);
+    return Desc.matchesAsWritten(*CallInArg0);
   return false;
 }
 
@@ -182,8 +184,7 @@ void SetgidSetuidOrderChecker::emitReport(ProgramStateRef State,
         "A 'setgid(getgid())' call following a 'setuid(getuid())' "
         "call is likely to fail; probably the order of these "
         "statements is wrong";
-    C.emitReport(std::make_unique<PathSensitiveBugReport>(
-        BT_WrongRevocationOrder, Msg, N));
+    C.emitReport(std::make_unique<PathSensitiveBugReport>(BT, Msg, N));
   }
 }
 

clang/test/Analysis/setgid-setuid-order.c

@@ -18,14 +18,31 @@ gid_t getgid();
 
 
 void correct_order() {
+  // A correct revocation sequence starts here.
   if (setgid(getgid()) == -1)
     return;
   if (setuid(getuid()) == -1)
     return;
+  // No warning for the following setgid statement.
+  // The previous setgid and setuid calls are a correct privilege revocation
+  // sequence. The checker does not care about the following statements (except
+  // if a wrong setuid-setgid sequence follows again).
   if (setgid(getgid()) == -1)
     return;
 }
 
+void incorrect_after_correct() {
+  if (setgid(getgid()) == -1)
+    return;
+  if (setuid(getuid()) == -1)
+    return;
+  // Incorrect sequence starts here.
+  if (setuid(getuid()) == -1)
+    return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+    return;
+}
+
 void incorrect_order() {
   if (setuid(getuid()) == -1)
     return;
@@ -77,11 +94,13 @@ void setuid_other_between() {
 void setgid_with_getuid() {
   if (setuid(getuid()) == -1)
     return;
+  // add a clang-tidy check for this case?
   if (setgid(getuid()) == -1)
     return;
 }
 
 void setuid_with_getgid() {
+  // add a clang-tidy check for this case?
   if (setuid(getgid()) == -1)
     return;
   if (setgid(getgid()) == -1)
@@ -157,14 +176,25 @@ void setresgid_between() {
     return;
 }
 
-void other_system_function_between() {
+void getgid_getuid_between() {
   if (setuid(getuid()) == -1)
     return;
-  gid_t g = getgid();
+  (void)getgid();
+  (void)getuid();
   if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
     return;
 }
 
+void stored_getgid_getuid() {
+  // possible future improvement: detect this case
+  uid_t u = getuid();
+  gid_t g = getgid();
+  if (setuid(u) == -1)
+    return;
+  if (setgid(g) == -1) // no warning
+    return;
+}
+
 void f_extern();
 
 void other_unknown_function_between() {
@@ -177,9 +207,51 @@ void other_unknown_function_between() {
 
 void setuid_error_case() {
   if (setuid(getuid()) == -1) {
-    setgid(getgid());
+    // No warning if we know that the first setuid call has failed.
+    (void)setgid(getgid());
     return;
   }
-  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+  (void)setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void setuid_success_case() {
+  if (setuid(getuid()) == 0) {
+    if (setgid(getgid()) == 0) { // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+    }
+  }
+}
+
+void incorrect_order_compare_zero() {
+  if (setuid(getuid()) != 0)
+    return;
+  (void)setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void setuid_error_case_compare_zero() {
+  if (setuid(getuid()) != 0) {
+    // No warning if we know that the first setuid call has failed.
+    (void)setgid(getgid());
+    return;
+  }
+}
+
+void incorrect_order_compare_other() {
+  if (setuid(getuid()) == -2) {
+    // This is a case for improvement:
+    // The checker does not recognize that this is an invalid error check,
+    // but this is really another type of bug not related to this checker.
+    (void)setgid(getgid()); // warning should appear here
+    return;
+  }
+  if (setgid(getgid()) == -2) { // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+    return;
+  }
+}
+
+const int FAIL = -1;
+
+void incorrect_order_compare_var() {
+  if (setuid(getuid()) == FAIL)
     return;
+  (void)setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
 }