From b63d8d8f7b2d101838af992749411dd79c2ed116 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Fri, 20 Jan 2012 05:35:06 +0000 Subject: [PATCH] Implement checker that looks for calls to mktemps and friends that have fewer than 6 Xs. Implements . git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@148531 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/CheckSecuritySyntaxOnly.cpp | 104 +++++++++++++++++- lib/StaticAnalyzer/Checkers/Checkers.td | 6 + test/Analysis/security-syntax-checks.m | 22 ++++ 3 files changed, 129 insertions(+), 3 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 6f55d164a7..0798a29643 100644 --- a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -45,10 +45,12 @@ struct ChecksFilter { DefaultBool check_gets; DefaultBool check_getpw; DefaultBool check_mktemp; + DefaultBool check_mkstemp; DefaultBool check_strcpy; DefaultBool check_rand; DefaultBool check_vfork; DefaultBool check_FloatLoopCounter; + DefaultBool check_UncheckedReturn; }; class WalkAST : public StmtVisitor { @@ -86,6 +88,7 @@ public: 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 checkCall_rand(const CallExpr *CE, const FunctionDecl *FD); @@ -125,6 +128,9 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { .Case("gets", &WalkAST::checkCall_gets) .Case("getpw", &WalkAST::checkCall_getpw) .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("drand48", &WalkAST::checkCall_rand) @@ -364,13 +370,17 @@ void WalkAST::checkCall_getpw(const CallExpr *CE, const FunctionDecl *FD) { } //===----------------------------------------------------------------------===// -// Check: Any use of 'mktemp' is insecure.It is obsoleted by mkstemp(). +// Check: Any use of 'mktemp' is insecure. It is obsoleted by mkstemp(). // CWE-377: Insecure Temporary File //===----------------------------------------------------------------------===// void WalkAST::checkCall_mktemp(const CallExpr *CE, const FunctionDecl *FD) { - if (!filter.check_mktemp) + if (!filter.check_mktemp) { + // Fall back to the security check of looking for enough 'X's in the + // format string, since that is a less severe warning. + checkCall_mkstemp(CE, FD); return; + } const FunctionProtoType *FPT = dyn_cast(FD->getType().IgnoreParens()); @@ -401,6 +411,88 @@ void WalkAST::checkCall_mktemp(const CallExpr *CE, const FunctionDecl *FD) { CELoc, &R, 1); } + +//===----------------------------------------------------------------------===// +// Check: Use of 'mkstemp', 'mktemp', 'mkdtemp' should contain at least 6 X's. +//===----------------------------------------------------------------------===// + +void WalkAST::checkCall_mkstemp(const CallExpr *CE, const FunctionDecl *FD) { + if (!filter.check_mkstemp) + return; + + StringRef Name = FD->getIdentifier()->getName(); + std::pair ArgSuffix = + llvm::StringSwitch >(Name) + .Case("mktemp", std::make_pair(0,-1)) + .Case("mkstemp", std::make_pair(0,-1)) + .Case("mkdtemp", std::make_pair(0,-1)) + .Case("mkstemps", std::make_pair(0,1)) + .Default(std::make_pair(-1, -1)); + + assert(ArgSuffix.first >= 0 && "Unsupported function"); + + // Check if the number of arguments is consistent with out expectations. + unsigned numArgs = CE->getNumArgs(); + if ((signed) numArgs <= ArgSuffix.first) + return; + + const StringLiteral *strArg = + dyn_cast(CE->getArg((unsigned)ArgSuffix.first) + ->IgnoreParenImpCasts()); + + // Currently we only handle string literals. It is possible to do better, + // either by looking at references to const variables, or by doing real + // flow analysis. + if (!strArg || strArg->getCharByteWidth() != 1) + return; + + // Count the number of X's, taking into account a possible cutoff suffix. + StringRef str = strArg->getString(); + unsigned numX = 0; + unsigned n = str.size(); + + // Take into account the suffix. + unsigned suffix = 0; + if (ArgSuffix.second >= 0) { + const Expr *suffixEx = CE->getArg((unsigned)ArgSuffix.second); + llvm::APSInt Result; + if (!suffixEx->EvaluateAsInt(Result, BR.getContext())) + return; + // FIXME: Issue a warning. + if (Result.isNegative()) + return; + suffix = (unsigned) Result.getZExtValue(); + n = (n > suffix) ? n - suffix : 0; + } + + for (unsigned i = 0; i < n; ++i) + if (str[i] == 'X') ++numX; + + if (numX >= 6) + return; + + // Issue a warning. + SourceRange R = strArg->getSourceRange(); + PathDiagnosticLocation CELoc = + PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); + llvm::SmallString<512> buf; + llvm::raw_svector_ostream out(buf); + out << "Call to '" << Name << "' should have at least 6 'X's in the" + " format string to be secure (" << numX << " 'X'"; + if (numX != 1) + out << 's'; + out << " seen"; + if (suffix) { + out << ", " << suffix << " character"; + if (suffix > 1) + out << 's'; + out << " used as a suffix"; + } + out << ')'; + BR.EmitBasicReport("Insecure temporary file creation", "Security", + out.str(), CELoc, &R, 1); +} + //===----------------------------------------------------------------------===// // Check: Any use of 'strcpy' is insecure. // @@ -535,7 +627,7 @@ void WalkAST::checkCall_rand(const CallExpr *CE, const FunctionDecl *FD) { //===----------------------------------------------------------------------===// void WalkAST::checkCall_random(const CallExpr *CE, const FunctionDecl *FD) { - if (!CheckRand) + if (!CheckRand || !filter.check_rand) return; const FunctionProtoType *FTP @@ -587,6 +679,9 @@ void WalkAST::checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD) { //===----------------------------------------------------------------------===// void WalkAST::checkUncheckedReturnValue(CallExpr *CE) { + if (!filter.check_UncheckedReturn) + return; + const FunctionDecl *FD = CE->getDirectCallee(); if (!FD) return; @@ -667,9 +762,12 @@ void ento::register##name(CheckerManager &mgr) {\ REGISTER_CHECKER(gets) REGISTER_CHECKER(getpw) +REGISTER_CHECKER(mkstemp) REGISTER_CHECKER(mktemp) REGISTER_CHECKER(strcpy) REGISTER_CHECKER(rand) REGISTER_CHECKER(vfork) REGISTER_CHECKER(FloatLoopCounter) +REGISTER_CHECKER(UncheckedReturn) + diff --git a/lib/StaticAnalyzer/Checkers/Checkers.td b/lib/StaticAnalyzer/Checkers/Checkers.td index fe22e8487c..47de3a364c 100644 --- a/lib/StaticAnalyzer/Checkers/Checkers.td +++ b/lib/StaticAnalyzer/Checkers/Checkers.td @@ -209,6 +209,9 @@ let ParentPackage = InsecureAPI in { def mktemp : Checker<"mktemp">, HelpText<"Warn on uses of the 'mktemp' function">, DescFile<"CheckSecuritySyntaxOnly.cpp">; + def mkstemp : Checker<"mkstemp">, + HelpText<"Warn when 'mkstemp' is passed fewer than 6 X's in the format string">, + DescFile<"CheckSecuritySyntaxOnly.cpp">; def rand : Checker<"rand">, HelpText<"Warn on uses of the 'rand', 'random', and related functions">, DescFile<"CheckSecuritySyntaxOnly.cpp">; @@ -218,6 +221,9 @@ let ParentPackage = InsecureAPI in { def vfork : Checker<"vfork">, HelpText<"Warn on uses of the 'vfork' function">, DescFile<"CheckSecuritySyntaxOnly.cpp">; + def UncheckedReturn : Checker<"UncheckedReturn">, + HelpText<"Warn on uses of functions whose return values must be always checked">, + DescFile<"CheckSecuritySyntaxOnly.cpp">; } let ParentPackage = Security in { def FloatLoopCounter : Checker<"FloatLoopCounter">, diff --git a/test/Analysis/security-syntax-checks.m b/test/Analysis/security-syntax-checks.m index 7c0cda3c79..b392bd1ea6 100644 --- a/test/Analysis/security-syntax-checks.m +++ b/test/Analysis/security-syntax-checks.m @@ -175,3 +175,25 @@ pid_t vfork(void); void test_vfork() { vfork(); //expected-warning{{Call to function 'vfork' is insecure as it can lead to denial of service situations in the parent process.}} } + +//===----------------------------------------------------------------------=== +// mkstemp() +//===----------------------------------------------------------------------=== + +char *mkdtemp(char *template); +int mkstemps(char *template, int suffixlen); +int mkstemp(char *template); +char *mktemp(char *template); + +void test_mkstemp() { + mkstemp("XX"); // expected-warning {{Call to 'mkstemp' should have at least 6 'X's in the format string to be secure (2 'X's seen)}} + mkstemp("XXXXXX"); + mkstemp("XXXXXXX"); + mkstemps("XXXXXX", 0); + mkstemps("XXXXXX", 1); // expected-warning {{5 'X's seen}} + mkstemps("XXXXXX", 2); // expected-warning {{Call to 'mkstemps' should have at least 6 'X's in the format string to be secure (4 'X's seen, 2 characters used as a suffix)}} + mkdtemp("XX"); // expected-warning {{2 'X's seen}} + mkstemp("X"); // expected-warning {{Call to 'mkstemp' should have at least 6 'X's in the format string to be secure (1 'X' seen)}} + mkdtemp("XXXXXX"); +} + -- 2.40.0