[clang][analyzer] Mark lstrcatA, lstrcpyA, and _strdup as insecure on Windows in SecuritySyntaxChecker
#164183
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ure on Windows in `SecuritySyntaxChecker`
|
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 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
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Stefan Milenkovic (stemil01) ChangesAddresses issue #103038. Some of the functions mentioned in that issue are already covered or do not exist under the specified names. Therefore, this patch adds only the three functions listed above. There is a separate commit for Full diff: https://github.com/llvm/llvm-project/pull/164183.diff 2 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index ffae3b9310979..547a6c9360fee 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -869,6 +869,16 @@ def getpw : Checker<"getpw">,
Dependencies<[SecuritySyntaxChecker]>,
Documentation<HasDocumentation>;
+def lstrcatA : Checker<"lstrcatA">,
+ HelpText<"Warn on uses of the 'lstrcatA' function">,
+ Dependencies<[SecuritySyntaxChecker]>,
+ Documentation<HasDocumentation>;
+
+def lstrcpyA : Checker<"lstrcpyA">,
+ HelpText<"Warn on uses of the 'lstrcpyA' function">,
+ Dependencies<[SecuritySyntaxChecker]>,
+ Documentation<HasDocumentation>;
+
def mktemp : Checker<"mktemp">,
HelpText<"Warn on uses of the 'mktemp' function">,
Dependencies<[SecuritySyntaxChecker]>,
@@ -890,6 +900,11 @@ def strcpy : Checker<"strcpy">,
Dependencies<[SecuritySyntaxChecker]>,
Documentation<HasDocumentation>;
+def strdup : Checker<"strdup">,
+ HelpText<"Warn on uses of the '_strdup' function">,
+ Dependencies<[SecuritySyntaxChecker]>,
+ Documentation<HasDocumentation>;
+
def vfork : Checker<"vfork">,
HelpText<"Warn on uses of the 'vfork' function">,
Dependencies<[SecuritySyntaxChecker]>,
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
index 5e75c1c4a3abd..c97f4fb18e1c6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -10,14 +10,19 @@
//
//===----------------------------------------------------------------------===//
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
#include "clang/AST/StmtVisitor.h"
+#include "clang/AST/TypeBase.h"
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
-#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/Support/raw_ostream.h"
@@ -40,9 +45,12 @@ struct ChecksFilter {
bool check_bzero = false;
bool check_gets = false;
bool check_getpw = false;
+ bool check_lstrcatA = false;
+ bool check_lstrcpyA = false;
bool check_mktemp = false;
bool check_mkstemp = false;
bool check_strcpy = false;
+ bool check_strdup = false;
bool check_DeprecatedOrUnsafeBufferHandling = false;
bool check_rand = false;
bool check_vfork = false;
@@ -55,9 +63,12 @@ struct ChecksFilter {
CheckerNameRef checkName_bzero;
CheckerNameRef checkName_gets;
CheckerNameRef checkName_getpw;
+ CheckerNameRef checkName_lstrcatA;
+ CheckerNameRef checkName_lstrcpyA;
CheckerNameRef checkName_mktemp;
CheckerNameRef checkName_mkstemp;
CheckerNameRef checkName_strcpy;
+ CheckerNameRef checkName_strdup;
CheckerNameRef checkName_DeprecatedOrUnsafeBufferHandling;
CheckerNameRef checkName_rand;
CheckerNameRef checkName_vfork;
@@ -104,9 +115,12 @@ class WalkAST : public StmtVisitor<WalkAST> {
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_lstrcatA(const CallExpr *CE, const FunctionDecl *FD);
+ void checkCall_lstrcpyA(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_strdup(const CallExpr *CE, const FunctionDecl *FD);
void checkCall_strcat(const CallExpr *CE, const FunctionDecl *FD);
void checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE,
const FunctionDecl *FD);
@@ -150,12 +164,15 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
.Case("bzero", &WalkAST::checkCall_bzero)
.Case("gets", &WalkAST::checkCall_gets)
.Case("getpw", &WalkAST::checkCall_getpw)
+ .Case("lstrcatA", &WalkAST::checkCall_lstrcatA)
+ .Case("lstrcpyA", &WalkAST::checkCall_lstrcpyA)
.Case("mktemp", &WalkAST::checkCall_mktemp)
.Case("mkstemp", &WalkAST::checkCall_mkstemp)
.Case("mkdtemp", &WalkAST::checkCall_mkstemp)
.Case("mkstemps", &WalkAST::checkCall_mkstemp)
.Cases({"strcpy", "__strcpy_chk"}, &WalkAST::checkCall_strcpy)
.Cases({"strcat", "__strcat_chk"}, &WalkAST::checkCall_strcat)
+ .Case("_strdup", &WalkAST::checkCall_strdup)
.Cases({"sprintf", "vsprintf", "scanf", "wscanf", "fscanf", "fwscanf",
"vscanf", "vwscanf", "vfscanf", "vfwscanf"},
&WalkAST::checkDeprecatedOrUnsafeBufferHandling)
@@ -542,6 +559,145 @@ void WalkAST::checkCall_getpw(const CallExpr *CE, const FunctionDecl *FD) {
CELoc, CE->getCallee()->getSourceRange());
}
+//===----------------------------------------------------------------------===//
+// Check: Any use of 'lstrcatA' is insecure.
+//
+// CWE-119: Improper Restriction of Operations within
+// the Bounds of a Memory Buffer
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_lstrcatA(const CallExpr *CE, const FunctionDecl *FD) {
+ if (!filter.check_lstrcatA)
+ return;
+
+ // Verify that the operating system is Windows
+ if (!FD->getASTContext().getTargetInfo().getTriple().isOSWindows())
+ return;
+
+ // Checking the type of the arguments
+ const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
+ if (!FPT)
+ return;
+
+ // Verify the function takes two arguments, three in the _chk version.
+ int numArgs = FPT->getNumParams();
+ if (numArgs != 2)
+ return;
+
+ // Verify the type for both arguments.
+ for (int i = 0; i < 2; i++) {
+ // Verify that the arguments are pointers.
+ const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>();
+ if (!PT)
+ return;
+
+ // Verify that the argument is a 'char*'.
+ if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().CharTy)
+ return;
+ }
+ // Second argument has to have 'const' qualifier
+ if (!FPT->getParamType(1)
+ ->getAs<PointerType>()
+ ->getPointeeType()
+ .isConstQualified())
+ return;
+
+ // Verify that the function returns 'char *'
+ const PointerType *PTReturnType = FPT->getReturnType()->getAs<PointerType>();
+ if (!PTReturnType)
+ return;
+ if (PTReturnType->getPointeeType().getUnqualifiedType() !=
+ BR.getContext().CharTy)
+ return;
+
+ // Issue a warning.
+ PathDiagnosticLocation CELoc =
+ PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+ BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
+ "Potential insecure memory buffer bounds restriction in "
+ "call 'lstrcatA'",
+ "Security",
+ "Call to function 'lstrcatA' is insecure as it does not "
+ "provide bounding of the memory buffer. Consider "
+ "using 'StringCchCat' instead. CWE-119.",
+ CELoc, CE->getCallee()->getSourceRange());
+}
+
+//===----------------------------------------------------------------------===//
+// Check: Any use of 'lstrcpyA' is insecure.
+//
+// CWE-119: Improper Restriction of Operations within
+// the Bounds of a Memory Buffer
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_lstrcpyA(const CallExpr *CE, const FunctionDecl *FD) {
+ if (!filter.check_lstrcpyA)
+ return;
+
+ // Verify that the operating system is Windows
+ if (!FD->getASTContext().getTargetInfo().getTriple().isOSWindows())
+ return;
+
+ // Checking the type of the arguments
+ const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
+ if (!FPT)
+ return;
+
+ // Verify the function takes two arguments, three in the _chk version.
+ int numArgs = FPT->getNumParams();
+ if (numArgs != 2)
+ return;
+
+ // Verify the type for both arguments.
+ for (int i = 0; i < 2; i++) {
+ // Verify that the arguments are pointers.
+ const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>();
+ if (!PT)
+ return;
+
+ // Verify that the argument is a 'char*'.
+ if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().CharTy)
+ return;
+ }
+ // Second argument has to have 'const' qualifier
+ if (!FPT->getParamType(1)
+ ->getAs<PointerType>()
+ ->getPointeeType()
+ .isConstQualified())
+ return;
+
+ // Verify that the function returns 'char *'
+ const PointerType *PTReturnType = FPT->getReturnType()->getAs<PointerType>();
+ if (!PTReturnType)
+ return;
+ if (PTReturnType->getPointeeType().getUnqualifiedType() !=
+ BR.getContext().CharTy)
+ return;
+
+ const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+ *Source = CE->getArg(1)->IgnoreImpCasts();
+
+ if (const auto *Array = dyn_cast<ConstantArrayType>(Target->getType())) {
+ uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+ if (const auto *String = dyn_cast<StringLiteral>(Source)) {
+ if (ArraySize >= String->getLength() + 1)
+ return;
+ }
+ }
+
+ // Issue a warning.
+ PathDiagnosticLocation CELoc =
+ PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+ BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
+ "Potential insecure memory buffer bounds restriction in "
+ "call 'lstrcpyA'",
+ "Security",
+ "Call to function 'lstrcpyA' is insecure as it does not "
+ "provide bounding of the memory buffer. Consider "
+ "using StringCchCopy instead. CWE-119.",
+ CELoc, CE->getCallee()->getSourceRange());
+}
+
//===----------------------------------------------------------------------===//
// Check: Any use of 'mktemp' is insecure. It is obsoleted by mkstemp().
// CWE-377: Insecure Temporary File
@@ -733,6 +889,60 @@ void WalkAST::checkCall_strcat(const CallExpr *CE, const FunctionDecl *FD) {
CELoc, CE->getCallee()->getSourceRange());
}
+// TODO: Add the CWE number if it exists for this?
+//===----------------------------------------------------------------------===//
+// Check: '_strdup' should not be used.
+//
+// CWE-690: Unchecked Return Value to NULL Pointer Dereference
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_strdup(const CallExpr *CE, const FunctionDecl *FD) {
+ if (!filter.check_strdup)
+ return;
+
+ // Verify that the operating system is Windows
+ if (!FD->getASTContext().getTargetInfo().getTriple().isOSWindows())
+ return;
+
+ const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
+ if (!FPT)
+ return;
+
+ // Verify the function takes one argument
+ int numArgs = FPT->getNumParams();
+ if (numArgs != 1)
+ return;
+
+ // Verify that the argument is a pointer
+ const PointerType *PT = FPT->getParamType(0)->getAs<PointerType>();
+ if (!PT)
+ return;
+
+ // Verify that the argument is a 'const char*'.
+ if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().CharTy)
+ return;
+ if (!PT->getPointeeType().isConstQualified())
+ return;
+
+ // Verify that the function returns 'char *'
+ const PointerType *PTReturnType = FPT->getReturnType()->getAs<PointerType>();
+ if (!PTReturnType)
+ return;
+ if (PTReturnType->getPointeeType().getUnqualifiedType() !=
+ BR.getContext().CharTy)
+ return;
+
+ // Issue a warning.
+ PathDiagnosticLocation CELoc =
+ PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+ BR.EmitBasicReport(AC->getDecl(), filter.checkName_strdup,
+ "Potential unfreed memory from call of '_strdup'",
+ "Security",
+ "Call to function '_strdup' allocates memory on heap "
+ "and requires to be freed manually. CWE-690.",
+ CELoc, CE->getCallee()->getSourceRange());
+}
+
//===----------------------------------------------------------------------===//
// Check: Any use of 'sprintf', 'vsprintf', 'scanf', 'wscanf', 'fscanf',
// 'fwscanf', 'vscanf', 'vwscanf', 'vfscanf', 'vfwscanf', 'sscanf',
@@ -1106,9 +1316,12 @@ REGISTER_CHECKER(bcopy)
REGISTER_CHECKER(bzero)
REGISTER_CHECKER(gets)
REGISTER_CHECKER(getpw)
+REGISTER_CHECKER(lstrcatA)
+REGISTER_CHECKER(lstrcpyA)
REGISTER_CHECKER(mkstemp)
REGISTER_CHECKER(mktemp)
REGISTER_CHECKER(strcpy)
+REGISTER_CHECKER(strdup)
REGISTER_CHECKER(rand)
REGISTER_CHECKER(vfork)
REGISTER_CHECKER(FloatLoopCounter)
|
Author
|
@haoNoQ @Xazax-hun @steakhal |
stemil01
changed the title
_lstrcatA, _strcpyA, and _strdup as insecure on Windows in SecuritySyntaxCheckerlstrcatA, lstrcpyA, and _strdup as insecure on Windows in SecuritySyntaxChecker
steakhal
requested changes
Nov 18, 2025
Contributor
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
Hey, thanks for your PR and your patience.
Feel free to ping us weekly if we don't reply.
We only accept patches with tests and documentation for public facing changes - such as this one implementing some new feature.
Wrt. the code, I could see that there is not much code reuse. I wonder if we could apply some engineering practices to help with that. However, given that the existing parts of this checker also lacked good practices, I don't think I'd push back here.
Comment on lines
+651
to
+661
| // Verify the type for both arguments. | ||
| for (int i = 0; i < 2; i++) { | ||
| // Verify that the arguments are pointers. | ||
| const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>(); | ||
| if (!PT) | ||
| return; | ||
|
|
||
| // Verify that the argument is a 'char*'. | ||
| if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().CharTy) | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Peronally, I would just declare a lambda with the checks and have an if (!paramPointsToChar(0) || !paramPointsToChar(1)) return; instead of using a for loop.
Maybe just pass a type to the lambda instead to have it more generic.
Comment on lines
+669
to
+675
| // Verify that the function returns 'char *' | ||
| const PointerType *PTReturnType = FPT->getReturnType()->getAs<PointerType>(); | ||
| if (!PTReturnType) | ||
| return; | ||
| if (PTReturnType->getPointeeType().getUnqualifiedType() != | ||
| BR.getContext().CharTy) | ||
| return; |
Contributor
There was a problem hiding this comment.
You could also use the aforementioned lambda here to check this.
Comment on lines
+677
to
+678
| const auto *Target = CE->getArg(0)->IgnoreImpCasts(), | ||
| *Source = CE->getArg(1)->IgnoreImpCasts(); |
Contributor
There was a problem hiding this comment.
I think most of us here prefer separate declarations.
| PathDiagnosticLocation CELoc = | ||
| PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); | ||
| BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy, | ||
| "Potential insecure memory buffer bounds restriction in " |
Contributor
There was a problem hiding this comment.
I'm not sure if restriction is the right word here.
| PathDiagnosticLocation CELoc = | ||
| PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); | ||
| BR.EmitBasicReport(AC->getDecl(), filter.checkName_strdup, | ||
| "Potential unfreed memory from call of '_strdup'", |
Contributor
There was a problem hiding this comment.
I think in other checkers we refer to this by unreleased memory.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses issue #103038.
Some of the functions mentioned in that issue are already covered or do not exist under the specified names. Therefore, this patch adds only the three functions listed above.
There is a separate commit for
clang-formatstyle changes since they seem to be inconsistent with the rest ofCheckers.tdfile.PR #164184 addresses the other part of the issue, which is making the list of insecure functions extensible.