[clang] bf5963b - [analyzer] Fix taint rule of fgets and setproctitle_init
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 14 07:56:59 PST 2022
Author: Balazs Benics
Date: 2022-02-14T16:55:55+01:00
New Revision: bf5963bf19670ea58facdf57492e147c13bb650f
URL: https://github.com/llvm/llvm-project/commit/bf5963bf19670ea58facdf57492e147c13bb650f
DIFF: https://github.com/llvm/llvm-project/commit/bf5963bf19670ea58facdf57492e147c13bb650f.diff
LOG: [analyzer] Fix taint rule of fgets and setproctitle_init
There was a typo in the rule.
`{{0}, ReturnValueIndex}` meant that the discrete index is `0` and the
variadic index is `-1`.
What we wanted instead is that both `0` and `-1` are in the discrete index
list.
Instead of this, we wanted to express that both `0` and the
`ReturnValueIndex` is in the discrete arg list.
The manual inspection revealed that `setproctitle_init` also suffered a
probably incomplete propagation rule.
Reviewed By: Szelethus, gamesh411
Differential Revision: https://reviews.llvm.org/D119129
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
clang/test/Analysis/taint-checker-callback-order-has-definition.c
clang/test/Analysis/taint-checker-callback-order-without-definition.c
clang/test/Analysis/taint-generic.c
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index 66143f78932c..d15a4659a96e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -559,7 +559,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
{{"atoll"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
{{"fgetc"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
{{"fgetln"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
- {{"fgets"}, TR::Prop({{2}}, {{0}, ReturnValueIndex})},
+ {{"fgets"}, TR::Prop({{2}}, {{0, ReturnValueIndex}})},
{{"fscanf"}, TR::Prop({{0}}, {{}, 2})},
{{"sscanf"}, TR::Prop({{0}}, {{}, 2})},
{{"getc"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
@@ -632,7 +632,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
if (TR::UntrustedEnv(C)) {
// void setproctitle_init(int argc, char *argv[], char *envp[])
GlobalCRules.push_back(
- {{{"setproctitle_init"}}, TR::Sink({{2}}, MsgCustomSink)});
+ {{{"setproctitle_init"}}, TR::Sink({{1, 2}}, MsgCustomSink)});
GlobalCRules.push_back({{"getenv"}, TR::Source({{ReturnValueIndex}})});
}
diff --git a/clang/test/Analysis/taint-checker-callback-order-has-definition.c b/clang/test/Analysis/taint-checker-callback-order-has-definition.c
index a070077004fa..cdf55e3aef41 100644
--- a/clang/test/Analysis/taint-checker-callback-order-has-definition.c
+++ b/clang/test/Analysis/taint-checker-callback-order-has-definition.c
@@ -25,11 +25,9 @@ void top(const char *fname, char *buf) {
(void)fgets(buf, 42, fp); // Trigger taint propagation.
// CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
// CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
- // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
// CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
-
+ //
// CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: -1
// CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 0
- // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 1
// CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 2
}
diff --git a/clang/test/Analysis/taint-checker-callback-order-without-definition.c b/clang/test/Analysis/taint-checker-callback-order-without-definition.c
index 962e8b259298..f2e070739e97 100644
--- a/clang/test/Analysis/taint-checker-callback-order-without-definition.c
+++ b/clang/test/Analysis/taint-checker-callback-order-without-definition.c
@@ -19,16 +19,11 @@ void top(const char *fname, char *buf) {
(void)fgets(buf, 42, fp); // Trigger taint propagation.
- // FIXME: Why is the arg index 1 prepared for taint?
- // Before the call it wasn't tainted, and it also shouldn't be tainted after the call.
-
// CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
// CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
- // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
// CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
//
// CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: -1
// CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 0
- // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 1
// CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 2
}
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index 6979c0667764..0612e1b9f98b 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -58,9 +58,11 @@ extern FILE *stdin;
#define bool _Bool
+char *getenv(const char *name);
int fscanf(FILE *restrict stream, const char *restrict format, ...);
int sprintf(char *str, const char *format, ...);
void setproctitle(const char *fmt, ...);
+void setproctitle_init(int argc, char *argv[], char *envp[]);
typedef __typeof(sizeof(int)) size_t;
// Define string functions. Use builtin for some of them. They all default to
@@ -404,3 +406,20 @@ void testConfigurationSinks(void) {
void testUnknownFunction(void (*foo)(void)) {
foo(); // no-crash
}
+
+void testProctitleFalseNegative() {
+ char flag[80];
+ fscanf(stdin, "%79s", flag);
+ char *argv[] = {"myapp", flag};
+ // FIXME: We should have a warning below: Untrusted data passed to sink.
+ setproctitle_init(1, argv, 0);
+}
+
+void testProctitle2(char *real_argv[]) {
+ char *app = getenv("APP_NAME");
+ if (!app)
+ return;
+ char *argv[] = {app, "--foobar"};
+ setproctitle_init(1, argv, 0); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+ setproctitle_init(1, real_argv, argv); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
More information about the cfe-commits
mailing list