gamesh411 created this revision.
gamesh411 added reviewers: steakhal, Szelethus, NoQ.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
gamesh411 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
The semantics of taint sinks is that if ANY of the arguments is tainted, a
warning should be emmitted. Before this change, if there were multiple
arguments that are sensitive, and if the first arg is not tainted, but any of
the noninitial are tainted, a warning is not emitted. After this change the
correct semantics is reflected in code.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114706
Files:
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
clang/test/Analysis/taint-generic.c
Index: clang/test/Analysis/taint-generic.c
===================================================================
--- clang/test/Analysis/taint-generic.c
+++ clang/test/Analysis/taint-generic.c
@@ -209,6 +209,15 @@
strncat(dst2, dst, ts); // no-warning
}
+void testTaintedBufferSizeNoninitialTaintedArg() {
+ size_t ts;
+ scanf("%zd", &ts);
+
+ const int nontainted_num = 1;
+
+ (void)calloc(nontainted_num, ts); //expected-warning {{Untrusted data is used to specify the buffer size}}
+}
+
#define AF_UNIX 1 /* local to host (pipes) */
#define AF_INET 2 /* internetwork: UDP, TCP, etc. */
#define AF_LOCAL AF_UNIX /* backward compatibility */
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -869,24 +869,26 @@
// TODO: It might make sense to run this check on demand. In some cases,
// we should check if the environment has been cleansed here. We also might
// need to know if the user was reset before these calls(seteuid).
- unsigned ArgNum = llvm::StringSwitch<unsigned>(Name)
- .Case("system", 0)
- .Case("popen", 0)
- .Case("execl", 0)
- .Case("execle", 0)
- .Case("execlp", 0)
- .Case("execv", 0)
- .Case("execvp", 0)
- .Case("execvP", 0)
- .Case("execve", 0)
- .Case("dlopen", 0)
- .Default(InvalidArgIndex);
-
- if (ArgNum == InvalidArgIndex || Call.getNumArgs() < (ArgNum + 1))
- return false;
-
- return generateReportIfTainted(Call.getArgExpr(ArgNum), MsgSanitizeSystemArgs,
- C);
+ const ArgVector SensitiveArgs = llvm::StringSwitch<ArgVector>(Name)
+ .Case("system", {0})
+ .Case("popen", {0})
+ .Case("execl", {0, 1})
+ .Case("execle", {0, 1})
+ .Case("execlp", {0, 1})
+ .Case("execv", {0, 1})
+ .Case("execvp", {0, 1})
+ .Case("execvP", {0})
+ .Case("execve", {0, 1, 2})
+ .Case("dlopen", {0})
+ .Default({});
+
+ // Find the first argument that receives a tainted value.
+ // The report is emitted as a side-effect.
+ return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+ return Idx < Call.getNumArgs() &&
+ generateReportIfTainted(Call.getArgExpr(Idx),
+ MsgSanitizeSystemArgs, C);
+ }) != SensitiveArgs.end();
}
// TODO: Should this check be a part of the CString checker?
@@ -895,40 +897,45 @@
CheckerContext &C) const {
const auto *FDecl = Call.getDecl()->getAsFunction();
// If the function has a buffer size argument, set ArgNum.
- unsigned ArgNum = InvalidArgIndex;
+ ArgVector SensitiveArgs = {};
unsigned BId = 0;
if ((BId = FDecl->getMemoryFunctionKind())) {
switch (BId) {
case Builtin::BImemcpy:
case Builtin::BImemmove:
case Builtin::BIstrncpy:
- ArgNum = 2;
+ SensitiveArgs = {2};
break;
case Builtin::BIstrndup:
- ArgNum = 1;
+ SensitiveArgs = {1};
break;
default:
break;
}
}
- if (ArgNum == InvalidArgIndex) {
+ if (SensitiveArgs.empty()) {
using CCtx = CheckerContext;
if (CCtx::isCLibraryFunction(FDecl, "malloc") ||
- CCtx::isCLibraryFunction(FDecl, "calloc") ||
CCtx::isCLibraryFunction(FDecl, "alloca"))
- ArgNum = 0;
+ SensitiveArgs = {0};
+ else if (CCtx::isCLibraryFunction(FDecl, "calloc"))
+ SensitiveArgs = {0, 1};
else if (CCtx::isCLibraryFunction(FDecl, "memccpy"))
- ArgNum = 3;
+ SensitiveArgs = {3};
else if (CCtx::isCLibraryFunction(FDecl, "realloc"))
- ArgNum = 1;
+ SensitiveArgs = {1};
else if (CCtx::isCLibraryFunction(FDecl, "bcopy"))
- ArgNum = 2;
+ SensitiveArgs = {2};
}
- return ArgNum != InvalidArgIndex && Call.getNumArgs() > ArgNum &&
- generateReportIfTainted(Call.getArgExpr(ArgNum), MsgTaintedBufferSize,
- C);
+ // Find the first argument that receives a tainted value.
+ // The report is emitted as a side-effect.
+ return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+ return Idx < Call.getNumArgs() &&
+ generateReportIfTainted(Call.getArgExpr(Idx),
+ MsgTaintedBufferSize, C);
+ }) != SensitiveArgs.end();
}
bool GenericTaintChecker::checkCustomSinks(const CallEvent &Call,
@@ -939,16 +946,15 @@
return false;
const auto &Value = It->second;
- const GenericTaintChecker::ArgVector &Args = Value.second;
- for (unsigned ArgNum : Args) {
- if (ArgNum >= Call.getNumArgs())
- continue;
-
- if (generateReportIfTainted(Call.getArgExpr(ArgNum), MsgCustomSink, C))
- return true;
- }
-
- return false;
+ const ArgVector &SensitiveArgs = Value.second;
+
+ // Find the first argument that receives a tainted value.
+ // The report is emitted as a side-effect.
+ return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+ return Idx < Call.getNumArgs() &&
+ generateReportIfTainted(Call.getArgExpr(Idx), MsgCustomSink,
+ C);
+ }) != SensitiveArgs.end();
}
void ento::registerGenericTaintChecker(CheckerManager &Mgr) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
