[cfe-commits] r148531 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp lib/StaticAnalyzer/Checkers/Checkers.td test/Analysis/security-syntax-checks.m
Ted Kremenek
kremenek at apple.com
Thu Jan 19 21:35:06 PST 2012
Author: kremenek
Date: Thu Jan 19 23:35:06 2012
New Revision: 148531
URL: http://llvm.org/viewvc/llvm-project?rev=148531&view=rev
Log:
Implement checker that looks for calls to mktemps and friends that have fewer than 6 Xs. Implements <rdar://problem/6336672>.
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/test/Analysis/security-syntax-checks.m
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp?rev=148531&r1=148530&r2=148531&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp Thu Jan 19 23:35:06 2012
@@ -45,10 +45,12 @@
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<WalkAST> {
@@ -86,6 +88,7 @@
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 @@
.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 @@
}
//===----------------------------------------------------------------------===//
-// 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<FunctionProtoType>(FD->getType().IgnoreParens());
@@ -401,6 +411,88 @@
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<signed, signed> ArgSuffix =
+ llvm::StringSwitch<std::pair<signed, signed> >(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<StringLiteral>(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_random(const CallExpr *CE, const FunctionDecl *FD) {
- if (!CheckRand)
+ if (!CheckRand || !filter.check_rand)
return;
const FunctionProtoType *FTP
@@ -587,6 +679,9 @@
//===----------------------------------------------------------------------===//
void WalkAST::checkUncheckedReturnValue(CallExpr *CE) {
+ if (!filter.check_UncheckedReturn)
+ return;
+
const FunctionDecl *FD = CE->getDirectCallee();
if (!FD)
return;
@@ -667,9 +762,12 @@
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)
+
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td?rev=148531&r1=148530&r2=148531&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td Thu Jan 19 23:35:06 2012
@@ -209,6 +209,9 @@
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 @@
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">,
Modified: cfe/trunk/test/Analysis/security-syntax-checks.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/security-syntax-checks.m?rev=148531&r1=148530&r2=148531&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/security-syntax-checks.m (original)
+++ cfe/trunk/test/Analysis/security-syntax-checks.m Thu Jan 19 23:35:06 2012
@@ -175,3 +175,25 @@
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");
+}
+
More information about the cfe-commits
mailing list