[analyzer] Add a syntactic security check for ObjC NSCoder API.

[NoQ]

Summary

Method -[NSCoder decodeValueOfObjCType:at:] is not only deprecated but also a security hazard, hence a loud check.

[dcoughlin]

This looks good to me, but I think we need a deployment target check on the diagnostic since the safe API is only available in iOS 11+, macOS 10.13+, tvOS 11+, and watchOS 4.0+. If the deployment target is early than those versions then we should not diagnose.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 def DeprecatedOrUnsafeBufferHandling :
   Checker<"DeprecatedOrUnsafeBufferHandling">,
   HelpText<"Warn on uses of unsecure or deprecated buffer manipulating "
            "functions">,
   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 {
 
 def FloatLoopCounter : Checker<"FloatLoopCounter">,
   HelpText<"Warn on using a floating point value as a loop counter (CERT: "
            "FLP30-C, FLP30-CPP)">,
   Dependencies<[SecuritySyntaxChecker]>,

clang/lib/Driver/ToolChains/Clang.cpp

if (!Args.hasArg(options::OPT__analyzer_no_default_checks)) {
     }
 
     // Disable some unix checkers for PS4.
     if (Triple.isPS4CPU()) {
       CmdArgs.push_back("-analyzer-disable-checker=unix.API");
       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");
 
     if (types::isCXX(Input.getType()))
       CmdArgs.push_back("-analyzer-checker=cplusplus");
 
     if (!Triple.isPS4CPU()) {
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.UncheckedReturn");

clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

struct ChecksFilter {
   DefaultBool check_mktemp;
   DefaultBool check_mkstemp;
   DefaultBool check_strcpy;
   DefaultBool check_DeprecatedOrUnsafeBufferHandling;
   DefaultBool check_rand;
   DefaultBool check_vfork;
   DefaultBool check_FloatLoopCounter;
   DefaultBool check_UncheckedReturn;
+  DefaultBool check_decodeValueOfObjCType;
 
   CheckerNameRef checkName_bcmp;
   CheckerNameRef checkName_bcopy;
   CheckerNameRef checkName_bzero;
   CheckerNameRef checkName_gets;
   CheckerNameRef checkName_getpw;
   CheckerNameRef checkName_mktemp;
   CheckerNameRef checkName_mkstemp;
   CheckerNameRef checkName_strcpy;
   CheckerNameRef checkName_DeprecatedOrUnsafeBufferHandling;
   CheckerNameRef checkName_rand;
   CheckerNameRef checkName_vfork;
   CheckerNameRef checkName_FloatLoopCounter;
   CheckerNameRef checkName_UncheckedReturn;
+  CheckerNameRef checkName_decodeValueOfObjCType;
 };
 
 class WalkAST : public StmtVisitor<WalkAST> {
   BugReporter &BR;
   AnalysisDeclContext* AC;
   enum { num_setids = 6 };
   IdentifierInfo *II_setid[num_setids];
 
   const bool CheckRand;
   const ChecksFilter &filter;
 
 public:
   WalkAST(BugReporter &br, AnalysisDeclContext* ac,
           const ChecksFilter &f)
   : BR(br), AC(ac), II_setid(),
     CheckRand(isArc4RandomAvailable(BR.getContext())),
     filter(f) {}
 
   // 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); }
 
   void VisitChildren(Stmt *S);
 
   // Helpers.
   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);
   void checkCall_bcmp(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_bcopy(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_gets(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_getpw(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_mktemp(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_mkstemp(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_strcat(const CallExpr *CE, const FunctionDecl *FD);
   void checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE,
                                              const FunctionDecl *FD);
   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
 
 //===----------------------------------------------------------------------===//
 // AST walking.
 //===----------------------------------------------------------------------===//
 
void WalkAST::VisitCallExpr(CallExpr *CE) {
   // Check and evaluate the call.
   if (evalFunction)
     (this->*evalFunction)(CE, FD);
 
   // Recurse and check children.
   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) {
       if (CallExpr *CE = dyn_cast<CallExpr>(Child))
         checkUncheckedReturnValue(CE);
       Visit(Child);
     }
 }
BR.EmitBasicReport(AC->getDecl(), filter.checkName_vfork,
                      "Call to function 'vfork' is insecure as it can lead to "
                      "denial of service situations in the parent process. "
                      "Replace calls to vfork with calls to the safer "
                      "'posix_spawn' function",
                      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>
 //===----------------------------------------------------------------------===//
 
 void WalkAST::checkUncheckedReturnValue(CallExpr *CE) {
   if (!filter.check_UncheckedReturn)
     return;
 

 REGISTER_CHECKER(mkstemp)
 REGISTER_CHECKER(mktemp)
 REGISTER_CHECKER(strcpy)
 REGISTER_CHECKER(rand)
 REGISTER_CHECKER(vfork)
 REGISTER_CHECKER(FloatLoopCounter)
 REGISTER_CHECKER(UncheckedReturn)
 REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling)
+REGISTER_CHECKER(decodeValueOfObjCType)

clang/test/Analysis/security-syntax-checks-nscoder.m

+// 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
+}

clang/www/analyzer/available_checks.html

 Warn on uses of the <code>vfork</code> function.</div></div></a></td>
 <td><div class="exampleContainer expandable">
 <div class="example"><pre>
 void test() {
   vfork(); // warn
 }
 </pre></div></div></td></tr>
 
+
+<tr><td><a id="security.insecureAPI.decodeValueOfObjCType"><div class="namedescr expandable"><span class="name">
+security.insecureAPI.decodeValueOfObjCType</span><span class="lang">
+(ObjC)</span><div class="descr">
+Warn on uses of the <code>-[NSCoder decodeValueOfObjCType:at:]</code> method.
+The safe alternative is <code>-[NSCoder decodeValueOfObjCType:at:size:]</code>.</div></div></a></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+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]; // warn
+}
+</pre></div></div></td></tr>
+
 </tbody></table>
 
 <!-- =========================== unix =========================== -->
 <h3 id="unix_checkers">Unix Checkers</h3>
 <table class="checkers">
 <colgroup><col class="namedescr"><col class="example"></colgroup>
 <thead><tr><td>Name, Description</td><td>Example</td></tr></thead>