Merge pull request #512 from haoNoQ/static-analyzer-cherrypicks-7

Static analyzer cherrypicks 7
swiftlang · Dec 20, 2019 · dc34e57 · dc34e57
2 parents 82e1d79 + 5ddd656
commit dc34e57
Show file tree

Hide file tree

Showing 15 changed files with 544 additions and 67 deletions.
diff --git 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<HasDocumentation>;
 
+def decodeValueOfObjCType : Checker<"decodeValueOfObjCType">,
+  HelpText<"Warn on uses of the '-decodeValueOfObjCType:at:' method">,
+  Dependencies<[SecuritySyntaxChecker]>,
+  Documentation<HasDocumentation>;
+
 } // end "security.insecureAPI"
 
 let ParentPackage = Security in {

diff --git 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
@@ -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<WalkAST> {
@@ -83,6 +85,7 @@ class WalkAST : public StmtVisitor<WalkAST> {
 
   // 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<WalkAST> {
   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<WalkAST> {
   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<MsgCheck>(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: <rdar://problem/6337132>
@@ -1035,3 +1102,4 @@ REGISTER_CHECKER(vfork)
 REGISTER_CHECKER(FloatLoopCounter)
 REGISTER_CHECKER(UncheckedReturn)
 REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling)
+REGISTER_CHECKER(decodeValueOfObjCType)