From c8426ae24cf7a171af407f8caf7be3df38abe6b4 Mon Sep 17 00:00:00 2001 From: Borsik Gabor Date: Sat, 23 Nov 2019 22:28:46 +0100 Subject: [PATCH 1/6] [analyzer] Add support for namespaces to GenericTaintChecker This patch introduces the namespaces for the configured functions and also enables the use of the member functions. I added an optional Scope field for every configured function. Functions without Scope match for every function regardless of the namespace. Functions with Scope will match if the full name of the function starts with the Scope. Multiple functions can exist with the same name. Differential Revision: https://reviews.llvm.org/D70878 (cherry picked from commit 273e67425243c74b33d24aa5b2c574877cc3e9bb) --- .../Checkers/GenericTaintChecker.cpp | 181 ++++++++++++------ .../Analysis/Inputs/taint-generic-config.yaml | 41 ++++ clang/test/Analysis/taint-generic.cpp | 126 ++++++++++++ 3 files changed, 290 insertions(+), 58 deletions(-) create mode 100644 clang/test/Analysis/taint-generic.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 978b0ce568b52..e64031dd67d0b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -24,9 +24,10 @@ #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" -#include "llvm/ADT/StringMap.h" #include "llvm/Support/YAMLTraits.h" +#include #include +#include #include using namespace clang; @@ -56,10 +57,11 @@ class GenericTaintChecker /// Used to parse the configuration file. struct TaintConfiguration { - using NameArgsPair = std::pair; + using NameScopeArgs = std::tuple; struct Propagation { std::string Name; + std::string Scope; ArgVector SrcArgs; SignedArgVector DstArgs; VariadicType VarType; @@ -67,8 +69,8 @@ class GenericTaintChecker }; std::vector Propagations; - std::vector Filters; - std::vector Sinks; + std::vector Filters; + std::vector Sinks; TaintConfiguration() = default; TaintConfiguration(const TaintConfiguration &) = default; @@ -97,18 +99,49 @@ class GenericTaintChecker BT.reset(new BugType(this, "Use of Untrusted Data", "Untrusted Data")); } + struct FunctionData { + FunctionData() = delete; + FunctionData(const FunctionData &) = default; + FunctionData(FunctionData &&) = default; + FunctionData &operator=(const FunctionData &) = delete; + FunctionData &operator=(FunctionData &&) = delete; + + static Optional create(const CallExpr *CE, + const CheckerContext &C) { + const FunctionDecl *FDecl = C.getCalleeDecl(CE); + if (!FDecl || (FDecl->getKind() != Decl::Function && + FDecl->getKind() != Decl::CXXMethod)) + return None; + + StringRef Name = C.getCalleeName(FDecl); + std::string FullName = FDecl->getQualifiedNameAsString(); + if (Name.empty() || FullName.empty()) + return None; + + return FunctionData{FDecl, Name, FullName}; + } + + bool isInScope(StringRef Scope) const { + return StringRef(FullName).startswith(Scope); + } + + const FunctionDecl *const FDecl; + const StringRef Name; + const std::string FullName; + }; + /// Catch taint related bugs. Check if tainted data is passed to a /// system call etc. Returns true on matching. - bool checkPre(const CallExpr *CE, const FunctionDecl *FDecl, StringRef Name, + bool checkPre(const CallExpr *CE, const FunctionData &FData, CheckerContext &C) const; /// Add taint sources on a pre-visit. Returns true on matching. - bool addSourcesPre(const CallExpr *CE, const FunctionDecl *FDecl, - StringRef Name, CheckerContext &C) const; + bool addSourcesPre(const CallExpr *CE, const FunctionData &FData, + CheckerContext &C) const; /// Mark filter's arguments not tainted on a pre-visit. Returns true on /// matching. - bool addFiltersPre(const CallExpr *CE, StringRef Name, + bool addFiltersPre(const CallExpr *CE, const FunctionData &FData, CheckerContext &C) const; /// Propagate taint generated at pre-visit. Returns true on matching. @@ -149,7 +182,7 @@ class GenericTaintChecker /// Check if tainted data is used as a custom sink's parameter. static constexpr llvm::StringLiteral MsgCustomSink = "Untrusted data is passed to a user-defined sink"; - bool checkCustomSinks(const CallExpr *CE, StringRef Name, + bool checkCustomSinks(const CallExpr *CE, const FunctionData &FData, CheckerContext &C) const; /// Generate a report if the expression is tainted or points to tainted data. @@ -157,8 +190,17 @@ class GenericTaintChecker CheckerContext &C) const; struct TaintPropagationRule; - using NameRuleMap = llvm::StringMap; - using NameArgMap = llvm::StringMap; + template + using ConfigDataMap = + std::unordered_multimap>; + using NameRuleMap = ConfigDataMap; + using NameArgMap = ConfigDataMap; + + /// Find a function with the given name and scope. Returns the first match + /// or the end of the map. + template + static auto findFunctionInConfig(const ConfigDataMap &Map, + const FunctionData &FData); /// A struct used to specify taint propagation rules for a function. /// @@ -200,8 +242,7 @@ class GenericTaintChecker /// Get the propagation rule for a given function. static TaintPropagationRule getTaintPropagationRule(const NameRuleMap &CustomPropagations, - const FunctionDecl *FDecl, StringRef Name, - CheckerContext &C); + const FunctionData &FData, CheckerContext &C); void addSrcArg(unsigned A) { SrcArgs.push_back(A); } void addDstArg(unsigned A) { DstArgs.push_back(A); } @@ -236,14 +277,15 @@ class GenericTaintChecker CheckerContext &C); }; - /// Defines a map between the propagation function's name and - /// TaintPropagationRule. + /// Defines a map between the propagation function's name, scope + /// and TaintPropagationRule. NameRuleMap CustomPropagations; - /// Defines a map between the filter function's name and filtering args. + /// Defines a map between the filter function's name, scope and filtering + /// args. NameArgMap CustomFilters; - /// Defines a map between the sink function's name and sinking args. + /// Defines a map between the sink function's name, scope and sinking args. NameArgMap CustomSinks; }; @@ -260,7 +302,7 @@ constexpr llvm::StringLiteral GenericTaintChecker::MsgCustomSink; using TaintConfig = GenericTaintChecker::TaintConfiguration; LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::Propagation) -LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::NameArgsPair) +LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::NameScopeArgs) namespace llvm { namespace yaml { @@ -275,6 +317,7 @@ template <> struct MappingTraits { template <> struct MappingTraits { static void mapping(IO &IO, TaintConfig::Propagation &Propagation) { IO.mapRequired("Name", Propagation.Name); + IO.mapOptional("Scope", Propagation.Scope); IO.mapOptional("SrcArgs", Propagation.SrcArgs); IO.mapOptional("DstArgs", Propagation.DstArgs); IO.mapOptional("VariadicType", Propagation.VarType, @@ -292,10 +335,11 @@ template <> struct ScalarEnumerationTraits { } }; -template <> struct MappingTraits { - static void mapping(IO &IO, TaintConfig::NameArgsPair &NameArg) { - IO.mapRequired("Name", NameArg.first); - IO.mapRequired("Args", NameArg.second); +template <> struct MappingTraits { + static void mapping(IO &IO, TaintConfig::NameScopeArgs &NSA) { + IO.mapRequired("Name", std::get<0>(NSA)); + IO.mapOptional("Scope", std::get<1>(NSA)); + IO.mapRequired("Args", std::get<2>(NSA)); } }; } // namespace yaml @@ -328,31 +372,51 @@ void GenericTaintChecker::parseConfiguration(CheckerManager &Mgr, const std::string &Option, TaintConfiguration &&Config) { for (auto &P : Config.Propagations) { - GenericTaintChecker::CustomPropagations.try_emplace( - P.Name, std::move(P.SrcArgs), - convertToArgVector(Mgr, Option, P.DstArgs), P.VarType, P.VarIndex); + GenericTaintChecker::CustomPropagations.emplace( + P.Name, + std::make_pair(P.Scope, TaintPropagationRule{ + std::move(P.SrcArgs), + convertToArgVector(Mgr, Option, P.DstArgs), + P.VarType, P.VarIndex})); } for (auto &F : Config.Filters) { - GenericTaintChecker::CustomFilters.try_emplace(F.first, - std::move(F.second)); + GenericTaintChecker::CustomFilters.emplace( + std::get<0>(F), + std::make_pair(std::move(std::get<1>(F)), std::move(std::get<2>(F)))); } for (auto &S : Config.Sinks) { - GenericTaintChecker::CustomSinks.try_emplace(S.first, std::move(S.second)); + GenericTaintChecker::CustomSinks.emplace( + std::get<0>(S), + std::make_pair(std::move(std::get<1>(S)), std::move(std::get<2>(S)))); } } +template +auto GenericTaintChecker::findFunctionInConfig(const ConfigDataMap &Map, + const FunctionData &FData) { + auto Range = Map.equal_range(FData.Name); + auto It = + std::find_if(Range.first, Range.second, [&FData](const auto &Entry) { + const auto &Value = Entry.second; + StringRef Scope = Value.first; + return Scope.empty() || FData.isInScope(Scope); + }); + return It != Range.second ? It : Map.end(); +} + GenericTaintChecker::TaintPropagationRule GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule( - const NameRuleMap &CustomPropagations, const FunctionDecl *FDecl, - StringRef Name, CheckerContext &C) { + const NameRuleMap &CustomPropagations, const FunctionData &FData, + CheckerContext &C) { // TODO: Currently, we might lose precision here: we always mark a return // value as tainted even if it's just a pointer, pointing to tainted data. // Check for exact name match for functions without builtin substitutes. + // Use qualified name, because these are C functions without namespace. TaintPropagationRule Rule = - llvm::StringSwitch(Name) + llvm::StringSwitch(FData.FullName) // Source functions // TODO: Add support for vfscanf & family. .Case("fdopen", TaintPropagationRule({}, {ReturnValueIndex})) @@ -397,6 +461,7 @@ GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule( // Check if it's one of the memory setting/copying functions. // This check is specialized but faster then calling isCLibraryFunction. + const FunctionDecl *FDecl = FData.FDecl; unsigned BId = 0; if ((BId = FDecl->getMemoryFunctionKind())) switch (BId) { @@ -440,35 +505,32 @@ GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule( // or smart memory copy: // - memccpy - copying until hitting a special character. - auto It = CustomPropagations.find(Name); - if (It != CustomPropagations.end()) - return It->getValue(); + auto It = findFunctionInConfig(CustomPropagations, FData); + if (It != CustomPropagations.end()) { + const auto &Value = It->second; + return Value.second; + } return TaintPropagationRule(); } void GenericTaintChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const { - const FunctionDecl *FDecl = C.getCalleeDecl(CE); - // Check for non-global functions. - if (!FDecl || FDecl->getKind() != Decl::Function) - return; - - StringRef Name = C.getCalleeName(FDecl); - if (Name.empty()) + Optional FData = FunctionData::create(CE, C); + if (!FData) return; // Check for taintedness related errors first: system call, uncontrolled // format string, tainted buffer size. - if (checkPre(CE, FDecl, Name, C)) + if (checkPre(CE, *FData, C)) return; // Marks the function's arguments and/or return value tainted if it present in // the list. - if (addSourcesPre(CE, FDecl, Name, C)) + if (addSourcesPre(CE, *FData, C)) return; - addFiltersPre(CE, Name, C); + addFiltersPre(CE, *FData, C); } void GenericTaintChecker::checkPostStmt(const CallExpr *CE, @@ -484,12 +546,11 @@ void GenericTaintChecker::printState(raw_ostream &Out, ProgramStateRef State, } bool GenericTaintChecker::addSourcesPre(const CallExpr *CE, - const FunctionDecl *FDecl, - StringRef Name, + const FunctionData &FData, CheckerContext &C) const { // First, try generating a propagation rule for this function. TaintPropagationRule Rule = TaintPropagationRule::getTaintPropagationRule( - this->CustomPropagations, FDecl, Name, C); + this->CustomPropagations, FData, C); if (!Rule.isNull()) { ProgramStateRef State = Rule.process(CE, C); if (State) { @@ -500,14 +561,16 @@ bool GenericTaintChecker::addSourcesPre(const CallExpr *CE, return false; } -bool GenericTaintChecker::addFiltersPre(const CallExpr *CE, StringRef Name, +bool GenericTaintChecker::addFiltersPre(const CallExpr *CE, + const FunctionData &FData, CheckerContext &C) const { - auto It = CustomFilters.find(Name); + auto It = findFunctionInConfig(CustomFilters, FData); if (It == CustomFilters.end()) return false; ProgramStateRef State = C.getState(); - const ArgVector &Args = It->getValue(); + const auto &Value = It->second; + const ArgVector &Args = Value.second; for (unsigned ArgNum : Args) { if (ArgNum >= CE->getNumArgs()) continue; @@ -564,19 +627,19 @@ bool GenericTaintChecker::propagateFromPre(const CallExpr *CE, } bool GenericTaintChecker::checkPre(const CallExpr *CE, - const FunctionDecl *FDecl, StringRef Name, + const FunctionData &FData, CheckerContext &C) const { if (checkUncontrolledFormatString(CE, C)) return true; - if (checkSystemCall(CE, Name, C)) + if (checkSystemCall(CE, FData.Name, C)) return true; - if (checkTaintedBufferSize(CE, FDecl, C)) + if (checkTaintedBufferSize(CE, FData.FDecl, C)) return true; - if (checkCustomSinks(CE, Name, C)) + if (checkCustomSinks(CE, FData, C)) return true; return false; @@ -595,7 +658,7 @@ Optional GenericTaintChecker::getPointedToSVal(CheckerContext &C, QualType ArgTy = Arg->getType().getCanonicalType(); if (!ArgTy->isPointerType()) - return None; + return State->getSVal(*AddrLoc); QualType ValTy = ArgTy->getPointeeType(); @@ -848,13 +911,15 @@ bool GenericTaintChecker::checkTaintedBufferSize(const CallExpr *CE, generateReportIfTainted(CE->getArg(ArgNum), MsgTaintedBufferSize, C); } -bool GenericTaintChecker::checkCustomSinks(const CallExpr *CE, StringRef Name, +bool GenericTaintChecker::checkCustomSinks(const CallExpr *CE, + const FunctionData &FData, CheckerContext &C) const { - auto It = CustomSinks.find(Name); + auto It = findFunctionInConfig(CustomSinks, FData); if (It == CustomSinks.end()) return false; - const GenericTaintChecker::ArgVector &Args = It->getValue(); + const auto &Value = It->second; + const GenericTaintChecker::ArgVector &Args = Value.second; for (unsigned ArgNum : Args) { if (ArgNum >= CE->getNumArgs()) continue; diff --git a/clang/test/Analysis/Inputs/taint-generic-config.yaml b/clang/test/Analysis/Inputs/taint-generic-config.yaml index 4605dd1c2efcc..39b52ccc32e67 100755 --- a/clang/test/Analysis/Inputs/taint-generic-config.yaml +++ b/clang/test/Analysis/Inputs/taint-generic-config.yaml @@ -9,12 +9,29 @@ Propagations: - Name: mySource2 DstArgs: [0] + # int x = myNamespace::mySource3(); // x is tainted + - Name: mySource3 + Scope: "myNamespace::" + DstArgs: [-1] + + # int x = myAnotherNamespace::mySource3(); // x is tainted + - Name: mySource3 + Scope: "myAnotherNamespace::" + DstArgs: [-1] + # int x, y; # myScanf("%d %d", &x, &y); // x and y are tainted - Name: myScanf VariadicType: Dst VariadicIndex: 1 + # int x, y; + # Foo::myScanf("%d %d", &x, &y); // x and y are tainted + - Name: myMemberScanf + Scope: "Foo::" + VariadicType: Dst + VariadicIndex: 1 + # int x; // x is tainted # int y; # myPropagator(x, &y); // y is tainted @@ -40,6 +57,18 @@ Filters: - Name: isOutOfRange Args: [0] + # int x; // x is tainted + # myNamespace::isOutOfRange(&x); // x is not tainted anymore + - Name: isOutOfRange2 + Scope: "myNamespace::" + Args: [0] + + # int x; // x is tainted + # myAnotherNamespace::isOutOfRange(&x); // x is not tainted anymore + - Name: isOutOfRange2 + Scope: "myAnotherNamespace::" + Args: [0] + # A list of sink functions Sinks: # int x, y; // x and y are tainted @@ -48,3 +77,15 @@ Sinks: # mySink(0, x, 1); // It won't warn - Name: mySink Args: [0, 2] + + # int x; // x is tainted + # myNamespace::mySink(x); // It will warn + - Name: mySink2 + Scope: "myNamespace::" + Args: [0] + + # int x; // x is tainted + # myAnotherNamespace::mySink(x); // It will warn + - Name: mySink2 + Scope: "myAnotherNamespace::" + Args: [0] diff --git a/clang/test/Analysis/taint-generic.cpp b/clang/test/Analysis/taint-generic.cpp new file mode 100644 index 0000000000000..09cd54471948e --- /dev/null +++ b/clang/test/Analysis/taint-generic.cpp @@ -0,0 +1,126 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-config alpha.security.taint.TaintPropagation:Config=%S/Inputs/taint-generic-config.yaml -Wno-format-security -verify -std=c++11 %s + +#define BUFSIZE 10 +int Buffer[BUFSIZE]; + +int scanf(const char*, ...); +int mySource1(); +int mySource3(); + +bool isOutOfRange2(const int*); + +void mySink2(int); + +// Test configuration +namespace myNamespace { + void scanf(const char*, ...); + void myScanf(const char*, ...); + int mySource3(); + + bool isOutOfRange(const int*); + bool isOutOfRange2(const int*); + + void mySink(int, int, int); + void mySink2(int); +} + +namespace myAnotherNamespace { + int mySource3(); + + bool isOutOfRange2(const int*); + + void mySink2(int); +} + +void testConfigurationNamespacePropagation1() { + int x; + // The built-in functions should be matched only for functions in + // the global namespace + myNamespace::scanf("%d", &x); + Buffer[x] = 1; // no-warning + + scanf("%d", &x); + Buffer[x] = 1; // expected-warning {{Out of bound memory access }} +} + +void testConfigurationNamespacePropagation2() { + int x = mySource3(); + Buffer[x] = 1; // no-warning + + int y = myNamespace::mySource3(); + Buffer[y] = 1; // expected-warning {{Out of bound memory access }} +} + +void testConfigurationNamespacePropagation3() { + int x = myAnotherNamespace::mySource3(); + Buffer[x] = 1; // expected-warning {{Out of bound memory access }} +} + +void testConfigurationNamespacePropagation4() { + int x; + // Configured functions without scope should match for all function. + myNamespace::myScanf("%d", &x); + Buffer[x] = 1; // expected-warning {{Out of bound memory access }} +} + +void testConfigurationNamespaceFilter1() { + int x = mySource1(); + if (myNamespace::isOutOfRange2(&x)) + return; + Buffer[x] = 1; // no-warning + + int y = mySource1(); + if (isOutOfRange2(&y)) + return; + Buffer[y] = 1; // expected-warning {{Out of bound memory access }} +} + +void testConfigurationNamespaceFilter2() { + int x = mySource1(); + if (myAnotherNamespace::isOutOfRange2(&x)) + return; + Buffer[x] = 1; // no-warning +} + +void testConfigurationNamespaceFilter3() { + int x = mySource1(); + if (myNamespace::isOutOfRange(&x)) + return; + Buffer[x] = 1; // no-warning +} + +void testConfigurationNamespaceSink1() { + int x = mySource1(); + mySink2(x); // no-warning + + int y = mySource1(); + myNamespace::mySink2(y); + // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}} +} + +void testConfigurationNamespaceSink2() { + int x = mySource1(); + myAnotherNamespace::mySink2(x); + // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}} +} + +void testConfigurationNamespaceSink3() { + int x = mySource1(); + myNamespace::mySink(x, 0, 1); + // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}} +} + +struct Foo { + void scanf(const char*, int*); + void myMemberScanf(const char*, int*); +}; + +void testConfigurationMemberFunc() { + int x; + Foo foo; + foo.scanf("%d", &x); + Buffer[x] = 1; // no-warning + + foo.myMemberScanf("%d", &x); + Buffer[x] = 1; // expected-warning {{Out of bound memory access }} +} From 9f15442ec183025cc12596e429cee53d220d6737 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Tue, 17 Dec 2019 14:49:53 -0800 Subject: [PATCH 2/6] [analysis] Discard type qualifiers when casting values retrieved from the Store. This canonicalizes the representation of unknown pointer symbols, which reduces the overall confusion in pointer cast representation. Patch by Vince Bridgers! Differential Revision: https://reviews.llvm.org/D70836 (cherry picked from commit 6d3f43ec61a60c37963ee5f54289cf0759fb5d61) --- clang/lib/StaticAnalyzer/Core/Store.cpp | 14 +++-- .../test/Analysis/uninit-val-const-likeness.c | 56 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 clang/test/Analysis/uninit-val-const-likeness.c diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp index b4ab6877726ce..20660d1c2d67b 100644 --- a/clang/lib/StaticAnalyzer/Core/Store.cpp +++ b/clang/lib/StaticAnalyzer/Core/Store.cpp @@ -393,6 +393,11 @@ SVal StoreManager::attemptDownCast(SVal Base, QualType TargetType, return UnknownVal(); } +static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) { + return ty1->getPointeeType().getTypePtr() == + ty2->getPointeeType().getTypePtr(); +} + /// CastRetrievedVal - Used by subclasses of StoreManager to implement /// implicit casts that arise from loads from regions that are reinterpreted /// as another region. @@ -421,10 +426,11 @@ SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R, // FIXME: We really need a single good function to perform casts for us // correctly every time we need it. if (castTy->isPointerType() && !castTy->isVoidPointerType()) - if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) - if (SR->getSymbol()->getType().getCanonicalType() != - castTy.getCanonicalType()) - return loc::MemRegionVal(castRegion(SR, castTy)); + if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) { + QualType sr = SR->getSymbol()->getType(); + if (!hasSameUnqualifiedPointeeType(sr, castTy)) + return loc::MemRegionVal(castRegion(SR, castTy)); + } return svalBuilder.dispatchCast(V, castTy); } diff --git a/clang/test/Analysis/uninit-val-const-likeness.c b/clang/test/Analysis/uninit-val-const-likeness.c new file mode 100644 index 0000000000000..1ee1aefe8dbaf --- /dev/null +++ b/clang/test/Analysis/uninit-val-const-likeness.c @@ -0,0 +1,56 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core %s -verify +// expected-no-diagnostics + +#define SIZE 2 + +typedef struct { + int noOfSymbols; +} Params; + +static void create(const Params * const params, int fooList[]) { + int tmpList[SIZE] = {0}; + for (int i = 0; i < params->noOfSymbols; i++) + fooList[i] = tmpList[i]; +} + +int work(Params * const params) { + int fooList[SIZE]; + create(params, fooList); + int sum = 0; + for (int i = 0; i < params->noOfSymbols; i++) + sum += fooList[i]; // no-warning + return sum; +} + +static void create2(const Params * const * pparams, int fooList[]) { + const Params * params = *pparams; + int tmpList[SIZE] = {0}; + for (int i = 0; i < params->noOfSymbols; i++) + fooList[i] = tmpList[i]; +} + +int work2(const Params * const params) { + int fooList[SIZE]; + create2(¶ms, fooList); + int sum = 0; + for (int i = 0; i < params->noOfSymbols; i++) + sum += fooList[i]; // no-warning + return sum; +} + +static void create3(Params * const * pparams, int fooList[]) { + const Params * params = *pparams; + int tmpList[SIZE] = {0}; + for (int i = 0; i < params->noOfSymbols; i++) + fooList[i] = tmpList[i]; +} + +int work3(const Params * const params) { + int fooList[SIZE]; + Params *const *ptr = (Params *const*)¶ms; + create3(ptr, fooList); + int sum = 0; + for (int i = 0; i < params->noOfSymbols; i++) + sum += fooList[i]; // no-warning + return sum; +} From 6e4c22f42a547219ccb8ee76d42ae80e5fdda479 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Wed, 18 Dec 2019 12:04:18 -0800 Subject: [PATCH 3/6] [analyzer] NonnullGlobalConstants: Add support for kCFNull. It's a singleton in CoreFoundation that always contains a non-null CFNullRef. (cherry picked from commit badba5118ff5cc6d61aeca6ee2dc2ead5bb5286f) --- .../Checkers/NonnullGlobalConstantsChecker.cpp | 4 +++- clang/test/Analysis/nonnull-global-constants.mm | 10 +++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp index 43dbe57b8432b..6efba433eed22 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp @@ -36,6 +36,7 @@ class NonnullGlobalConstantsChecker : public Checker { mutable IdentifierInfo *NSStringII = nullptr; mutable IdentifierInfo *CFStringRefII = nullptr; mutable IdentifierInfo *CFBooleanRefII = nullptr; + mutable IdentifierInfo *CFNullRefII = nullptr; public: NonnullGlobalConstantsChecker() {} @@ -61,6 +62,7 @@ void NonnullGlobalConstantsChecker::initIdentifierInfo(ASTContext &Ctx) const { NSStringII = &Ctx.Idents.get("NSString"); CFStringRefII = &Ctx.Idents.get("CFStringRef"); CFBooleanRefII = &Ctx.Idents.get("CFBooleanRef"); + CFNullRefII = &Ctx.Idents.get("CFNullRef"); } /// Add an assumption that const string-like globals are non-null. @@ -136,7 +138,7 @@ bool NonnullGlobalConstantsChecker::isNonnullType(QualType Ty) const { T->getInterfaceDecl()->getIdentifier() == NSStringII; } else if (auto *T = dyn_cast(Ty)) { IdentifierInfo* II = T->getDecl()->getIdentifier(); - return II == CFStringRefII || II == CFBooleanRefII; + return II == CFStringRefII || II == CFBooleanRefII || II == CFNullRefII; } return false; } diff --git a/clang/test/Analysis/nonnull-global-constants.mm b/clang/test/Analysis/nonnull-global-constants.mm index 9e1a588ba47a8..8c174c48b30d6 100644 --- a/clang/test/Analysis/nonnull-global-constants.mm +++ b/clang/test/Analysis/nonnull-global-constants.mm @@ -7,7 +7,11 @@ @class NSString; typedef const struct __CFString *CFStringRef; -typedef const struct __CFBoolean * CFBooleanRef; +typedef const struct __CFBoolean *CFBooleanRef; + +#define CF_BRIDGED_TYPE(T) __attribute__((objc_bridge(T))) +typedef const struct CF_BRIDGED_TYPE(NSNull) __CFNull *CFNullRef; +extern const CFNullRef kCFNull; // Global NSString* is non-null. extern NSString *const StringConstGlobal; @@ -113,3 +117,7 @@ void testNonnullNonconstCFString() { void testNonnullNonnullCFString() { clang_analyzer_eval(str4); // expected-warning{{TRUE}} } + +void test_kCFNull() { + clang_analyzer_eval(kCFNull); // expected-warning{{TRUE}} +} From ef2eb0542ffcc807cf8b3e22567ba8c68390563a Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Wed, 18 Dec 2019 13:19:44 -0800 Subject: [PATCH 4/6] [analyzer] Teach MismatchedDealloc about initWithBytesNoCopy with deallocator. MallocChecker warns when memory is passed into -[NSData initWithBytesNoCopy] but isn't allocated by malloc(), because it will be deallocated by free(). However, initWithBytesNoCopy has an overload that takes an arbitrary block for deallocating the object. If such overload is used, it is no longer necessary to make sure that the memory is allocated by malloc(). (cherry picked from commit bce1cce6bf1286541c57690ab1129fbc02c60f93) --- .../StaticAnalyzer/Checkers/MallocChecker.cpp | 3 +++ .../Inputs/system-header-simulator-objc.h | 5 +++- clang/test/Analysis/malloc.mm | 23 ++++++++++++++++++- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 41d2c83829814..a8a9af20d6618 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1469,6 +1469,9 @@ void MallocChecker::checkPostObjCMessage(const ObjCMethodCall &Call, if (!*FreeWhenDone) return; + if (Call.hasNonZeroCallbackArg()) + return; + bool IsKnownToBeAllocatedMemory; ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(0), Call.getOriginExpr(), C.getState(), diff --git a/clang/test/Analysis/Inputs/system-header-simulator-objc.h b/clang/test/Analysis/Inputs/system-header-simulator-objc.h index df751d03e642a..0dc6b369b0151 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator-objc.h +++ b/clang/test/Analysis/Inputs/system-header-simulator-objc.h @@ -117,7 +117,10 @@ typedef double NSTimeInterval; + (id)dataWithBytesNoCopy:(void *)bytes length:(NSUInteger)length freeWhenDone:(BOOL)b; - (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)length; - (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)length freeWhenDone:(BOOL)b; -- (id)initWithBytes:(void *)bytes length:(NSUInteger) length; +- (id)initWithBytesNoCopy:(void *)bytes + length:(NSUInteger)length + deallocator:(void (^)(void *bytes, NSUInteger length))deallocator; +- (id)initWithBytes:(void *)bytes length:(NSUInteger)length; @end typedef struct { diff --git a/clang/test/Analysis/malloc.mm b/clang/test/Analysis/malloc.mm index e84644b9dd732..1b7dd2756e1bb 100644 --- a/clang/test/Analysis/malloc.mm +++ b/clang/test/Analysis/malloc.mm @@ -1,4 +1,8 @@ -// RUN: %clang_analyze_cc1 -std=c++14 -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify -fblocks %s +// RUN: %clang_analyze_cc1 -std=c++14 \ +// RUN: -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete \ +// RUN: -analyzer-checker=unix.MismatchedDeallocator \ +// RUN: -verify -fblocks %s + #import "Inputs/system-header-simulator-objc.h" #import "Inputs/system-header-simulator-for-malloc.h" @@ -61,6 +65,23 @@ void testNSStringFreeWhenDoneNO(NSUInteger dataLength) { NSString *nsstr = [[NSString alloc] initWithBytesNoCopy:data length:dataLength encoding:NSUTF8StringEncoding freeWhenDone:0]; // expected-warning{{leak}} } +void testNSStringFreeWhenDoneNewDelete(NSUInteger dataLength) { + unsigned char *data = new unsigned char(42); + NSData *nsdata = [[NSData alloc] initWithBytesNoCopy:data + length:dataLength freeWhenDone:1]; + // expected-warning@-2{{-initWithBytesNoCopy:length:freeWhenDone: cannot take ownership of memory allocated by 'new'}} +} + +void testNSStringFreeWhenDoneNewDelete2(NSUInteger dataLength) { + unsigned char *data = new unsigned char(42); + NSData *nsdata = [[NSData alloc] initWithBytesNoCopy:data + length:dataLength + deallocator:^(void *bytes, + NSUInteger length) { + delete (unsigned char *)bytes; + }]; // no-warning +} + void testNSStringFreeWhenDoneNO2(NSUInteger dataLength) { unichar *data = (unichar*)malloc(42); NSString *nsstr = [[NSString alloc] initWithCharactersNoCopy:data length:dataLength freeWhenDone:0]; // expected-warning{{leak}} From 89dda46e584435287f583a46146131a8e7d44969 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Wed, 18 Dec 2019 17:59:16 -0800 Subject: [PATCH 5/6] [analysis] Re-discard type sugar when casting values retrieved from the Store. Canonicalization was accidentally omitted in 6d3f43ec. (cherry picked from commit f0ced2ddb44e4bd970fec310591891a0cdb4462c) --- clang/lib/StaticAnalyzer/Core/Store.cpp | 6 +++--- .../test/Analysis/uninit-val-const-likeness.c | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp index 20660d1c2d67b..b33129c88cea4 100644 --- a/clang/lib/StaticAnalyzer/Core/Store.cpp +++ b/clang/lib/StaticAnalyzer/Core/Store.cpp @@ -394,8 +394,8 @@ SVal StoreManager::attemptDownCast(SVal Base, QualType TargetType, } static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) { - return ty1->getPointeeType().getTypePtr() == - ty2->getPointeeType().getTypePtr(); + return ty1->getPointeeType().getCanonicalType().getTypePtr() == + ty2->getPointeeType().getCanonicalType().getTypePtr(); } /// CastRetrievedVal - Used by subclasses of StoreManager to implement @@ -427,7 +427,7 @@ SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R, // correctly every time we need it. if (castTy->isPointerType() && !castTy->isVoidPointerType()) if (const auto *SR = dyn_cast_or_null(V.getAsRegion())) { - QualType sr = SR->getSymbol()->getType(); + QualType sr = SR->getSymbol()->getType(); if (!hasSameUnqualifiedPointeeType(sr, castTy)) return loc::MemRegionVal(castRegion(SR, castTy)); } diff --git a/clang/test/Analysis/uninit-val-const-likeness.c b/clang/test/Analysis/uninit-val-const-likeness.c index 1ee1aefe8dbaf..013ab7882755a 100644 --- a/clang/test/Analysis/uninit-val-const-likeness.c +++ b/clang/test/Analysis/uninit-val-const-likeness.c @@ -54,3 +54,21 @@ int work3(const Params * const params) { sum += fooList[i]; // no-warning return sum; } + +typedef Params ParamsTypedef; +typedef const ParamsTypedef *ConstParamsTypedef; + +static void create4(ConstParamsTypedef const params, int fooList[]) { + int tmpList[SIZE] = {0}; + for (int i = 0; i < params->noOfSymbols; i++) + fooList[i] = tmpList[i]; +} + +int work4(Params * const params) { + int fooList[SIZE]; + create4(params, fooList); + int sum = 0; + for (int i = 0; i < params->noOfSymbols; i++) + sum += fooList[i]; // no-warning + return sum; +} From 5ddd6562e540769819fb57dc9040e6ab39baf4ac Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Thu, 19 Dec 2019 14:21:02 -0800 Subject: [PATCH 6/6] [analyzer] Add a syntactic security check for ObjC NSCoder API. Method '-[NSCoder decodeValueOfObjCType:at:]' is not only deprecated but also a security hazard, hence a loud check. Differential Revision: https://reviews.llvm.org/D71728 (cherry picked from commit b284005072122fe4af879725e3c8090009f89ca0) --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 5 ++ clang/lib/Driver/ToolChains/Clang.cpp | 5 +- .../Checkers/CheckSecuritySyntaxOnly.cpp | 68 +++++++++++++++++++ .../Analysis/security-syntax-checks-nscoder.m | 36 ++++++++++ clang/www/analyzer/available_checks.html | 16 +++++ 5 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 clang/test/Analysis/security-syntax-checks-nscoder.m diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index be455ae14f57a..114a963fce707 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -773,6 +773,11 @@ def DeprecatedOrUnsafeBufferHandling : Dependencies<[SecuritySyntaxChecker]>, Documentation; +def decodeValueOfObjCType : Checker<"decodeValueOfObjCType">, + HelpText<"Warn on uses of the '-decodeValueOfObjCType:at:' method">, + Dependencies<[SecuritySyntaxChecker]>, + Documentation; + } // end "security.insecureAPI" let ParentPackage = Security in { diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index f3a6befe0370c..f6e15d569a33f 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -2611,8 +2611,11 @@ static void RenderAnalyzerOptions(const ArgList &Args, ArgStringList &CmdArgs, CmdArgs.push_back("-analyzer-disable-checker=unix.Vfork"); } - if (Triple.isOSDarwin()) + if (Triple.isOSDarwin()) { CmdArgs.push_back("-analyzer-checker=osx"); + CmdArgs.push_back( + "-analyzer-checker=security.insecureAPI.decodeValueOfObjCType"); + } CmdArgs.push_back("-analyzer-checker=deadcode"); diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 260a2896e78c8..d9ffa562c0aaa 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -49,6 +49,7 @@ struct ChecksFilter { DefaultBool check_vfork; DefaultBool check_FloatLoopCounter; DefaultBool check_UncheckedReturn; + DefaultBool check_decodeValueOfObjCType; CheckerNameRef checkName_bcmp; CheckerNameRef checkName_bcopy; @@ -63,6 +64,7 @@ struct ChecksFilter { CheckerNameRef checkName_vfork; CheckerNameRef checkName_FloatLoopCounter; CheckerNameRef checkName_UncheckedReturn; + CheckerNameRef checkName_decodeValueOfObjCType; }; class WalkAST : public StmtVisitor { @@ -83,6 +85,7 @@ class WalkAST : public StmtVisitor { // Statement visitor methods. void VisitCallExpr(CallExpr *CE); + void VisitObjCMessageExpr(ObjCMessageExpr *CE); void VisitForStmt(ForStmt *S); void VisitCompoundStmt (CompoundStmt *S); void VisitStmt(Stmt *S) { VisitChildren(S); } @@ -93,6 +96,7 @@ class WalkAST : public StmtVisitor { bool checkCall_strCommon(const CallExpr *CE, const FunctionDecl *FD); typedef void (WalkAST::*FnCheck)(const CallExpr *, const FunctionDecl *); + typedef void (WalkAST::*MsgCheck)(const ObjCMessageExpr *); // Checker-specific methods. void checkLoopConditionForFloat(const ForStmt *FS); @@ -110,6 +114,7 @@ class WalkAST : public StmtVisitor { void checkCall_rand(const CallExpr *CE, const FunctionDecl *FD); void checkCall_random(const CallExpr *CE, const FunctionDecl *FD); void checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD); + void checkMsg_decodeValueOfObjCType(const ObjCMessageExpr *ME); void checkUncheckedReturnValue(CallExpr *CE); }; } // end anonymous namespace @@ -182,6 +187,20 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { VisitChildren(CE); } +void WalkAST::VisitObjCMessageExpr(ObjCMessageExpr *ME) { + MsgCheck evalFunction = + llvm::StringSwitch(ME->getSelector().getAsString()) + .Case("decodeValueOfObjCType:at:", + &WalkAST::checkMsg_decodeValueOfObjCType) + .Default(nullptr); + + if (evalFunction) + (this->*evalFunction)(ME); + + // Recurse and check children. + VisitChildren(ME); +} + void WalkAST::VisitCompoundStmt(CompoundStmt *S) { for (Stmt *Child : S->children()) if (Child) { @@ -923,6 +942,54 @@ void WalkAST::checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD) { CELoc, CE->getCallee()->getSourceRange()); } +//===----------------------------------------------------------------------===// +// Check: '-decodeValueOfObjCType:at:' should not be used. +// It is deprecated in favor of '-decodeValueOfObjCType:at:size:' due to +// likelihood of buffer overflows. +//===----------------------------------------------------------------------===// + +void WalkAST::checkMsg_decodeValueOfObjCType(const ObjCMessageExpr *ME) { + if (!filter.check_decodeValueOfObjCType) + return; + + // Check availability of the secure alternative: + // iOS 11+, macOS 10.13+, tvOS 11+, and watchOS 4.0+ + // FIXME: We probably shouldn't register the check if it's not available. + const TargetInfo &TI = AC->getASTContext().getTargetInfo(); + const llvm::Triple &T = TI.getTriple(); + const VersionTuple &VT = TI.getPlatformMinVersion(); + switch (T.getOS()) { + case llvm::Triple::IOS: + if (VT < VersionTuple(11, 0)) + return; + break; + case llvm::Triple::MacOSX: + if (VT < VersionTuple(10, 13)) + return; + break; + case llvm::Triple::WatchOS: + if (VT < VersionTuple(4, 0)) + return; + break; + case llvm::Triple::TvOS: + if (VT < VersionTuple(11, 0)) + return; + break; + default: + return; + } + + PathDiagnosticLocation MELoc = + PathDiagnosticLocation::createBegin(ME, BR.getSourceManager(), AC); + BR.EmitBasicReport( + AC->getDecl(), filter.checkName_decodeValueOfObjCType, + "Potential buffer overflow in '-decodeValueOfObjCType:at:'", "Security", + "Deprecated method '-decodeValueOfObjCType:at:' is insecure " + "as it can lead to potential buffer overflows. Use the safer " + "'-decodeValueOfObjCType:at:size:' method.", + MELoc, ME->getSourceRange()); +} + //===----------------------------------------------------------------------===// // Check: Should check whether privileges are dropped successfully. // Originally: @@ -1035,3 +1102,4 @@ REGISTER_CHECKER(vfork) REGISTER_CHECKER(FloatLoopCounter) REGISTER_CHECKER(UncheckedReturn) REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling) +REGISTER_CHECKER(decodeValueOfObjCType) diff --git a/clang/test/Analysis/security-syntax-checks-nscoder.m b/clang/test/Analysis/security-syntax-checks-nscoder.m new file mode 100644 index 0000000000000..23aa95bedccdc --- /dev/null +++ b/clang/test/Analysis/security-syntax-checks-nscoder.m @@ -0,0 +1,36 @@ +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-ios11.0 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-ios10.0 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-macos10.13 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-macos10.12 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-watchos4.0 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-watchos3.0 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-tvos11.0 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-tvos10.0 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s + +// notavailable-no-diagnostics + +typedef unsigned long NSUInteger; + +@interface NSCoder +- (void)decodeValueOfObjCType:(const char *)type + at:(void *)data; +- (void)decodeValueOfObjCType:(const char *)type + at:(void *)data + size:(NSUInteger)size; +@end + +void test(NSCoder *decoder) { + // This would be a vulnerability on 64-bit platforms + // but not on 32-bit platforms. + NSUInteger x; + [decoder decodeValueOfObjCType:"I" at:&x]; // available-warning{{Deprecated method '-decodeValueOfObjCType:at:' is insecure as it can lead to potential buffer overflows. Use the safer '-decodeValueOfObjCType:at:size:' method}} + [decoder decodeValueOfObjCType:"I" at:&x size:sizeof(x)]; // no-warning +} diff --git a/clang/www/analyzer/available_checks.html b/clang/www/analyzer/available_checks.html index c610e2bda7332..b28fc84652352 100644 --- a/clang/www/analyzer/available_checks.html +++ b/clang/www/analyzer/available_checks.html @@ -1446,6 +1446,22 @@

Security Checkers

} + + + +