From 11b97da83141db857361ec9535dcd637ffcd0439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= Date: Wed, 22 May 2024 12:11:18 +0200 Subject: [PATCH] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (#91445) --- clang/docs/analyzer/checkers.rst | 41 +++ .../clang/StaticAnalyzer/Checkers/Checkers.td | 5 + .../StaticAnalyzer/Checkers/CMakeLists.txt | 1 + .../Checkers/SetgidSetuidOrderChecker.cpp | 196 +++++++++++++ .../test/Analysis/setgid-setuid-order-notes.c | 73 +++++ clang/test/Analysis/setgid-setuid-order.c | 257 ++++++++++++++++++ 6 files changed, 573 insertions(+) create mode 100644 clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp create mode 100644 clang/test/Analysis/setgid-setuid-order-notes.c create mode 100644 clang/test/Analysis/setgid-setuid-order.c diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index eb8b58323da4d78..b4bd9dac1cbc2d5 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -1179,6 +1179,47 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C) strncpy(buf, "a", 1); // warn } +security.SetgidSetuidOrder (C) +"""""""""""""""""""""""""""""" +When dropping user-level and group-level privileges in a program by using +``setuid`` and ``setgid`` calls, it is important to reset the group-level +privileges (with ``setgid``) first. Function ``setgid`` will likely fail if +the superuser privileges are already dropped. + +The checker checks for sequences of ``setuid(getuid())`` and +``setgid(getgid())`` calls (in this order). If such a sequence is found and +there is no other privilege-changing function call (``seteuid``, ``setreuid``, +``setresuid`` and the GID versions of these) in between, a warning is +generated. The checker finds only exactly ``setuid(getuid())`` calls (and the +GID versions), not for example if the result of ``getuid()`` is stored in a +variable. + +.. code-block:: c + + void test1() { + // ... + // end of section with elevated privileges + // reset privileges (user and group) to normal user + if (setuid(getuid()) != 0) { + handle_error(); + return; + } + if (setgid(getgid()) != 0) { // warning: A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail + handle_error(); + return; + } + // user-ID and group-ID are reset to normal user now + // ... + } + +In the code above the problem is that ``setuid(getuid())`` removes superuser +privileges before ``setgid(getgid())`` is called. To fix the problem the +``setgid(getgid())`` should be called first. Further attention is needed to +avoid code like ``setgid(getuid())`` (this checker does not detect bugs like +this) and always check the return value of these calls. + +This check corresponds to SEI CERT Rule `POS36-C `_. + .. _unix-checkers: unix diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 64414e3d37f7510..d0ba1ce548403e8 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">, Dependencies<[SecuritySyntaxChecker]>, Documentation; +def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">, + HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and " + "'setuid(getuid())' (CERT: POS36-C)">, + Documentation; + } // end "security" let ParentPackage = ENV in { diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 4443ffd09293881..45d3788f105dc9c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers ReturnUndefChecker.cpp ReturnValueChecker.cpp RunLoopAutoreleaseLeakChecker.cpp + SetgidSetuidOrderChecker.cpp SimpleStreamChecker.cpp SmartPtrChecker.cpp SmartPtrModeling.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp new file mode 100644 index 000000000000000..dbe3fd33a6b43fc --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp @@ -0,0 +1,196 @@ +//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls ---===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines a checker to detect possible reversed order of privilege +// revocations when 'setgid' and 'setuid' is used. +// +//===----------------------------------------------------------------------===// + +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" + +using namespace clang; +using namespace ento; + +namespace { + +enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid }; + +class SetgidSetuidOrderChecker : public Checker { + 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}; + + const CallDescriptionSet 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: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond, + bool Assumption) const; + +private: + void processSetuid(ProgramStateRef State, const CallEvent &Call, + CheckerContext &C) const; + void processSetgid(ProgramStateRef State, const CallEvent &Call, + CheckerContext &C) const; + void processOther(ProgramStateRef State, const CallEvent &Call, + CheckerContext &C) const; + /// Check if a function like \c getuid or \c getgid is called directly from + /// the first argument of function called from \a Call. + bool isFunctionCalledInArg(const CallDescription &Desc, + const CallEvent &Call) const; + void emitReport(ProgramStateRef State, CheckerContext &C) const; +}; + +} // end anonymous namespace + +/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not +/// followed by other different privilege-change functions. +/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we +/// have found the bug to be reported. Value \c Setgid is used too to prevent +/// warnings at a setgid-setuid-setgid sequence. +REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, SetPrivilegeFunctionKind) +/// Store the symbol value of the last 'setuid(getuid())' call. This is used to +/// detect if the result is compared to -1 and avoid warnings on that branch +/// (which is the failure branch of the call), and for identification of note +/// tags. +REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef) + +void SetgidSetuidOrderChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + if (SetuidDesc.matches(Call)) { + processSetuid(State, Call, C); + } else if (SetgidDesc.matches(Call)) { + processSetgid(State, Call, C); + } else if (OtherSetPrivilegeDesc.contains(Call)) { + processOther(State, Call, C); + } +} + +ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State, + SVal Cond, + bool Assumption) const { + SValBuilder &SVB = State->getStateManager().getSValBuilder(); + SymbolRef LastSetuidSym = State->get(); + if (!LastSetuidSym) + return State; + + // 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(); + if (!FailComparison) + return State; + 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(Irrelevant); + State = State->set(SymbolRef{}); + } + return State; +} + +void SetgidSetuidOrderChecker::processSetuid(ProgramStateRef State, + const CallEvent &Call, + CheckerContext &C) const { + bool IsSetuidWithGetuid = isFunctionCalledInArg(GetuidDesc, Call); + if (State->get() != Setgid && IsSetuidWithGetuid) { + SymbolRef RetSym = Call.getReturnValue().getAsSymbol(); + State = State->set(Setuid); + State = State->set(RetSym); + const NoteTag *Note = C.getNoteTag([this, + RetSym](PathSensitiveBugReport &BR) { + if (!BR.isInteresting(RetSym) || &BR.getBugType() != &this->BT) + return ""; + return "Call to 'setuid' found here that removes superuser privileges"; + }); + C.addTransition(State, Note); + return; + } + State = State->set(Irrelevant); + State = State->set(SymbolRef{}); + C.addTransition(State); +} + +void SetgidSetuidOrderChecker::processSetgid(ProgramStateRef State, + const CallEvent &Call, + CheckerContext &C) const { + bool IsSetgidWithGetgid = isFunctionCalledInArg(GetgidDesc, Call); + if (State->get() == Setuid) { + if (IsSetgidWithGetgid) { + State = State->set(Irrelevant); + emitReport(State, C); + return; + } + State = State->set(Irrelevant); + } else { + State = State->set(IsSetgidWithGetgid ? Setgid + : Irrelevant); + } + State = State->set(SymbolRef{}); + C.addTransition(State); +} + +void SetgidSetuidOrderChecker::processOther(ProgramStateRef State, + const CallEvent &Call, + CheckerContext &C) const { + State = State->set(SymbolRef{}); + State = State->set(Irrelevant); + C.addTransition(State); +} + +bool SetgidSetuidOrderChecker::isFunctionCalledInArg( + const CallDescription &Desc, const CallEvent &Call) const { + if (const auto *CallInArg0 = + dyn_cast(Call.getArgExpr(0)->IgnoreParenImpCasts())) + return Desc.matchesAsWritten(*CallInArg0); + return false; +} + +void SetgidSetuidOrderChecker::emitReport(ProgramStateRef State, + CheckerContext &C) const { + if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { + llvm::StringLiteral Msg = + "A 'setgid(getgid())' call following a 'setuid(getuid())' " + "call is likely to fail; probably the order of these " + "statements is wrong"; + auto Report = std::make_unique(BT, Msg, N); + Report->markInteresting(State->get()); + C.emitReport(std::move(Report)); + } +} + +void ento::registerSetgidSetuidOrderChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} + +bool ento::shouldRegisterSetgidSetuidOrderChecker(const CheckerManager &mgr) { + return true; +} diff --git a/clang/test/Analysis/setgid-setuid-order-notes.c b/clang/test/Analysis/setgid-setuid-order-notes.c new file mode 100644 index 000000000000000..03402413581c640 --- /dev/null +++ b/clang/test/Analysis/setgid-setuid-order-notes.c @@ -0,0 +1,73 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder -analyzer-output=text -verify %s + +typedef int uid_t; +typedef int gid_t; + +int setuid(uid_t); +int setgid(gid_t); + +uid_t getuid(); +gid_t getgid(); + + + +void test_note_1() { + if (setuid(getuid()) == -1) // expected-note{{Assuming the condition is false}} \ + // expected-note{{Taking false branch}} + return; + if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \ + // expected-note{{Assuming the condition is false}} \ + // expected-note{{Taking false branch}} + return; + if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \ + // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} + return; +} + +void test_note_2() { + if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \ + // expected-note 2 {{Assuming the condition is false}} \ + // expected-note 2 {{Taking false branch}} + return; + if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \ + // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \ + // expected-note{{Assuming the condition is false}} \ + // expected-note{{Taking false branch}} + return; + if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \ + // expected-note{{Assuming the condition is false}} \ + // expected-note{{Taking false branch}} + return; + if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \ + // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} + return; +} + +int f_setuid() { + return setuid(getuid()); // expected-note{{Call to 'setuid' found here that removes superuser privileges}} +} + +int f_setgid() { + return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \ + // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} +} + +void test_note_3() { + if (f_setuid() == -1) // expected-note{{Assuming the condition is false}} \ + // expected-note{{Calling 'f_setuid'}} \ + // expected-note{{Returning from 'f_setuid'}} \ + // expected-note{{Taking false branch}} + return; + if (f_setgid() == -1) // expected-note{{Calling 'f_setgid'}} + return; +} + +void test_note_4() { + if (setuid(getuid()) == 0) { // expected-note{{Assuming the condition is true}} \ + // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \ + // expected-note{{Taking true branch}} + if (setgid(getgid()) == 0) { // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \ + // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} + } + } +} diff --git a/clang/test/Analysis/setgid-setuid-order.c b/clang/test/Analysis/setgid-setuid-order.c new file mode 100644 index 000000000000000..1c411aa6a27b5a3 --- /dev/null +++ b/clang/test/Analysis/setgid-setuid-order.c @@ -0,0 +1,257 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder -verify %s + +typedef int uid_t; +typedef int gid_t; + +int setuid(uid_t); +int setgid(gid_t); +int seteuid(uid_t); +int setegid(gid_t); +int setreuid(uid_t, uid_t); +int setregid(gid_t, gid_t); +int setresuid(uid_t, uid_t, uid_t); +int setresgid(gid_t, gid_t, gid_t); + +uid_t getuid(); +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; + if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} + return; + if (setgid(getgid()) == -1) + return; +} + +void warn_at_second_time() { + 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; + 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; +} + +uid_t f_uid(); +gid_t f_gid(); + +void setuid_other() { + if (setuid(f_uid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setgid_other() { + if (setuid(getuid()) == -1) + return; + if (setgid(f_gid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setuid_other_between() { + if (setuid(getuid()) == -1) + return; + if (setuid(f_uid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +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) + return; +} + +int f_setuid() { + return setuid(getuid()); +} + +int f_setgid() { + return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} +} + +void function_calls() { + if (f_setuid() == -1) + return; + if (f_setgid() == -1) + return; +} + +void seteuid_between() { + if (setuid(getuid()) == -1) + return; + if (seteuid(getuid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setegid_between() { + if (setuid(getuid()) == -1) + return; + if (setegid(getgid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setreuid_between() { + if (setuid(getuid()) == -1) + return; + if (setreuid(getuid(), getuid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setregid_between() { + if (setuid(getuid()) == -1) + return; + if (setregid(getgid(), getgid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setresuid_between() { + if (setuid(getuid()) == -1) + return; + if (setresuid(getuid(), getuid(), getuid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void setresgid_between() { + if (setuid(getuid()) == -1) + return; + if (setresgid(getgid(), getgid(), getgid()) == -1) + return; + if (setgid(getgid()) == -1) + return; +} + +void getgid_getuid_between() { + if (setuid(getuid()) == -1) + return; + (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() { + if (setuid(getuid()) == -1) + return; + f_extern(); + if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} + return; +} + +void setuid_error_case() { + if (setuid(getuid()) == -1) { + // No warning if we know that the first setuid call has failed. + (void)setgid(getgid()); + return; + } + (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}} +}