-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang-tidy] New bugprone-unsafe-format-string check #168691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,7 @@ add_clang_library(clangTidyBugproneModule STATIC | |
| UnhandledSelfAssignmentCheck.cpp | ||
| UniquePtrArrayMismatchCheck.cpp | ||
| UnsafeFunctionsCheck.cpp | ||
| UnsafeFormatStringCheck.cpp | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
| UnusedLocalNonTrivialVariableCheck.cpp | ||
| UnusedRaiiCheck.cpp | ||
| UnusedReturnValueCheck.cpp | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,153 @@ | ||||||
| //===--- UnsafeFormatStringCheck.cpp - clang-tidy -----------------------===// | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| // | ||||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||
| // See https://llvm.org/LICENSE.txt for license information. | ||||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||
| // | ||||||
| //===----------------------------------------------------------------------===// | ||||||
|
|
||||||
| #include "UnsafeFormatStringCheck.h" | ||||||
| #include "clang/ASTMatchers/ASTMatchFinder.h" | ||||||
| #include "llvm/Support/ConvertUTF.h" | ||||||
|
|
||||||
| using namespace clang::ast_matchers; | ||||||
|
|
||||||
| namespace clang::tidy::bugprone { | ||||||
|
|
||||||
| UnsafeFormatStringCheck::UnsafeFormatStringCheck(StringRef Name, | ||||||
| ClangTidyContext *Context) | ||||||
| : ClangTidyCheck(Name, Context) {} | ||||||
|
|
||||||
| void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { | ||||||
| // Matches sprintf and scanf family functions in std namespace in C++ and | ||||||
| // globally in C. | ||||||
| auto VulnerableFunctions = | ||||||
| hasAnyName("sprintf", "vsprintf", "scanf", "fscanf", "sscanf", "vscanf", | ||||||
| "vfscanf", "vsscanf", "wscanf", "fwscanf", "swscanf", | ||||||
| "vwscanf", "vfwscanf", "vswscanf"); | ||||||
|
Comment on lines
+25
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This list should be converted into an option in case users use custom printf/scanf-like functions |
||||||
| Finder->addMatcher( | ||||||
| callExpr(callee(functionDecl(VulnerableFunctions, | ||||||
| anyOf(isInStdNamespace(), | ||||||
| hasParent(translationUnitDecl())))), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||||||
| anyOf(hasArgument(0, stringLiteral().bind("format")), | ||||||
| hasArgument(1, stringLiteral().bind("format")))) | ||||||
| .bind("call"), | ||||||
| this); | ||||||
| } | ||||||
|
|
||||||
| void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { | ||||||
| const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); | ||||||
| const auto *Format = Result.Nodes.getNodeAs<StringLiteral>("format"); | ||||||
|
|
||||||
| if (!Call || !Format) | ||||||
| return; | ||||||
|
Comment on lines
+42
to
+43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We expect matchers to work, no need for this (or you can place assert).
Suggested change
|
||||||
|
|
||||||
| std::string FormatString; | ||||||
| if (Format->getCharByteWidth() == 1) { | ||||||
| FormatString = Format->getString().str(); | ||||||
| } else if (Format->getCharByteWidth() == 2) { | ||||||
| // Handle wide strings by converting to narrow string for analysis | ||||||
| convertUTF16ToUTF8String(Format->getBytes(), FormatString); | ||||||
| } else if (Format->getCharByteWidth() == 4) { | ||||||
| // Handle wide strings by converting to narrow string for analysis | ||||||
| convertUTF32ToUTF8String(Format->getBytes(), FormatString); | ||||||
| } | ||||||
|
|
||||||
| const auto *Callee = cast<FunctionDecl>(Call->getCalleeDecl()); | ||||||
| StringRef FunctionName = Callee->getName(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| bool IsScanfFamily = FunctionName.contains("scanf"); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| if (!hasUnboundedStringSpecifier(FormatString, IsScanfFamily)) | ||||||
| return; | ||||||
|
|
||||||
| auto Diag = | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to declare variable |
||||||
| diag( | ||||||
| Call->getBeginLoc(), | ||||||
| IsScanfFamily | ||||||
| ? "format specifier '%%s' without field width may cause buffer " | ||||||
| "overflow; consider using '%%Ns' where N limits input length" | ||||||
| : "format specifier '%%s' without precision may cause buffer " | ||||||
| "overflow; consider using '%%.Ns' where N limits output length") | ||||||
| << Call->getSourceRange(); | ||||||
| } | ||||||
|
|
||||||
| bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef Fmt, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is part of this was copied somewhere from clang codebase or purely hand-crafted? |
||||||
| bool IsScanfFamily) { | ||||||
| size_t Pos = 0; | ||||||
| size_t N = Fmt.size(); | ||||||
| while ((Pos = Fmt.find('%', Pos)) != StringRef::npos) { | ||||||
| if (Pos + 1 >= N) | ||||||
| break; | ||||||
|
|
||||||
| // Skip %% | ||||||
| if (Fmt[Pos + 1] == '%') { | ||||||
| Pos += 2; | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| size_t SpecPos = Pos + 1; | ||||||
|
|
||||||
| // Skip flags | ||||||
| while (SpecPos < N && | ||||||
| (Fmt[SpecPos] == '-' || Fmt[SpecPos] == '+' || Fmt[SpecPos] == ' ' || | ||||||
| Fmt[SpecPos] == '#' || Fmt[SpecPos] == '0')) { | ||||||
| SpecPos++; | ||||||
| } | ||||||
|
|
||||||
| // Check for field width | ||||||
| bool HasFieldWidth = false; | ||||||
| if (SpecPos < N && Fmt[SpecPos] == '*') { | ||||||
| HasFieldWidth = true; | ||||||
| SpecPos++; | ||||||
| } else { | ||||||
| while (SpecPos < N && isdigit(Fmt[SpecPos])) { | ||||||
| HasFieldWidth = true; | ||||||
| SpecPos++; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Check for precision | ||||||
| bool HasPrecision = false; | ||||||
| if (SpecPos < N && Fmt[SpecPos] == '.') { | ||||||
| SpecPos++; | ||||||
| if (SpecPos < N && Fmt[SpecPos] == '*') { | ||||||
| HasPrecision = true; | ||||||
| SpecPos++; | ||||||
| } else { | ||||||
| while (SpecPos < N && isdigit(Fmt[SpecPos])) { | ||||||
| HasPrecision = true; | ||||||
| SpecPos++; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Skip length modifiers | ||||||
| while (SpecPos < N && (Fmt[SpecPos] == 'h' || Fmt[SpecPos] == 'l' || | ||||||
| Fmt[SpecPos] == 'L' || Fmt[SpecPos] == 'z' || | ||||||
| Fmt[SpecPos] == 'j' || Fmt[SpecPos] == 't')) { | ||||||
| SpecPos++; | ||||||
| } | ||||||
|
|
||||||
| // Check for 's' specifier | ||||||
| if (SpecPos < N && Fmt[SpecPos] == 's') { | ||||||
| if (IsScanfFamily) { | ||||||
| // For scanf family, field width provides protection | ||||||
| if (!HasFieldWidth) { | ||||||
| return true; | ||||||
| } | ||||||
| } else { | ||||||
| // For sprintf family, only precision provides protection | ||||||
| if (!HasPrecision) { | ||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Pos = SpecPos + 1; | ||||||
| } | ||||||
|
|
||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| } // namespace clang::tidy::bugprone | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,34 @@ | ||||||
| //===--- UnsafeFormatStringCheck.h - clang-tidy ---------------*- C++ -*-===// | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| // | ||||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||
| // See https://llvm.org/LICENSE.txt for license information. | ||||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||
| // | ||||||
| //===----------------------------------------------------------------------===// | ||||||
|
|
||||||
| #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFORMATSTRINGCHECK_H | ||||||
| #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFORMATSTRINGCHECK_H | ||||||
|
|
||||||
| #include "../ClangTidyCheck.h" | ||||||
|
|
||||||
| namespace clang::tidy::bugprone { | ||||||
|
|
||||||
| /// Detects usage of vulnerable format string functions with unbounded %s | ||||||
| /// specifiers that can cause buffer overflows. | ||||||
| /// | ||||||
| /// For the user-facing documentation see: | ||||||
| /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-format-string.html | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| class UnsafeFormatStringCheck : public ClangTidyCheck { | ||||||
| public: | ||||||
| UnsafeFormatStringCheck(StringRef Name, ClangTidyContext *Context); | ||||||
| void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||||||
| void check(const ast_matchers::MatchFinder::MatchResult &Result) override; | ||||||
|
|
||||||
| private: | ||||||
| bool hasUnboundedStringSpecifier(StringRef Fmt, bool IsScanfFamily); | ||||||
| std::string getSafeAlternative(StringRef FunctionName); | ||||||
| }; | ||||||
|
|
||||||
| } // namespace clang::tidy::bugprone | ||||||
|
|
||||||
| #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFORMATSTRINGCHECK_H | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,73 @@ | ||||||||||
| .. title:: clang-tidy - bugprone-unsafe-format-string | ||||||||||
|
|
||||||||||
| bugprone-unsafe-format-string | ||||||||||
| ============================== | ||||||||||
|
Comment on lines
+3
to
+4
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
|
||||||||||
| Detects usage of vulnerable format string functions with unbounded ``%s`` | ||||||||||
| specifiers that can cause buffer overflows. | ||||||||||
|
|
||||||||||
| The check identifies calls to format string functions like ``sprintf``, ``scanf``, | ||||||||||
| and their variants that use ``%s`` format specifiers without proper limits. | ||||||||||
| This can lead to buffer overflow vulnerabilities when the input string is longer | ||||||||||
| than the destination buffer. | ||||||||||
|
|
||||||||||
| Format Specifier Behavior | ||||||||||
| -------------------------- | ||||||||||
|
Comment on lines
+14
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
|
||||||||||
| The check distinguishes between different function families: | ||||||||||
|
|
||||||||||
| **scanf family functions**: Field width limits input length | ||||||||||
| - ``%s`` - unsafe (no limit) | ||||||||||
| - ``%99s`` - safe (reads at most 99 characters) | ||||||||||
|
|
||||||||||
| **sprintf family functions**: Precision limits output length | ||||||||||
| - ``%s`` - unsafe (no limit) | ||||||||||
| - ``%99s`` - unsafe (minimum width, no maximum) | ||||||||||
| - ``%.99s`` - safe (outputs at most 99 characters) | ||||||||||
| - ``%10.99s`` - safe (minimum 10 chars, maximum 99 chars) | ||||||||||
|
|
||||||||||
| Examples | ||||||||||
| -------- | ||||||||||
|
|
||||||||||
| .. code-block:: c | ||||||||||
| char buffer[100]; | ||||||||||
| const char* input = "user input"; | ||||||||||
| // Unsafe sprintf usage | ||||||||||
| sprintf(buffer, "%s", input); // No limit | ||||||||||
| sprintf(buffer, "%99s", input); // Field width is minimum, not maximum | ||||||||||
| // Safe sprintf usage | ||||||||||
| sprintf(buffer, "%.99s", input); // Precision limits to 99 chars | ||||||||||
| sprintf(buffer, "%10.99s", input); // Min 10, max 99 chars | ||||||||||
| // Unsafe scanf usage | ||||||||||
| scanf("%s", buffer); // No limit | ||||||||||
| // Safe scanf usage | ||||||||||
| scanf("%99s", buffer); // Field width limits to 99 chars | ||||||||||
| // Safe alternative: use safer functions | ||||||||||
| snprintf(buffer, sizeof(buffer), "%s", input); | ||||||||||
| Checked Functions | ||||||||||
| ----------------- | ||||||||||
|
|
||||||||||
| The check detects unsafe format strings in these functions: | ||||||||||
|
|
||||||||||
| **sprintf family** (precision ``.N`` provides safety): | ||||||||||
| * ``sprintf``, ``vsprintf`` | ||||||||||
|
|
||||||||||
| **scanf family** (field width ``N`` provides safety): | ||||||||||
| * ``scanf``, ``fscanf``, ``sscanf`` | ||||||||||
| * ``vscanf``, ``vfscanf``, ``vsscanf`` | ||||||||||
| * ``wscanf``, ``fwscanf``, ``swscanf`` | ||||||||||
| * ``vwscanf``, ``vfwscanf``, ``vswscanf`` | ||||||||||
|
|
||||||||||
| Recommendations | ||||||||||
| --------------- | ||||||||||
|
|
||||||||||
| * For ``sprintf`` family: Use precision specifiers (``%.Ns``) or ``snprintf`` | ||||||||||
| * For ``scanf`` family: Use field width specifiers (``%Ns``) | ||||||||||
| * Consider using safer string handling functions when possible | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| #pragma clang system_header | ||
|
|
||
| #ifdef __cplusplus | ||
| #define restrict /*restrict*/ | ||
| #endif | ||
|
|
||
| #ifndef __cplusplus | ||
| typedef __WCHAR_TYPE__ wchar_t; | ||
| #endif | ||
|
|
||
| typedef __typeof(sizeof(int)) size_t; | ||
| typedef long long __int64_t; | ||
| typedef __int64_t __darwin_off_t; | ||
| typedef __darwin_off_t fpos_t; | ||
| typedef int off_t; | ||
| typedef long ssize_t; | ||
|
|
||
| typedef struct _FILE FILE; | ||
|
|
||
| extern FILE *stdin; | ||
| extern FILE *stdout; | ||
| extern FILE *stderr; | ||
|
|
||
| typedef __builtin_va_list va_list; | ||
| #define va_start(ap, param) __builtin_va_start(ap, param) | ||
| #define va_end(ap) __builtin_va_end(ap) | ||
| #define va_arg(ap, type) __builtin_va_arg(ap, type) | ||
| #define va_copy(dst, src) __builtin_va_copy(dst, src) | ||
|
|
||
|
|
||
| #ifdef __cplusplus | ||
| namespace std { | ||
| #endif | ||
| extern int fscanf ( FILE *restrict stream, const char *restrict format, ... ); | ||
| extern int scanf ( const char *restrict format, ... ); | ||
| extern int sscanf ( const char *restrict s, const char *restrict format, ...); | ||
| extern int vscanf( const char *restrict format, va_list vlist ); | ||
| extern int vfscanf ( FILE *restrict stream, const char *restrict format, va_list arg ); | ||
|
|
||
| extern int vsscanf( const char *restrict buffer, const char *restrict format, va_list vlist ); | ||
| extern int vwscanf( const wchar_t* format, va_list vlist ); | ||
| extern int vfwscanf( FILE* stream, const wchar_t* format, va_list vlist ); | ||
| extern int vswscanf( const wchar_t* buffer, const wchar_t* format, va_list vlist ); | ||
| extern int swscanf (const wchar_t* ws, const wchar_t* format, ...); | ||
| extern int wscanf( const wchar_t *format, ... ); | ||
| extern int fwscanf( FILE *stream, const wchar_t *format, ... ); | ||
|
|
||
| extern int printf( const char* format, ... ); | ||
| extern int sprintf( char* buffer, const char* format, ... ); | ||
| extern int vsprintf (char * s, const char * format, va_list arg ); | ||
| extern int vsnprintf (char * s, size_t n, const char * format, va_list arg ); | ||
| extern int fprintf( FILE* stream, const char* format, ... ); | ||
| extern int snprintf( char* restrict buffer, size_t bufsz, | ||
| const char* restrict format, ... ); | ||
| #ifdef __cplusplus | ||
| } //namespace std { | ||
| #endif | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add newline. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be before
bugprone-unsafe-functions.