diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -471,6 +471,16 @@ def CStringOutOfBounds : Checker<"OutOfBounds">, HelpText<"Check for out-of-bounds access in string functions">, Dependencies<[CStringModeling]>, + CheckerOptions<[ + CmdLineOption + ]>, Documentation; def CStringBufferOverlap : Checker<"BufferOverlap">, @@ -517,7 +527,14 @@ "NoStoreFuncVisitor.", "true", Released, - Hide> + Hide>, + CmdLineOption ]>, Dependencies<[CStringModeling]>, Documentation, diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -36,6 +36,7 @@ enum OOB_Kind { OOB_Precedes, OOB_Excedes, OOB_Tainted }; void reportOOB(CheckerContext &C, ProgramStateRef errorState, OOB_Kind kind, + SVal InterestingValue = UndefinedVal(), std::unique_ptr Visitor = nullptr) const; public: @@ -205,10 +206,10 @@ // If we are under constrained and the index variables are tainted, report. if (state_exceedsUpperBound && state_withinUpperBound) { - SVal ByteOffset = rawOffset.getByteOffset(); - if (isTainted(state, ByteOffset)) { + if (isTainted(state, *upperboundToCheck)) { reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted, - std::make_unique(ByteOffset)); + *upperboundToCheck, + std::make_unique(*upperboundToCheck)); return; } } else if (state_exceedsUpperBound) { @@ -229,7 +230,7 @@ void ArrayBoundCheckerV2::reportOOB( CheckerContext &checkerContext, ProgramStateRef errorState, OOB_Kind kind, - std::unique_ptr Visitor) const { + SVal InterestingValue, std::unique_ptr Visitor) const { ExplodedNode *errorNode = checkerContext.generateErrorNode(errorState); if (!errorNode) @@ -257,6 +258,7 @@ } auto BR = std::make_unique(*BT, os.str(), errorNode); + BR->markInteresting(InterestingValue); BR->addVisitor(std::move(Visitor)); checkerContext.emitReport(std::move(BR)); } diff --git a/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp @@ -12,6 +12,7 @@ //===----------------------------------------------------------------------===// #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Checkers/Taint.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -23,20 +24,24 @@ namespace { class BoolAssignmentChecker : public Checker< check::Bind > { mutable std::unique_ptr BT; - void emitReport(ProgramStateRef state, CheckerContext &C) const; + void emitReport(ProgramStateRef state, CheckerContext &C, + bool IsTainted = false) const; + public: void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const; }; } // end anonymous namespace -void BoolAssignmentChecker::emitReport(ProgramStateRef state, - CheckerContext &C) const { +void BoolAssignmentChecker::emitReport(ProgramStateRef state, CheckerContext &C, + bool IsTainted) const { if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) { if (!BT) BT.reset(new BuiltinBug(this, "Assignment of a non-Boolean value")); - C.emitReport( - std::make_unique(*BT, BT->getDescription(), N)); + // FIXME: Reword the message. + StringRef Msg = IsTainted ? "Might assigns a tainted non-Boolean value" + : "Assignment of a non-Boolean value"; + C.emitReport(std::make_unique(*BT, Msg, N)); } } @@ -90,6 +95,8 @@ if (!StIn) emitReport(StOut, C); + if (StIn && StOut && taint::isTainted(state, *NV)) + emitReport(StOut, C, /*IsTainted=*/true); } void ento::registerBoolAssignmentChecker(CheckerManager &mgr) { diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -14,6 +14,7 @@ #include "InterCheckerAPI.h" #include "clang/Basic/CharInfo.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Checkers/Taint.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -55,7 +56,8 @@ enum class AccessKind { write, read }; static ErrorMessage createOutOfBoundErrorMsg(StringRef FunctionDescription, - AccessKind Access) { + AccessKind Access, + bool IsTainted = false) { ErrorMessage Message; llvm::raw_svector_ostream Os(Message); @@ -64,9 +66,9 @@ << &FunctionDescription.data()[1]; if (Access == AccessKind::write) { - Os << " overflows the destination buffer"; + Os << (IsTainted ? " might" : "") << " overflows the destination buffer"; } else { // read access - Os << " accesses out-of-bound array element"; + Os << (IsTainted ? " might" : "") << " accesses out-of-bound array element"; } return Message; @@ -94,6 +96,9 @@ bool CheckCStringNotNullTerm = false; bool CheckCStringUninitializedRead = false; + // Taint makes no sense for NullArg, BufferOverlap and NotNullTerm checks. + bool ConsiderTaintForCStringOutOfBounds = false; + CheckerNameRef CheckNameCStringNullArg; CheckerNameRef CheckNameCStringOutOfBounds; CheckerNameRef CheckNameCStringBufferOverlap; @@ -369,6 +374,15 @@ createOutOfBoundErrorMsg(CurrentFunctionDescription, Access); emitOutOfBoundsBug(C, StOutBound, Buffer.Expression, Message); return nullptr; + } else if (Filter.CheckCStringOutOfBounds && + Filter.ConsiderTaintForCStringOutOfBounds && StOutBound && + StInBound && + (taint::isTainted(state, Idx) || taint::isTainted(state, Size))) { + // FIXME: Explain whether the index or the extent was tainted. + // FIXME: Add a visitor explaining where the taint was introduced. + ErrorMessage Message = createOutOfBoundErrorMsg(CurrentFunctionDescription, + Access, /*IsTainted=*/true); + emitOutOfBoundsBug(C, StOutBound, Buffer.Expression, Message); } // Ensure that we wouldn't read uninitialized value. @@ -2489,8 +2503,20 @@ \ bool ento::shouldRegister##name(const CheckerManager &mgr) { return true; } +#define REGISTER_CHECKER_WITH_TAINT(name) \ + void ento::register##name(CheckerManager &mgr) { \ + CStringChecker *checker = mgr.getChecker(); \ + checker->Filter.Check##name = true; \ + checker->Filter.CheckName##name = mgr.getCurrentCheckerName(); \ + checker->Filter.ConsiderTaintFor##name = \ + mgr.getAnalyzerOptions().getCheckerBooleanOption( \ + mgr.getCurrentCheckerName().getName(), "ConsiderTaint"); \ + } \ + \ + bool ento::shouldRegister##name(const CheckerManager &mgr) { return true; } + REGISTER_CHECKER(CStringNullArg) -REGISTER_CHECKER(CStringOutOfBounds) REGISTER_CHECKER(CStringBufferOverlap) REGISTER_CHECKER(CStringNotNullTerm) REGISTER_CHECKER(CStringUninitializedRead) +REGISTER_CHECKER_WITH_TAINT(CStringOutOfBounds) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -51,6 +51,7 @@ #include "clang/AST/DeclTemplate.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/OperationKinds.h" #include "clang/AST/ParentMap.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -60,6 +61,7 @@ #include "clang/Basic/TargetInfo.h" #include "clang/Lex/Lexer.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Checkers/Taint.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -214,16 +216,24 @@ REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, SymbolRef, RefState) +namespace { +struct StateAndPred { + ProgramStateRef State; + ExplodedNode *Pred; +}; +} // namespace + /// Check if the memory associated with this symbol was released. static bool isReleased(SymbolRef Sym, CheckerContext &C); /// Update the RefState to reflect the new memory allocation. /// The optional \p RetVal parameter specifies the newly allocated pointer /// value; if unspecified, the value of expression \p E is used. -static ProgramStateRef MallocUpdateRefState(CheckerContext &C, const Expr *E, - ProgramStateRef State, - AllocationFamily Family, - Optional RetVal = None); +static StateAndPred MallocUpdateRefState(CheckerContext &C, const Expr *E, + ProgramStateRef State, + ExplodedNode *Pred, + AllocationFamily Family, + Optional RetVal = None); //===----------------------------------------------------------------------===// // The modeling of memory reallocation. @@ -310,6 +320,11 @@ bool ShouldRegisterNoOwnershipChangeVisitor = false; + /// When enabled, allocating with a tainted size going to emit a bugreport if + /// we can not prove that the size is less then or equal to UINT32MAX/2. + /// This heuristic is similar to rsize_t. + bool ShouldCheckTaintedAllocation = false; + /// Many checkers are essentially built into this one, so enabling them will /// make MallocChecker perform additional modeling and reporting. enum CheckKind { @@ -364,6 +379,7 @@ mutable std::unique_ptr BT_MismatchedDealloc; mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds]; mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds]; + mutable std::unique_ptr BT_TaintedAllocation; #define CHECK_FN(NAME) \ void NAME(const CallEvent &Call, CheckerContext &C) const; @@ -455,9 +471,9 @@ /// Process C++ operator new()'s allocation, which is the part of C++ /// new-expression that goes before the constructor. LLVM_NODISCARD - ProgramStateRef processNewAllocation(const CXXAllocatorCall &Call, - CheckerContext &C, - AllocationFamily Family) const; + StateAndPred processNewAllocation(const CXXAllocatorCall &Call, + CheckerContext &C, + AllocationFamily Family) const; /// Perform a zero-allocation check. /// @@ -490,9 +506,10 @@ /// \param [in] State The \c ProgramState right before allocation. /// \returns The ProgramState right after allocation. LLVM_NODISCARD - ProgramStateRef MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call, - const OwnershipAttr *Att, - ProgramStateRef State) const; + StateAndPred MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call, + const OwnershipAttr *Att, + ProgramStateRef State, + ExplodedNode *Pred) const; /// Models memory allocation. /// @@ -504,10 +521,10 @@ /// \param [in] State The \c ProgramState right before allocation. /// \returns The ProgramState right after allocation. LLVM_NODISCARD - static ProgramStateRef MallocMemAux(CheckerContext &C, const CallEvent &Call, - const Expr *SizeEx, SVal Init, - ProgramStateRef State, - AllocationFamily Family); + StateAndPred MallocMemAux(CheckerContext &C, const CallEvent &Call, + const Expr *SizeEx, SVal Init, + ProgramStateRef State, ExplodedNode *Pred, + AllocationFamily Family) const; /// Models memory allocation. /// @@ -519,17 +536,17 @@ /// \param [in] State The \c ProgramState right before allocation. /// \returns The ProgramState right after allocation. LLVM_NODISCARD - static ProgramStateRef MallocMemAux(CheckerContext &C, const CallEvent &Call, - SVal Size, SVal Init, - ProgramStateRef State, - AllocationFamily Family); + StateAndPred MallocMemAux(CheckerContext &C, const CallEvent &Call, SVal Size, + SVal Init, ProgramStateRef State, + ExplodedNode *Pred, AllocationFamily Family) const; // Check if this malloc() for special flags. At present that means M_ZERO or // __GFP_ZERO (in which case, treat it like calloc). LLVM_NODISCARD - llvm::Optional - performKernelMalloc(const CallEvent &Call, CheckerContext &C, - const ProgramStateRef &State) const; + llvm::Optional performKernelMalloc(const CallEvent &Call, + CheckerContext &C, + ProgramStateRef State, + ExplodedNode *Pred) const; /// Model functions with the ownership_takes and ownership_holds attributes. /// @@ -619,10 +636,10 @@ /// has an '_n' suffix, such as g_realloc_n. /// \returns The ProgramState right after reallocation. LLVM_NODISCARD - ProgramStateRef ReallocMemAux(CheckerContext &C, const CallEvent &Call, - bool ShouldFreeOnFail, ProgramStateRef State, - AllocationFamily Family, - bool SuffixWithN = false) const; + StateAndPred ReallocMemAux(CheckerContext &C, const CallEvent &Call, + bool ShouldFreeOnFail, ProgramStateRef State, + ExplodedNode *Pred, AllocationFamily Family, + bool SuffixWithN = false) const; /// Evaluates the buffer size that needs to be allocated. /// @@ -639,8 +656,8 @@ /// \param [in] State The \c ProgramState right before reallocation. /// \returns The ProgramState right after allocation. LLVM_NODISCARD - static ProgramStateRef CallocMem(CheckerContext &C, const CallEvent &Call, - ProgramStateRef State); + StateAndPred CallocMem(CheckerContext &C, const CallEvent &Call, + ProgramStateRef State, ExplodedNode *Pred) const; /// See if deallocation happens in a suspicious context. If so, escape the /// pointers that otherwise would have been deallocated and return true. @@ -1138,9 +1155,10 @@ return Func && Func->hasAttr(); } -llvm::Optional +llvm::Optional MallocChecker::performKernelMalloc(const CallEvent &Call, CheckerContext &C, - const ProgramStateRef &State) const { + ProgramStateRef State, + ExplodedNode *Pred) const { // 3-argument malloc(), as commonly used in {Free,Net,Open}BSD Kernels: // // void *malloc(unsigned long size, struct malloc_type *mtp, int flags); @@ -1211,7 +1229,7 @@ // If M_ZERO is set, treat this like calloc (initialized). if (TrueState && !FalseState) { SVal ZeroVal = C.getSValBuilder().makeZeroVal(Ctx.CharTy); - return MallocMemAux(C, Call, Call.getArgExpr(0), ZeroVal, TrueState, + return MallocMemAux(C, Call, Call.getArgExpr(0), ZeroVal, TrueState, Pred, AF_Malloc); } @@ -1231,24 +1249,24 @@ void MallocChecker::checkBasicAlloc(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State, - AF_Malloc); - State = ProcessZeroAllocCheck(Call, 0, State); - C.addTransition(State); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; + Pair = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), Pair.State, + Pair.Pred, AF_Malloc); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkKernelMalloc(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - llvm::Optional MaybeState = - performKernelMalloc(Call, C, State); - if (MaybeState.hasValue()) - State = MaybeState.getValue(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; + llvm::Optional MaybePair = + performKernelMalloc(Call, C, Pair.State, Pair.Pred); + if (MaybePair.hasValue()) + Pair = MaybePair.getValue(); else - State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State, - AF_Malloc); - C.addTransition(State); + Pair = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), Pair.State, + Pair.Pred, AF_Malloc); + C.addTransition(Pair.State, Pair.Pred); } static bool isStandardRealloc(const CallEvent &Call) { @@ -1289,19 +1307,18 @@ // https://bugs.llvm.org/show_bug.cgi?id=46253 if (!isStandardRealloc(Call) && !isGRealloc(Call)) return; - ProgramStateRef State = C.getState(); - State = ReallocMemAux(C, Call, ShouldFreeOnFail, State, AF_Malloc); - State = ProcessZeroAllocCheck(Call, 1, State); - C.addTransition(State); + StateAndPred Pair = ReallocMemAux(C, Call, ShouldFreeOnFail, C.getState(), + C.getPredecessor(), AF_Malloc); + Pair.State = ProcessZeroAllocCheck(Call, 1, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkCalloc(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - State = CallocMem(C, Call, State); - State = ProcessZeroAllocCheck(Call, 0, State); - State = ProcessZeroAllocCheck(Call, 1, State); - C.addTransition(State); + StateAndPred Pair = CallocMem(C, Call, C.getState(), C.getPredecessor()); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); + Pair.State = ProcessZeroAllocCheck(Call, 1, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkFree(const CallEvent &Call, CheckerContext &C) const { @@ -1316,33 +1333,32 @@ void MallocChecker::checkAlloca(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State, - AF_Alloca); - State = ProcessZeroAllocCheck(Call, 0, State); - C.addTransition(State); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; + Pair = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), Pair.State, + Pair.Pred, AF_Alloca); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkStrdup(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); const auto *CE = dyn_cast_or_null(Call.getOriginExpr()); if (!CE) return; - State = MallocUpdateRefState(C, CE, State, AF_Malloc); - C.addTransition(State); + StateAndPred Pair = + MallocUpdateRefState(C, CE, C.getState(), C.getPredecessor(), AF_Malloc); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkIfNameIndex(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; // Should we model this differently? We can allocate a fixed number of // elements with zeros in the last one. - State = - MallocMemAux(C, Call, UnknownVal(), UnknownVal(), State, AF_IfNameIndex); - - C.addTransition(State); + Pair = MallocMemAux(C, Call, UnknownVal(), UnknownVal(), Pair.State, + Pair.Pred, AF_IfNameIndex); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkIfFreeNameIndex(const CallEvent &Call, @@ -1356,7 +1372,7 @@ void MallocChecker::checkCXXNewOrCXXDelete(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; bool IsKnownToBeAllocatedMemory = false; const auto *CE = dyn_cast_or_null(Call.getOriginExpr()); if (!CE) @@ -1371,85 +1387,88 @@ const FunctionDecl *FD = C.getCalleeDecl(CE); switch (FD->getOverloadedOperator()) { case OO_New: - State = - MallocMemAux(C, Call, CE->getArg(0), UndefinedVal(), State, AF_CXXNew); - State = ProcessZeroAllocCheck(Call, 0, State); + Pair = MallocMemAux(C, Call, CE->getArg(0), UndefinedVal(), Pair.State, + Pair.Pred, AF_CXXNew); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); break; case OO_Array_New: - State = MallocMemAux(C, Call, CE->getArg(0), UndefinedVal(), State, - AF_CXXNewArray); - State = ProcessZeroAllocCheck(Call, 0, State); + Pair = MallocMemAux(C, Call, CE->getArg(0), UndefinedVal(), Pair.State, + Pair.Pred, AF_CXXNewArray); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); break; case OO_Delete: - State = FreeMemAux(C, Call, State, 0, false, IsKnownToBeAllocatedMemory, - AF_CXXNew); + Pair.State = FreeMemAux(C, Call, Pair.State, 0, false, + IsKnownToBeAllocatedMemory, AF_CXXNew); break; case OO_Array_Delete: - State = FreeMemAux(C, Call, State, 0, false, IsKnownToBeAllocatedMemory, - AF_CXXNewArray); + Pair.State = FreeMemAux(C, Call, Pair.State, 0, false, + IsKnownToBeAllocatedMemory, AF_CXXNewArray); break; default: llvm_unreachable("not a new/delete operator"); } - C.addTransition(State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkGMalloc0(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; SValBuilder &svalBuilder = C.getSValBuilder(); SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy); - State = MallocMemAux(C, Call, Call.getArgExpr(0), zeroVal, State, AF_Malloc); - State = ProcessZeroAllocCheck(Call, 0, State); - C.addTransition(State); + Pair = MallocMemAux(C, Call, Call.getArgExpr(0), zeroVal, Pair.State, + Pair.Pred, AF_Malloc); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkGMemdup(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - State = - MallocMemAux(C, Call, Call.getArgExpr(1), UnknownVal(), State, AF_Malloc); - State = ProcessZeroAllocCheck(Call, 1, State); - C.addTransition(State); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; + Pair = MallocMemAux(C, Call, Call.getArgExpr(1), UnknownVal(), Pair.State, + Pair.Pred, AF_Malloc); + Pair.State = ProcessZeroAllocCheck(Call, 1, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkGMallocN(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; SVal Init = UndefinedVal(); SVal TotalSize = evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1)); - State = MallocMemAux(C, Call, TotalSize, Init, State, AF_Malloc); - State = ProcessZeroAllocCheck(Call, 0, State); - State = ProcessZeroAllocCheck(Call, 1, State); - C.addTransition(State); + Pair = + MallocMemAux(C, Call, TotalSize, Init, Pair.State, Pair.Pred, AF_Malloc); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); + Pair.State = ProcessZeroAllocCheck(Call, 1, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkGMallocN0(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; SValBuilder &SB = C.getSValBuilder(); SVal Init = SB.makeZeroVal(SB.getContext().CharTy); SVal TotalSize = evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1)); - State = MallocMemAux(C, Call, TotalSize, Init, State, AF_Malloc); - State = ProcessZeroAllocCheck(Call, 0, State); - State = ProcessZeroAllocCheck(Call, 1, State); - C.addTransition(State); + Pair = + MallocMemAux(C, Call, TotalSize, Init, Pair.State, Pair.Pred, AF_Malloc); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State); + Pair.State = ProcessZeroAllocCheck(Call, 1, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkReallocN(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - State = ReallocMemAux(C, Call, /*ShouldFreeOnFail=*/false, State, AF_Malloc, - /*SuffixWithN=*/true); - State = ProcessZeroAllocCheck(Call, 1, State); - State = ProcessZeroAllocCheck(Call, 2, State); - C.addTransition(State); + StateAndPred Pair = ReallocMemAux(C, Call, /*ShouldFreeOnFail=*/false, + C.getState(), C.getPredecessor(), AF_Malloc, + /*SuffixWithN=*/true); + Pair.State = ProcessZeroAllocCheck(Call, 1, Pair.State); + Pair.State = ProcessZeroAllocCheck(Call, 2, Pair.State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkOwnershipAttr(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; const auto *CE = dyn_cast_or_null(Call.getOriginExpr()); if (!CE) return; @@ -1464,16 +1483,16 @@ for (const auto *I : FD->specific_attrs()) { switch (I->getOwnKind()) { case OwnershipAttr::Returns: - State = MallocMemReturnsAttr(C, Call, I, State); + Pair = MallocMemReturnsAttr(C, Call, I, Pair.State, Pair.Pred); break; case OwnershipAttr::Takes: case OwnershipAttr::Holds: - State = FreeMemAttr(C, Call, I, State); + Pair.State = FreeMemAttr(C, Call, I, Pair.State); break; } } } - C.addTransition(State); + C.addTransition(Pair.State, Pair.Pred); } void MallocChecker::checkPostCall(const CallEvent &Call, @@ -1613,41 +1632,42 @@ return false; } -ProgramStateRef +StateAndPred MallocChecker::processNewAllocation(const CXXAllocatorCall &Call, CheckerContext &C, AllocationFamily Family) const { if (!isStandardNewDelete(Call)) - return nullptr; + return {nullptr, C.getPredecessor()}; const CXXNewExpr *NE = Call.getOriginExpr(); const ParentMap &PM = C.getLocationContext()->getParentMap(); - ProgramStateRef State = C.getState(); + StateAndPred Pair = {C.getState(), C.getPredecessor()}; // Non-trivial constructors have a chance to escape 'this', but marking all // invocations of trivial constructors as escaped would cause too great of // reduction of true positives, so let's just do that for constructors that // have an argument of a pointer-to-record type. if (!PM.isConsumedExpr(NE) && hasNonTrivialConstructorCall(NE)) - return State; + return Pair; // The return value from operator new is bound to a specified initialization // value (if any) and we don't want to loose this value. So we call // MallocUpdateRefState() instead of MallocMemAux() which breaks the // existing binding. SVal Target = Call.getObjectUnderConstruction(); - State = MallocUpdateRefState(C, NE, State, Family, Target); - State = ProcessZeroAllocCheck(Call, 0, State, Target); - return State; + + Pair = MallocUpdateRefState(C, NE, Pair.State, Pair.Pred, Family, Target); + Pair.State = ProcessZeroAllocCheck(Call, 0, Pair.State, Target); + return Pair; } void MallocChecker::checkNewAllocator(const CXXAllocatorCall &Call, CheckerContext &C) const { if (!C.wasInlined) { - ProgramStateRef State = processNewAllocation( + StateAndPred Pair = processNewAllocation( Call, C, (Call.getOriginExpr()->isArray() ? AF_CXXNewArray : AF_CXXNew)); - C.addTransition(State); + C.addTransition(Pair.State, Pair.Pred); } } @@ -1698,48 +1718,49 @@ C.addTransition(State); } -ProgramStateRef -MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call, - const OwnershipAttr *Att, - ProgramStateRef State) const { +StateAndPred MallocChecker::MallocMemReturnsAttr(CheckerContext &C, + const CallEvent &Call, + const OwnershipAttr *Att, + ProgramStateRef State, + ExplodedNode *Pred) const { if (!State) - return nullptr; + return {nullptr, Pred}; if (Att->getModule()->getName() != "malloc") - return nullptr; + return {nullptr, Pred}; OwnershipAttr::args_iterator I = Att->args_begin(), E = Att->args_end(); if (I != E) { return MallocMemAux(C, Call, Call.getArgExpr(I->getASTIndex()), - UndefinedVal(), State, AF_Malloc); + UndefinedVal(), State, Pred, AF_Malloc); } - return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, AF_Malloc); + return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, Pred, + AF_Malloc); } -ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, - const CallEvent &Call, - const Expr *SizeEx, SVal Init, - ProgramStateRef State, - AllocationFamily Family) { +StateAndPred MallocChecker::MallocMemAux( + CheckerContext &C, const CallEvent &Call, const Expr *SizeEx, SVal Init, + ProgramStateRef State, ExplodedNode *Pred, AllocationFamily Family) const { if (!State) - return nullptr; + return {nullptr, Pred}; assert(SizeEx); - return MallocMemAux(C, Call, C.getSVal(SizeEx), Init, State, Family); + return MallocMemAux(C, Call, C.getSVal(SizeEx), Init, State, Pred, Family); } -ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, - const CallEvent &Call, SVal Size, - SVal Init, ProgramStateRef State, - AllocationFamily Family) { +StateAndPred MallocChecker::MallocMemAux(CheckerContext &C, + const CallEvent &Call, SVal Size, + SVal Init, ProgramStateRef State, + ExplodedNode *Pred, + AllocationFamily Family) const { if (!State) - return nullptr; + return {nullptr, Pred}; const Expr *CE = Call.getOriginExpr(); // We expect the malloc functions to return a pointer. if (!Loc::isLocType(CE->getType())) - return nullptr; + return {nullptr, Pred}; // Bind the return value to the symbolic value from the heap region. // TODO: We could rewrite post visit to eval call; 'malloc' does not have @@ -1757,16 +1778,67 @@ // Set the region's extent. State = setDynamicExtent(State, RetVal.getAsRegion(), Size.castAs(), svalBuilder); + if (taint::isTainted(State, Size)) { + const NoteTag *Note = + C.getNoteTag([Size](PathSensitiveBugReport &BR) -> std::string { + if (BR.isInteresting(Size)) + return "Allocating tainted amount of memory"; + return ""; + }); + Pred = C.addTransition(State, Pred, Note); + } + + // If we allocate tainted and unconstrained amount of memory. + if (ShouldCheckTaintedAllocation && taint::isTainted(State, Size)) { + class clang::ASTContext &Ctx = C.getASTContext(); + QualType Ty = Ctx.UnsignedIntTy; + + const uint64_t TyBits = Ctx.getTypeSize(Ty); + assert(TyBits > 0); + assert(TyBits <= 64); + auto val = ((uint64_t)1) << (TyBits - 1); // 0b1000...0 + NonLoc ReasonableUpperbound = + svalBuilder.makeIntVal(val, Ty).castAs(); + + SVal Check = svalBuilder.evalBinOpNN( + State, clang::BO_GE, Size.castAs(), ReasonableUpperbound, + svalBuilder.getConditionType()); + + ProgramStateRef St1, St2; + std::tie(St1, St2) = State->assume(Check.castAs()); + if ((St1 && !St2) || (St1 && St2)) { + ExplodedNode *N = C.generateErrorNode(State); + if (!N) + return {nullptr, Pred}; + + if (!BT_TaintedAllocation) + BT_TaintedAllocation.reset(new BugType(CheckNames[CK_MallocChecker], + "Tainted allocation", + categories::MemoryError)); + + // Generate a report for this bug. + auto Report = std::make_unique( + *BT_TaintedAllocation, + "Allocating tainted amount of memory without a reasonable " + "upperbound.", + N); + Report->addRange(Call.getSourceRange()); + Report->addVisitor(std::make_unique(Size)); + + C.emitReport(std::move(Report)); + } + } - return MallocUpdateRefState(C, CE, State, Family); + return MallocUpdateRefState(C, CE, State, Pred, Family); } -static ProgramStateRef MallocUpdateRefState(CheckerContext &C, const Expr *E, - ProgramStateRef State, - AllocationFamily Family, - Optional RetVal) { +static StateAndPred MallocUpdateRefState(CheckerContext &C, const Expr *E, + ProgramStateRef State, + ExplodedNode *Pred, + AllocationFamily Family, + Optional RetVal) { if (!State) - return nullptr; + return {nullptr, Pred}; // Get the return value. if (!RetVal) @@ -1774,7 +1846,7 @@ // We expect the malloc functions to return a pointer. if (!RetVal->getAs()) - return nullptr; + return {nullptr, Pred}; SymbolRef Sym = RetVal->getAsLocSymbol(); // This is a return value of a function that was not inlined, such as malloc() @@ -1782,7 +1854,8 @@ assert(Sym); // Set the symbol's state to Allocated. - return State->set(Sym, RefState::getAllocated(Family, E)); + State = State->set(Sym, RefState::getAllocated(Family, E)); + return {State, Pred}; } ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C, @@ -2571,24 +2644,25 @@ } } -ProgramStateRef +StateAndPred MallocChecker::ReallocMemAux(CheckerContext &C, const CallEvent &Call, bool ShouldFreeOnFail, ProgramStateRef State, - AllocationFamily Family, bool SuffixWithN) const { + ExplodedNode *Pred, AllocationFamily Family, + bool SuffixWithN) const { if (!State) - return nullptr; + return {nullptr, Pred}; const CallExpr *CE = cast(Call.getOriginExpr()); if (SuffixWithN && CE->getNumArgs() < 3) - return nullptr; + return {nullptr, Pred}; else if (CE->getNumArgs() < 2) - return nullptr; + return {nullptr, Pred}; const Expr *arg0Expr = CE->getArg(0); SVal Arg0Val = C.getSVal(arg0Expr); if (!Arg0Val.getAs()) - return nullptr; + return {nullptr, Pred}; DefinedOrUnknownSVal arg0Val = Arg0Val.castAs(); SValBuilder &svalBuilder = C.getSValBuilder(); @@ -2604,7 +2678,7 @@ if (SuffixWithN) TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2)); if (!TotalSize.getAs()) - return nullptr; + return {nullptr, Pred}; // Compare the size argument to 0. DefinedOrUnknownSVal SizeZero = @@ -2624,13 +2698,12 @@ // If the ptr is NULL and the size is not 0, the call is equivalent to // malloc(size). if (PrtIsNull && !SizeIsZero) { - ProgramStateRef stateMalloc = MallocMemAux( - C, Call, TotalSize, UndefinedVal(), StatePtrIsNull, Family); - return stateMalloc; + return MallocMemAux(C, Call, TotalSize, UndefinedVal(), StatePtrIsNull, + Pred, Family); } if (PrtIsNull && SizeIsZero) - return State; + return {State, Pred}; assert(!PrtIsNull); @@ -2644,16 +2717,16 @@ // any constrains on the output pointer. if (ProgramStateRef stateFree = FreeMemAux( C, Call, StateSizeIsZero, 0, false, IsKnownToBeAllocated, Family)) - return stateFree; + return {stateFree, Pred}; // Default behavior. if (ProgramStateRef stateFree = FreeMemAux(C, Call, State, 0, false, IsKnownToBeAllocated, Family)) { - ProgramStateRef stateRealloc = - MallocMemAux(C, Call, TotalSize, UnknownVal(), stateFree, Family); - if (!stateRealloc) - return nullptr; + StateAndPred ReallocState = + MallocMemAux(C, Call, TotalSize, UnknownVal(), stateFree, Pred, Family); + if (!ReallocState.State) + return ReallocState; OwnershipAfterReallocKind Kind = OAR_ToBeFreedAfterFailure; if (ShouldFreeOnFail) @@ -2671,30 +2744,30 @@ // Record the info about the reallocated symbol so that we could properly // process failed reallocation. - stateRealloc = stateRealloc->set(ToPtr, - ReallocPair(FromPtr, Kind)); + ReallocState.State = ReallocState.State->set( + ToPtr, ReallocPair(FromPtr, Kind)); // The reallocated symbol should stay alive for as long as the new symbol. C.getSymbolManager().addSymbolDependency(ToPtr, FromPtr); - return stateRealloc; + return ReallocState; } - return nullptr; + return {nullptr, Pred}; } -ProgramStateRef MallocChecker::CallocMem(CheckerContext &C, - const CallEvent &Call, - ProgramStateRef State) { +StateAndPred MallocChecker::CallocMem(CheckerContext &C, const CallEvent &Call, + ProgramStateRef State, + ExplodedNode *Pred) const { if (!State) - return nullptr; + return {nullptr, Pred}; if (Call.getNumArgs() < 2) - return nullptr; + return {nullptr, Pred}; SValBuilder &svalBuilder = C.getSValBuilder(); SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy); SVal TotalSize = evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1)); - return MallocMemAux(C, Call, TotalSize, zeroVal, State, AF_Malloc); + return MallocMemAux(C, Call, TotalSize, zeroVal, State, Pred, AF_Malloc); } MallocChecker::LeakInfo MallocChecker::getAllocationSite(const ExplodedNode *N, @@ -3619,6 +3692,9 @@ checker->ShouldRegisterNoOwnershipChangeVisitor = mgr.getAnalyzerOptions().getCheckerBooleanOption( checker, "AddNoOwnershipChangeNotes"); + checker->ShouldCheckTaintedAllocation = + mgr.getAnalyzerOptions().getCheckerBooleanOption( + checker, "CheckTaintedAllocation"); } bool ento::shouldRegisterDynamicMemoryModeling(const CheckerManager &mgr) { diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -41,6 +41,8 @@ //===----------------------------------------------------------------------===// #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Checkers/Taint.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -110,7 +112,15 @@ CheckerContext &C) const = 0; virtual ValueConstraintPtr negate() const { llvm_unreachable("Not implemented"); - }; + } + + virtual void markInteresting(const CallEvent &Call, + PathSensitiveBugReport &BR) const { + BR.markInteresting(getArgSVal(Call, getArgNo())); + } + + virtual bool dependsOnTaintedValue(const CallEvent &Call, + CheckerContext &C) const = 0; // Check whether the constraint is malformed or not. It is malformed if the // specified argument has a mismatch with the given FunctionDecl (e.g. the @@ -212,6 +222,12 @@ return std::make_shared(Tmp); } + bool dependsOnTaintedValue(const CallEvent &Call, + CheckerContext &C) const override { + assert(getArgNo() != Ret); + return taint::isTainted(Call.getState(), Call.getArgSVal(getArgNo())); + } + bool checkSpecificValidity(const FunctionDecl *FD) const override { const bool ValidArg = getArgType(FD, ArgN)->isIntegralType(FD->getASTContext()); @@ -235,6 +251,21 @@ ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary, CheckerContext &C) const override; + + void markInteresting(const CallEvent &Call, + PathSensitiveBugReport &BR) const override { + BR.markInteresting(getArgSVal(Call, getArgNo())); + BR.markInteresting(getArgSVal(Call, getOtherArgNo())); + } + + bool dependsOnTaintedValue(const CallEvent &Call, + CheckerContext &C) const override { + assert(getArgNo() != Ret); + assert(getOtherArgNo() != Ret); + ProgramStateRef State = Call.getState(); + return taint::isTainted(State, Call.getArgSVal(getArgNo())) || + taint::isTainted(State, Call.getArgSVal(getOtherArgNo())); + } }; class NotNullConstraint : public ValueConstraint { @@ -266,6 +297,12 @@ return std::make_shared(Tmp); } + bool dependsOnTaintedValue(const CallEvent &Call, + CheckerContext &C) const override { + assert(getArgNo() != Ret); + return false; + } + bool checkSpecificValidity(const FunctionDecl *FD) const override { const bool ValidArg = getArgType(FD, ArgN)->isPointerType(); assert(ValidArg && @@ -366,6 +403,49 @@ return std::make_shared(Tmp); } + void markInteresting(const CallEvent &Call, + PathSensitiveBugReport &BR) const override { + BR.markInteresting(getArgSVal(Call, getArgNo())); + if (SizeArgN.hasValue()) + BR.markInteresting(getArgSVal(Call, SizeArgN.getValue())); + if (SizeMultiplierArgN.hasValue()) + BR.markInteresting(getArgSVal(Call, SizeMultiplierArgN.getValue())); + } + + bool dependsOnTaintedValue(const CallEvent &Call, + CheckerContext &C) const override { + assert(getArgNo() != Ret); + ProgramStateRef State = Call.getState(); + + const auto IsExtentTainted = [&C, State](SVal BufferVal) -> bool { + const MemRegion *Buffer = BufferVal.getAsRegion(); + if (!Buffer) + return false; + SVal Extent = getDynamicExtent(State, Buffer, C.getSValBuilder()); + return taint::isTainted(State, Extent); + }; + + if (IsExtentTainted(Call.getArgSVal(getArgNo()))) + return true; + + if (SizeArgN.hasValue()) { + assert(SizeArgN.getValue() != Ret); + SVal SizeArgNVal = Call.getArgSVal(SizeArgN.getValue()); + if (taint::isTainted(State, SizeArgNVal)) + return true; + } + + if (SizeMultiplierArgN.hasValue()) { + assert(SizeMultiplierArgN.getValue() != Ret); + SVal SizeMultiplierArgNVal = + Call.getArgSVal(SizeMultiplierArgN.getValue()); + if (taint::isTainted(State, SizeMultiplierArgNVal)) + return true; + } + + return false; + } + bool checkSpecificValidity(const FunctionDecl *FD) const override { const bool ValidArg = getArgType(FD, ArgN)->isPointerType(); assert(ValidArg && @@ -596,17 +676,21 @@ void reportBug(const CallEvent &Call, ExplodedNode *N, const ValueConstraint *VC, const Summary &Summary, - CheckerContext &C) const { + CheckerContext &C, bool IsTainted = false) const { if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker]) return; + std::string Msg = (Twine("Function argument constraint is not satisfied, constraint: ") + - VC->getName().data()) + VC->getName().data() + + (IsTainted ? "; It depends on tainted value" : "")) .str(); + if (!BT_InvalidArg) BT_InvalidArg = std::make_unique( CheckNames[CK_StdCLibraryFunctionArgsChecker], "Unsatisfied argument constraints", categories::LogicError); + auto R = std::make_unique(*BT_InvalidArg, Msg, N); for (ArgNo ArgN : VC->getArgsToTrack()) @@ -619,6 +703,9 @@ R->addNote(VC->describe(C.getState(), Summary), R->getLocation(), Call.getArgSourceRange(VC->getArgNo())); + // FIXME: What if we don't want to mark every part interesting but only a + // smaller portion of it. + VC->markInteresting(Call, *R); C.emitReport(std::move(R)); } }; @@ -836,6 +923,11 @@ if (ExplodedNode *N = C.generateErrorNode(NewState)) reportBug(Call, N, Constraint.get(), Summary, C); break; + } else if (FailureSt && SuccessSt && + Constraint->dependsOnTaintedValue(Call, C)) { + if (ExplodedNode *N = C.generateErrorNode(NewState)) + reportBug(Call, N, Constraint.get(), Summary, C, /*IsTainted=*/true); + break; } else { // We will apply the constraint even if we cannot reason about the // argument. This means both SuccessSt and FailureSt can be true. If we diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2261,6 +2261,12 @@ // How to handle multiple metadata for the same region? if (const auto *meta = dyn_cast(sym)) markInteresting(meta->getRegion(), TKind); + + auto SubSyms = llvm::make_range(sym->symbol_begin(), sym->symbol_end()); + for (SymbolRef SubSym : SubSyms) { + if (SymbolData::classof(SubSym)) + insertToInterestingnessMap(InterestingSymbols, SubSym, TKind); + } } void PathSensitiveBugReport::markNotInteresting(SymbolRef sym) { @@ -2341,10 +2347,25 @@ return None; // We don't currently consider metadata symbols to be interesting // even if we know their region is interesting. Is that correct behavior? - auto It = InterestingSymbols.find(sym); - if (It == InterestingSymbols.end()) - return None; - return It->getSecond(); + auto TryToLookupTrackingKind = + [this](SymbolRef Sym) -> Optional { + auto It = InterestingSymbols.find(Sym); + if (It == InterestingSymbols.end()) + return None; + return It->getSecond(); + }; + + if (auto MaybeTK = TryToLookupTrackingKind(sym)) + return MaybeTK; + + auto SubSyms = llvm::make_range(sym->symbol_begin(), sym->symbol_end()); + for (SymbolRef SubSym : SubSyms) { + if (SymbolData::classof(SubSym)) { + if (auto MaybeTK = TryToLookupTrackingKind(SubSym)) + return MaybeTK; + } + } + return None; } Optional diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -12,6 +12,7 @@ // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01 // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = "" +// CHECK-NEXT: alpha.unix.cstring.OutOfBounds:ConsiderTaint = false // CHECK-NEXT: apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries = false // CHECK-NEXT: apiModeling.StdCLibraryFunctions:ModelPOSIX = false // CHECK-NEXT: apply-fixits = false @@ -123,6 +124,7 @@ // CHECK-NEXT: track-conditions = true // CHECK-NEXT: track-conditions-debug = false // CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = true +// CHECK-NEXT: unix.DynamicMemoryModeling:CheckTaintedAllocation = false // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false // CHECK-NEXT: unroll-loops = false // CHECK-NEXT: verbose-report-filename = false diff --git a/clang/test/Analysis/bool-assignment.c b/clang/test/Analysis/bool-assignment.c --- a/clang/test/Analysis/bool-assignment.c +++ b/clang/test/Analysis/bool-assignment.c @@ -1,5 +1,5 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.BoolAssignment -analyzer-store=region -verify -std=c99 -Dbool=_Bool %s -// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.BoolAssignment -analyzer-store=region -verify -x c++ %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.BoolAssignment,alpha.security.taint -analyzer-store=region -verify -std=c99 -Dbool=_Bool %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.BoolAssignment,alpha.security.taint -analyzer-store=region -verify -x c++ %s // Test C++'s bool and C's _Bool. // FIXME: We stopped warning on these when SValBuilder got smarter about @@ -104,3 +104,10 @@ } x = y; // no-warning } + +int scanf(const char *format, ...); +void test_tainted_Boolean() { + int n; + scanf("%d", &n); + Boolean copy = n; // expected-warning {{Might assigns a tainted non-Boolean value}} +} diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c --- a/clang/test/Analysis/malloc.c +++ b/clang/test/Analysis/malloc.c @@ -1,9 +1,12 @@ // RUN: %clang_analyze_cc1 -Wno-strict-prototypes -Wno-error=implicit-int -analyzer-store=region -verify %s \ +// RUN: -triple x86_64-pc-linux-gnu \ // RUN: -analyzer-checker=core \ // RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \ // RUN: -analyzer-checker=alpha.core.CastSize \ // RUN: -analyzer-checker=unix \ -// RUN: -analyzer-checker=debug.ExprInspection +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-checker=alpha.security.taint \ +// RUN: -analyzer-config unix.DynamicMemoryModeling:CheckTaintedAllocation=true #include "Inputs/system-header-simulator.h" @@ -27,6 +30,7 @@ #endif // !defined(_WCHAR_T_DEFINED) typedef __typeof(sizeof(int)) size_t; +int scanf(const char *format, ...); void *malloc(size_t); void *alloca(size_t); void *valloc(size_t); @@ -1890,9 +1894,88 @@ void testExtent(void) { int x = conjure(); clang_analyzer_dump(x); - // expected-warning-re@-1 {{{{^conj_\$[[:digit:]]+{int, LC1, S[[:digit:]]+, #1}}}}}} + // expected-warning-re@-1 {{{{^conj_\$[[:digit:]]+{int, LC[[:digit:]]+, S[[:digit:]]+, #1}}}}}} int *p = (int *)malloc(x); clang_analyzer_dumpExtent(p); - // expected-warning-re@-1 {{{{^conj_\$[[:digit:]]+{int, LC1, S[[:digit:]]+, #1}}}}}} + // expected-warning-re@-1 {{{{^conj_\$[[:digit:]]+{int, LC[[:digit:]]+, S[[:digit:]]+, #1}}}}}} free(p); } + +void tainted_allocation_char() { + char n; + scanf("%c", &n); + if (n < 0) { + // 'n' might be negative, in which case it wraps around + // Unfortunately, right now the SymIntExpr of `conj{char,n} < 2147483648U` + // is not evaluated according to the C semantics. The promotion is elided. + // THIS IS BAD. + char *p = malloc(n); // FIXME: This should trigger a warning. + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + free(p); + } + + char *p = malloc(n); // FIXME: This should trigger a warning. Same reasoning. + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + free(p); +} + +void tainted_allocation_int() { + int n; + scanf("%d", &n); + if (n < 100) { + if (n > 0) { + char *p = malloc(n); // no-warning + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + free(p); + } + // 'n' might be negative, in which case it wraps around + char *p = malloc(n); // no-warning + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + // expected-warning@-2 {{Allocating tainted amount of memory without a reasonable upperbound}} + free(p); + } + + char *p = malloc(n); + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + // expected-warning@-2 {{Allocating tainted amount of memory without a reasonable upperbound}} + free(p); +} + +void tainted_allocation_size_t_within() { + const size_t UINT32_MAX_PER_2 = 2147483647; // 01111111111111111111111111111111 + size_t n; + scanf("%lu", &n); + + if (n < 100) { + char *p = malloc(n); // no-warning + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + free(p); + } + + // Just within the reasonable range. + if (n <= UINT32_MAX_PER_2) { + char *p = malloc(n); // no-warning + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + free(p); + } + + // No bounds check or whatsoever. + char *p = malloc(n); + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + // expected-warning@-2 {{Allocating tainted amount of memory without a reasonable upperbound}} + free(p); +} + +void tainted_allocation_size_t_outside() { + const size_t UINT32_MAX_PER_2 = 2147483647; // 01111111111111111111111111111111 + size_t n; + scanf("%lu", &n); + + // Just outside the reasonable range. + if (n <= (UINT32_MAX_PER_2 + 1)) { + char *p = malloc(n); + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + // expected-warning@-2 {{Allocating tainted amount of memory without a reasonable upperbound}} + free(p); + } +} diff --git a/clang/test/Analysis/std-c-library-functions-taint.c b/clang/test/Analysis/std-c-library-functions-taint.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/std-c-library-functions-taint.c @@ -0,0 +1,182 @@ +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-output=text \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-checker=alpha.security.taint \ +// RUN: -analyzer-checker=alpha.unix.StdCLibraryFunctionArgs \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \ +// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \ +// RUN: -triple x86_64-unknown-linux -verify + +void clang_analyzer_eval(int); +void clang_analyzer_dump(int); +void clang_analyzer_dump_str(const char *); +void clang_analyzer_isTainted(int); +void clang_analyzer_isTainted_str(const char *); +void clang_analyzer_dumpExtent(void *); +void clang_analyzer_warnIfReached(); + +const unsigned char MAX_CHAR = -1; +const int EOF = -1; + +int scanf(const char *restrict format, ...); +typedef __typeof(sizeof(int)) size_t; +typedef long int ssize_t; +void *malloc(size_t size); +void free(void *ptr); +int toupper(int c); + +// Test if the range constraints are checked for 'toupper'. +void testSanityWithoutTaint(int x) { + (void)toupper(x); // no-warning + (void)toupper(x + 42); // no-warning + (void)toupper(1000); + // expected-warning@-1 {{Function argument constraint is not satisfied, constraint: Range}} + // expected-note@-2 {{Function argument constraint is not satisfied, constraint: Range}} + // expected-note@-3 {{The 1st arg should be within the range [[-1, -1], [0, 255]]}} +} + +// Test if we perfectly constrain a tainted value, then it will become a concrete value. +void testTaintedExactValue() { + int n; + scanf("%d", &n); // no-warning + + if (n == EOF) { + // expected-note@-1 + {{Assuming 'n' is equal to 'EOF'}} + // expected-note@-2 + {{Taking true branch}} + // 'n' becomes concrete, which is no longer tainted. + + clang_analyzer_isTainted(n); // expected-warning {{NO}} expected-note {{NO}} + clang_analyzer_dump(n); // expected-warning {{-1 S32b}} expected-note {{-1 S32b}} + (void)toupper(n); // no-warning, EOF is ok + } +} + +// -----======== Testing RangeConstraint ========----- +// Don't warn if the RangeConstraint is known to be satisfied. +void testTaintedValueIsWithinRange() { + int n; + scanf("%d", &n); + + if (0 <= n && n <= MAX_CHAR) { + // expected-note@-1 + {{Assuming 'n' is >= 0}} + // expected-note@-2 + {{Left side of '&&' is true}} + // expected-note@-3 + {{Assuming 'n' is <= 'MAX_CHAR'}} + // expected-note@-4 + {{Taking true branch}} + + clang_analyzer_isTainted(n); // expected-warning {{YES}} expected-note {{YES}} + clang_analyzer_dump(n); // expected-warning {{conj_$}} expected-note {{conj_$}} + (void)toupper(n); // no-warning, constrained to be safe + } +} + +// Test if the RangeConstraint might not hold, warn for tainted. +void testTaintedValueMightBeOutOfRange() { + int n; + scanf("%d", &n); // expected-note {{Propagated taint to the 2nd parameter}} + + if (0 <= n && n <= MAX_CHAR) { + // expected-note@-1 + {{Assuming 'n' is >= 0}} + // expected-note@-2 + {{Left side of '&&' is true}} + // expected-note@-3 + {{Assuming 'n' is <= 'MAX_CHAR'}} + // expected-note@-4 + {{Taking true branch}} + + clang_analyzer_isTainted(n + 1); // expected-warning {{YES}} expected-note {{YES}} + clang_analyzer_dump(n + 1); // expected-warning {{(conj_$}} expected-note {{(conj_$}} + (void)toupper(n + 1); // 'n+1' might be MAX_CHAR+1, which does not satisfie the precondition of 'toupper' + // expected-warning@-1 {{Function argument constraint is not satisfied, constraint: Range; It depends on tainted value}} + // expected-note@-2 {{Function argument constraint is not satisfied, constraint: Range; It depends on tainted value}} + // expected-note@-3 {{The 1st arg should be within the range [[-1, -1], [0, 255]]}} + } +} + +// -----======== Testing NotNullConstraint ========----- +// It's just a made up example, where we get a tainted pointer. +char *strdup(const char *s); +void testTaintedPointer(const char *fmt, char *buf) { + char *ptr; + scanf(fmt, &ptr); // One does not simply read a pointer - well we do. + clang_analyzer_isTainted_str(ptr); // expected-warning {{YES}} expected-note {{YES}} + clang_analyzer_dump_str(ptr); // expected-warning {{&SymRegion{conj_$}} expected-note {{&SymRegion{conj_$}} + + char *copy = strdup(ptr); // 'ptr' might be null. + + clang_analyzer_isTainted_str(copy); // expected-warning {{YES}} expected-note {{YES}} + clang_analyzer_dump_str(copy); // expected-warning {{&SymRegion{conj_$}} expected-note {{&SymRegion{conj_$}} + + clang_analyzer_eval(ptr != copy); + // expected-warning@-1 {{TRUE}} expected-note@-1 {{TRUE}} expected-note@-1 {{Assuming 'ptr' is not equal to 'copy'}} + // expected-warning@-2 {{FALSE}} expected-note@-2 {{FALSE}} expected-note@-2 {{Assuming 'ptr' is equal to 'copy'}} + free(copy); +} + +// -----======== Testing ComparisonConstraint ========----- +// Only ReturnValueCondition uses this, but taint related issues only checked inside check::PreCall. +// Thus, testing this is impossible. + +// -----======== Testing BufferSizeConstraint ========----- +// Testing BufferSizeConstraint, which has 3 different scenarios: + +// Scenario No 1: Concrete value as the minimum buffer size. +// Extent of `buf` must be at least 26 bytes according the POSIX standard. +struct tm; +char *asctime_r(const struct tm *restrict tm, char *restrict buf); +void testTaintedExtentMightBeOutOfRange1(struct tm *tm) { + int n; + scanf("%d", &n); // expected-note {{Propagated taint to the 2nd parameter}} + if (n <= 0) // expected-note + {{Assuming 'n' is > 0}} expected-note + {{Taking false branch}} + return; + + char *buf = malloc(n); + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} Generic taint checker. + // expected-note@-2 {{Untrusted data is used to specify the buffer size}} Generic taint checker. + // expected-note@-3 {{'buf' initialized here}} + + asctime_r(tm, buf); + // expected-warning@-1 {{Function argument constraint is not satisfied, constraint: BufferSize; It depends on tainted value}} + // expected-note@-2 {{Function argument constraint is not satisfied, constraint: BufferSize; It depends on tainted value}} + // expected-note@-3 {{The size of the 2nd arg should be equal to or less than the value of 26}} + + free(buf); +} + +// Scenario No 2: Argument as a buffer size. +// The extent of `address` is should be equal to `address_len`. +struct sockaddr; +typedef int socklen_t; +const int SOCKET_LEN = 404; +int bind(int socket, const struct sockaddr *address, socklen_t address_len); +void testTaintedExtentMightBeOutOfRange2(int fd, struct sockaddr *my_address) { + int my_address_len; + scanf("%d", &my_address_len); + // expected-note@-1 {{Value assigned to 'my_address_len'}} + // expected-note@-2 {{Propagated taint to the 2nd parameter}} + + bind(fd, my_address, my_address_len); + // expected-warning@-1 {{Function argument constraint is not satisfied, constraint: BufferSize; It depends on tainted value}} + // expected-note@-2 {{Function argument constraint is not satisfied, constraint: BufferSize; It depends on tainted value}} + // expected-note@-3 {{The size of the 2nd arg should be equal to or less than the value of the 3rd arg}} +} + +// Scenario No 3: The size is computed as a multiplication of other args. +// Here, `ptr` is the buffer, and its minimum size is `size * nmemb`. +struct FILE_impl; +typedef struct FILE_impl FILE; +size_t fread(void *restrict ptr, size_t size, size_t nmemb, FILE *restrict); +void testTaintedExtentMightBeOutOfRange3(FILE *stream) { + int count; + scanf("%d", &count); + // expected-note@-1 {{Value assigned to 'count'}} + // expected-note@-2 {{Propagated taint to the 2nd parameter}} + + if (count <= 0) // expected-note + {{Assuming 'count' is > 0}} expected-note + {{Taking false branch}} + return; + + int buffer[40]; // expected-note {{'buffer' initialized here}} + fread(buffer, sizeof(buffer[0]), count, stream); + // expected-warning@-1 {{Function argument constraint is not satisfied, constraint: BufferSize; It depends on tainted value}} + // expected-note@-2 {{Function argument constraint is not satisfied, constraint: BufferSize; It depends on tainted value}} + // expected-note@-3 {{The size of the 1st arg should be equal to or less than the value of the 2nd arg times the 3rd arg}} +} diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c --- a/clang/test/Analysis/string.c +++ b/clang/test/Analysis/string.c @@ -1,10 +1,13 @@ -// RUN: %clang_analyze_cc1 -verify %s -Wno-null-dereference \ +// RUN: %clang_analyze_cc1 %s -Wno-null-dereference \ // RUN: -analyzer-checker=core \ // RUN: -analyzer-checker=unix.cstring \ // RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.security.taint \ // RUN: -analyzer-checker=alpha.unix.cstring \ // RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false +// RUN: -analyzer-config alpha.unix.cstring.OutOfBounds:ConsiderTaint=true \ +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -verify=expected,tainted,OOB-consider-tainted // // RUN: %clang_analyze_cc1 -verify %s -Wno-null-dereference -DUSE_BUILTINS \ // RUN: -analyzer-checker=core \ @@ -22,7 +25,7 @@ // RUN: -analyzer-checker=debug.ExprInspection \ // RUN: -analyzer-config eagerly-assume=false // -// RUN: %clang_analyze_cc1 -verify %s -Wno-null-dereference \ +// RUN: %clang_analyze_cc1 %s -Wno-null-dereference \ // RUN: -DUSE_BUILTINS -DVARIANT \ // RUN: -analyzer-checker=core \ // RUN: -analyzer-checker=alpha.security.taint \ @@ -30,7 +33,8 @@ // RUN: -analyzer-checker=unix.Malloc \ // RUN: -analyzer-checker=alpha.unix.cstring \ // RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -verify=expected,tainted // // RUN: %clang_analyze_cc1 -verify %s -Wno-null-dereference \ // RUN: -DSUPPRESS_OUT_OF_BOUND \ @@ -481,6 +485,23 @@ } #endif +void strcat_overflow_tainted_dst_extent(char *y) { + int n; + scanf("%d", &n); + char *p = malloc(n); // tainted-warning {{Untrusted data is used to specify the buffer size}} + p[0] = '\0'; + + if (strlen(y) == 3) + strcat(p, y); // OOB-consider-tainted-warning {{String concatenation function might overflows the destination buffer}} + free(p); +} + +void strcat_overflow_tainted_src_content(char *y) { + char x[4] = "12"; + scanf("%s100", y); // Assuming that y is at least 100 bytes long. + strcat(x, y); // FIXME: The extent is not tainted, but we should still emit a warning here. +} + void strcat_no_overflow(char *y) { char x[5] = "12"; if (strlen(y) == 2) diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c --- a/clang/test/Analysis/taint-diagnostic-visitor.c +++ b/clang/test/Analysis/taint-diagnostic-visitor.c @@ -1,9 +1,12 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,unix.Malloc,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s // This file is for testing enhanced diagnostics produced by the GenericTaintChecker int scanf(const char *restrict format, ...); int system(const char *command); +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t size); +void free(void *ptr); void taintDiagnostic(void) { @@ -34,3 +37,19 @@ int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}} // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}} } + +void taintDiagnosticMalloc(int conj) { + int x; + scanf("%d", &x); + // expected-note@-1 2 {{Taint originated here}} Once for malloc(tainted), once for BoundsV2. + + int *p = (int *)malloc(x + conj); // Generic taint checker forbids tainted allocation. + // expected-warning@-1 {{Untrusted data is used to specify the buffer size}} + // expected-note@-2 {{Untrusted data is used to specify the buffer size}} + // expected-note@-3 {{Allocating tainted amount of memory}} + + p[1] = 1; // BoundsV2 checker can not prove that the access is safe. + // expected-warning@-1 {{Out of bound memory access (index is tainted)}} + // expected-note@-2 {{Out of bound memory access (index is tainted)}} + free(p); +}