[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

[stemil01]

@haoNoQ @Xazax-hun @steakhal
Could you please review or suggest who would be the appropriate reviewer for this area?

[steakhal]

Hey, sorry for the long delay, but better later than never, right?

See most of my comments inline.
I have one additional note, new options should be documented in the docs alongside the rest of the checker options so that it will be present at https://clang.llvm.org/docs/analyzer/checkers.html

[steakhal]

Have you looked a similar existing option?
I have a feeling they don't use space as a separator, but something else like a comma or semicolon.

[steakhal]

I think this option is not expressive enough. I'd suggest BannedFunctions or DisallowedFunctions instead.

[steakhal]

If this checker base is hidden, would this option appear in the checker help?

[steakhal]

Now that we have a more generic way of setting the list of checked functions, would this and the rest of the checkers be obsolete if one would add the bcmp to the BannedFunctions list?
What if we made this part of that default list?

[steakhal]

It's nice that we have a test case for the default value of the option, but I think we should really test the case when we set something custom function name too. Would it find calls to the banned custom function? Currently not tests demonstrate this, making the added code uncovered by tests.
(That test should be done in some other file than this one where I'm making this comment)