[clang][analyzer] Add new option to specify functions SecuritySyntaxChecker warns about

[stemil01]

Addresses #103038.

Currently, the list of functions for which SecuritySyntaxChecker emits warnings is fixed. This patch adds a new command-line option Warn, which allows users to provide a space-separated list of function names to mark as explicitly insecure.

There is a separate commit for clang-format style changes since they affect the existing formatting of Checkers.td file.

PR #164183 addresses the other part of the issue, which is explicitly adding checks for the functions listed in the issue.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Stefan Milenkovic (stemil01)

Changes

Addresses #103038.

Currently, the list of functions for which SecuritySyntaxChecker emits warnings is fixed. This patch adds a new command-line option Warn, which allows users to provide a space-separated list of function names to mark as explicitly insecure.

There is a separate commit for clang-format style changes since they affect the existing formatting of Checkers.td file.

---

Full diff: https://github.com/llvm/llvm-project/pull/164184.diff

3 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+77-67)
• (modified) clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp (+47-11)
• (modified) clang/test/Analysis/analyzer-config.c (+1)

diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index ffae3b9310979..da8985ce44c0c 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -839,79 +839,89 @@ def PaddingChecker : Checker<"Padding">,
 
 let ParentPackage = InsecureAPI in {
 
-def SecuritySyntaxChecker : Checker<"SecuritySyntaxChecker">,
-  HelpText<"Base of various security function related checkers">,
-  Documentation<NotDocumented>,
-  Hidden;
-
-def bcmp : Checker<"bcmp">,
-  HelpText<"Warn on uses of the 'bcmp' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
-
-def bcopy : Checker<"bcopy">,
-  HelpText<"Warn on uses of the 'bcopy' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
-
-def bzero : Checker<"bzero">,
-  HelpText<"Warn on uses of the 'bzero' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
-
-def gets : Checker<"gets">,
-  HelpText<"Warn on uses of the 'gets' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
-
-def getpw : Checker<"getpw">,
-  HelpText<"Warn on uses of the 'getpw' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
-
-def mktemp : Checker<"mktemp">,
-  HelpText<"Warn on uses of the 'mktemp' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
-
-def mkstemp : Checker<"mkstemp">,
-  HelpText<"Warn when 'mkstemp' is passed fewer than 6 X's in the format "
-           "string">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
+  def SecuritySyntaxChecker
+      : Checker<"SecuritySyntaxChecker">,
+        HelpText<"Base of various security function related checkers">,
+        CheckerOptions<[CmdLineOption<
+            String, "Warn",
+            "List of space-separated function name to be warned about. "
+            "Defaults to an empty list.",
+            "", InAlpha>]>,
+        Documentation<NotDocumented>,
+        Hidden;
+
+  def bcmp : Checker<"bcmp">,
+             HelpText<"Warn on uses of the 'bcmp' function">,
+             Dependencies<[SecuritySyntaxChecker]>,
+             Documentation<HasDocumentation>;
+
+  def bcopy : Checker<"bcopy">,
+              HelpText<"Warn on uses of the 'bcopy' function">,
+              Dependencies<[SecuritySyntaxChecker]>,
+              Documentation<HasDocumentation>;
+
+  def bzero : Checker<"bzero">,
+              HelpText<"Warn on uses of the 'bzero' function">,
+              Dependencies<[SecuritySyntaxChecker]>,
+              Documentation<HasDocumentation>;
+
+  def gets : Checker<"gets">,
+             HelpText<"Warn on uses of the 'gets' function">,
+             Dependencies<[SecuritySyntaxChecker]>,
+             Documentation<HasDocumentation>;
+
+  def getpw : Checker<"getpw">,
+              HelpText<"Warn on uses of the 'getpw' function">,
+              Dependencies<[SecuritySyntaxChecker]>,
+              Documentation<HasDocumentation>;
+
+  def mktemp : Checker<"mktemp">,
+               HelpText<"Warn on uses of the 'mktemp' function">,
+               Dependencies<[SecuritySyntaxChecker]>,
+               Documentation<HasDocumentation>;
+
+  def mkstemp
+      : Checker<"mkstemp">,
+        HelpText<"Warn when 'mkstemp' is passed fewer than 6 X's in the format "
+                 "string">,
+        Dependencies<[SecuritySyntaxChecker]>,
+        Documentation<HasDocumentation>;
 
-def rand : Checker<"rand">,
-  HelpText<"Warn on uses of the 'rand', 'random', and related functions">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
+  def rand
+      : Checker<"rand">,
+        HelpText<"Warn on uses of the 'rand', 'random', and related functions">,
+        Dependencies<[SecuritySyntaxChecker]>,
+        Documentation<HasDocumentation>;
 
-def strcpy : Checker<"strcpy">,
-  HelpText<"Warn on uses of the 'strcpy' and 'strcat' functions">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
+  def strcpy : Checker<"strcpy">,
+               HelpText<"Warn on uses of the 'strcpy' and 'strcat' functions">,
+               Dependencies<[SecuritySyntaxChecker]>,
+               Documentation<HasDocumentation>;
 
-def vfork : Checker<"vfork">,
-  HelpText<"Warn on uses of the 'vfork' function">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
+  def vfork : Checker<"vfork">,
+              HelpText<"Warn on uses of the 'vfork' function">,
+              Dependencies<[SecuritySyntaxChecker]>,
+              Documentation<HasDocumentation>;
 
-def UncheckedReturn : Checker<"UncheckedReturn">,
-  HelpText<"Warn on uses of functions whose return values must be always "
-           "checked">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
+  def UncheckedReturn
+      : Checker<"UncheckedReturn">,
+        HelpText<"Warn on uses of functions whose return values must be always "
+                 "checked">,
+        Dependencies<[SecuritySyntaxChecker]>,
+        Documentation<HasDocumentation>;
 
-def DeprecatedOrUnsafeBufferHandling :
-  Checker<"DeprecatedOrUnsafeBufferHandling">,
-  HelpText<"Warn on uses of unsecure or deprecated buffer manipulating "
-           "functions">,
-  Dependencies<[SecuritySyntaxChecker]>,
-  Documentation<HasDocumentation>;
+  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>;
+  def decodeValueOfObjCType
+      : Checker<"decodeValueOfObjCType">,
+        HelpText<"Warn on uses of the '-decodeValueOfObjCType:at:' method">,
+        Dependencies<[SecuritySyntaxChecker]>,
+        Documentation<HasDocumentation>;
 
 } // end "security.insecureAPI"
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
index 5e75c1c4a3abd..5415e19431ca5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -74,13 +74,14 @@ class WalkAST : public StmtVisitor<WalkAST> {
 
   const bool CheckRand;
   const ChecksFilter &filter;
+  llvm::StringSet<> WarnFunctions;
 
 public:
-  WalkAST(BugReporter &br, AnalysisDeclContext* ac,
-          const ChecksFilter &f)
-  : BR(br), AC(ac), II_setid(),
-    CheckRand(isArc4RandomAvailable(BR.getContext())),
-    filter(f) {}
+  WalkAST(BugReporter &br, AnalysisDeclContext *ac, const ChecksFilter &f,
+          llvm::StringSet<> wf)
+      : BR(br), AC(ac), II_setid(),
+        CheckRand(isArc4RandomAvailable(BR.getContext())), filter(f),
+        WarnFunctions(wf) {}
 
   // Statement visitor methods.
   void VisitCallExpr(CallExpr *CE);
@@ -101,6 +102,7 @@ class WalkAST : public StmtVisitor<WalkAST> {
   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_custom(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);
@@ -175,12 +177,10 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
           .Case("rand_r", &WalkAST::checkCall_rand)
           .Case("random", &WalkAST::checkCall_random)
           .Case("vfork", &WalkAST::checkCall_vfork)
-          .Default(nullptr);
+          .Default(&WalkAST::checkCall_custom);
 
-  // If the callee isn't defined, it is not of security concern.
   // Check and evaluate the call.
-  if (evalFunction)
-    (this->*evalFunction)(CE, FD);
+  (this->*evalFunction)(CE, FD);
 
   // Recurse and check children.
   VisitChildren(CE);
@@ -542,6 +542,29 @@ void WalkAST::checkCall_getpw(const CallExpr *CE, const FunctionDecl *FD) {
                      CELoc, CE->getCallee()->getSourceRange());
 }
 
+//===----------------------------------------------------------------------===//
+// Check: Any use of a function from the user-provided list.
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_custom(const CallExpr *CE, const FunctionDecl *FD) {
+  IdentifierInfo *II = FD->getIdentifier();
+  if (!II) // if no identifier, not a simple C function
+    return;
+  StringRef Name = II->getName();
+  Name.consume_front("__builtin_");
+
+  if (!(this->WarnFunctions.contains(Name)))
+    return;
+
+  // Issue a warning.
+  std::string Msg = ("Call to user-defined function '" + Name + "'.").str();
+  PathDiagnosticLocation CELoc =
+      PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+  BR.EmitBasicReport(AC->getDecl(), filter.checkName_getpw,
+                     "User-provided function to be warned about", "Security",
+                     Msg, CELoc, CE->getCallee()->getSourceRange());
+}
+
 //===----------------------------------------------------------------------===//
 // Check: Any use of 'mktemp' is insecure.  It is obsoleted by mkstemp().
 // CWE-377: Insecure Temporary File
@@ -1075,17 +1098,30 @@ namespace {
 class SecuritySyntaxChecker : public Checker<check::ASTCodeBody> {
 public:
   ChecksFilter filter;
+  llvm::StringSet<> WarnFunctions;
 
   void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
                         BugReporter &BR) const {
-    WalkAST walker(BR, mgr.getAnalysisDeclContext(D), filter);
+    WalkAST walker(BR, mgr.getAnalysisDeclContext(D), filter, WarnFunctions);
     walker.Visit(D->getBody());
   }
+
+  void setWarnFunctions(StringRef Input) {
+    StringRef CurrentStr = Input;
+    while (!CurrentStr.empty()) {
+      auto SplitResult = CurrentStr.split(' ');
+      WarnFunctions.insert(SplitResult.first);
+      CurrentStr = SplitResult.second;
+    }
+  }
 };
 }
 
 void ento::registerSecuritySyntaxChecker(CheckerManager &mgr) {
-  mgr.registerChecker<SecuritySyntaxChecker>();
+  SecuritySyntaxChecker *checker = mgr.registerChecker<SecuritySyntaxChecker>();
+  StringRef WarnOption =
+      mgr.getAnalyzerOptions().getCheckerStringOption(checker, "Warn");
+  checker->setWarnFunctions(WarnOption);
 }
 
 bool ento::shouldRegisterSecuritySyntaxChecker(const CheckerManager &mgr) {
diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c
index 7936273415ad4..88b0ee427404a 100644
--- a/clang/test/Analysis/analyzer-config.c
+++ b/clang/test/Analysis/analyzer-config.c
@@ -121,6 +121,7 @@
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
 // CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false
+// CHECK-NEXT: security.insecureAPI.SecuritySyntaxChecker:Warn = ""
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: silence-checkers = ""
 // CHECK-NEXT: stable-report-filename = false