https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/68607
From 143db26ffe8620c2b45eb15d331466c883bbfce0 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.kr...@ericsson.com>
Date: Mon, 9 Oct 2023 16:52:13 +0200
Subject: [PATCH 1/8] [analyzer] Removing untrusted buffer size taint warning
alpha.security.taint.TaintPropagation checker
emitted a false warning to the following code
char buf[100];
size_t size = tainted();
if (size > 100)
return;
memset(buf, 0, size); // warn: untrusted data used as buffer size
The checker does not take into consideration that the
size tainted variable is bounded.
The false warning was emmitted also for the malloc/calloc calls.
These warning (the sink) should be implemented in the
MallocChecker and CStringChecker checkers instead, where more sophisticated
handling can be done taking into consideration buffer size and integer
constraints.
---
.../Checkers/GenericTaintChecker.cpp | 49 +++++--------
.../test/Analysis/taint-diagnostic-visitor.c | 68 +++++++++----------
clang/test/Analysis/taint-generic.c | 26 ++++---
3 files changed, 67 insertions(+), 76 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index 4ceaf933d0bfc8..b949cac504eddf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -59,13 +59,6 @@ constexpr llvm::StringLiteral MsgSanitizeSystemArgs =
"Untrusted data is passed to a system call "
"(CERT/STR02-C. Sanitize data passed to complex subsystems)";
-/// Check if tainted data is used as a buffer size in strn.. functions,
-/// and allocators.
-constexpr llvm::StringLiteral MsgTaintedBufferSize =
- "Untrusted data is used to specify the buffer size "
- "(CERT/STR31-C. Guarantee that storage for strings has sufficient space "
- "for character data and the null terminator)";
-
/// Check if tainted data is used as a custom sink's parameter.
constexpr llvm::StringLiteral MsgCustomSink =
"Untrusted data is passed to a user-defined sink";
@@ -733,13 +726,23 @@ void GenericTaintChecker::initTaintRules(CheckerContext
&C) const {
{{CDF_MaybeBuiltin, {{"stpcpy"}}},
TR::Prop({{1}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"strcat"}}},
- TR::Prop({{1}}, {{0, ReturnValueIndex}})},
+ TR::Prop({{0,1}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"wcsncat"}}},
TR::Prop({{1}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"strdup"}}}, TR::Prop({{0}},
{{ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"strdupa"}}},
TR::Prop({{0}}, {{ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"wcsdup"}}}, TR::Prop({{0}},
{{ReturnValueIndex}})},
+ {{CDF_MaybeBuiltin, BI.getName(Builtin::BImemcpy)},
+ TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
+ {{CDF_MaybeBuiltin, {BI.getName(Builtin::BImemmove)}},
+ TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
+ {{CDF_MaybeBuiltin, {BI.getName(Builtin::BIstrncpy)}},
+ TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
+ {{CDF_MaybeBuiltin, {BI.getName(Builtin::BIstrndup)}},
+ TR::Prop({{0, 1}}, {{ReturnValueIndex}})},
+ {{CDF_MaybeBuiltin, {"bcopy"}},
+ TR::Prop({{0, 2}}, {{1}})},
// Sinks
{{{"system"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
@@ -753,32 +756,16 @@ void GenericTaintChecker::initTaintRules(CheckerContext
&C) const {
{{{"execvp"}}, TR::Sink({{0, 1}}, MsgSanitizeSystemArgs)},
{{{"execvpe"}}, TR::Sink({{0, 1, 2}}, MsgSanitizeSystemArgs)},
{{{"dlopen"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
- {{CDF_MaybeBuiltin, {{"malloc"}}}, TR::Sink({{0}},
MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {{"calloc"}}}, TR::Sink({{0}},
MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {{"alloca"}}}, TR::Sink({{0}},
MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {{"memccpy"}}},
- TR::Sink({{3}}, MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {{"realloc"}}},
- TR::Sink({{1}}, MsgTaintedBufferSize)},
+ // malloc, calloc, alloca, realloc, memccpy
+ // are intentionally left out as taint sinks
+ // because unconditional reporting for these functions
+ // generate many false positives.
+ // These taint sinks should be implemented in other checkers
+ // with more sophisticated sanitation heuristics.
{{{{"setproctitle"}}}, TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
{{{{"setproctitle_fast"}}},
TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
-
- // SinkProps
- {{CDF_MaybeBuiltin, BI.getName(Builtin::BImemcpy)},
- TR::SinkProp({{2}}, {{1, 2}}, {{0, ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {BI.getName(Builtin::BImemmove)}},
- TR::SinkProp({{2}}, {{1, 2}}, {{0, ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {BI.getName(Builtin::BIstrncpy)}},
- TR::SinkProp({{2}}, {{1, 2}}, {{0, ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {BI.getName(Builtin::BIstrndup)}},
- TR::SinkProp({{1}}, {{0, 1}}, {{ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDF_MaybeBuiltin, {{"bcopy"}}},
- TR::SinkProp({{2}}, {{0, 2}}, {{1}}, MsgTaintedBufferSize)}};
+ };
// `getenv` returns taint only in untrusted environments.
if (TR::UntrustedEnv(C)) {
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c
b/clang/test/Analysis/taint-diagnostic-visitor.c
index 8a7510177f3e44..e3370e64dab319 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -10,7 +10,8 @@ int scanf(const char *restrict format, ...);
int system(const char *command);
char* getenv( const char* env_var );
size_t strlen( const char* str );
-int atoi( const char* str );
+char *strcat( char *dest, const char *src );
+char* strcpy( char* dest, const char* src );
void *malloc(size_t size );
void free( void *ptr );
char *fgets(char *str, int n, FILE *stream);
@@ -53,34 +54,32 @@ void taintDiagnosticVLA(void) {
// Tests if the originated note is correctly placed even if the path is
// propagating through variables and expressions
-char *taintDiagnosticPropagation(){
- char *pathbuf;
- char *size=getenv("SIZE"); // expected-note {{Taint originated here}}
- // expected-note@-1 {{Taint propagated to the
return value}}
- if (size){ // expected-note {{Assuming 'size' is non-null}}
- // expected-note@-1 {{Taking true branch}}
- pathbuf=(char*) malloc(atoi(size)); // expected-warning{{Untrusted data is
used to specify the buffer size}}
- // expected-note@-1{{Untrusted
data is used to specify the buffer size}}
- // expected-note@-2 {{Taint
propagated to the return value}}
- return pathbuf;
+int taintDiagnosticPropagation(){
+ int res;
+ char *cmd=getenv("CMD"); // expected-note {{Taint originated here}}
+ // expected-note@-1 {{Taint propagated to the
return value}}
+ if (cmd){ // expected-note {{Assuming 'cmd' is non-null}}
+ // expected-note@-1 {{Taking true branch}}
+ res = system(cmd); // expected-warning{{Untrusted data is passed to a
system call}}
+ // expected-note@-1{{Untrusted data is passed to a
system call}}
+ return res;
}
- return 0;
+ return -1;
}
// Taint origin should be marked correctly even if there are multiple taint
// sources in the function
-char *taintDiagnosticPropagation2(){
- char *pathbuf;
+int taintDiagnosticPropagation2(){
+ int res;
char *user_env2=getenv("USER_ENV_VAR2");//unrelated taint source
- char *size=getenv("SIZE"); // expected-note {{Taint originated here}}
- // expected-note@-1 {{Taint propagated to the
return value}}
+ char *cmd=getenv("CMD"); // expected-note {{Taint originated here}}
+ // expected-note@-1 {{Taint propagated to the
return value}}
char *user_env=getenv("USER_ENV_VAR");//unrelated taint source
- if (size){ // expected-note {{Assuming 'size' is non-null}}
- // expected-note@-1 {{Taking true branch}}
- pathbuf=(char*) malloc(atoi(size)+1); // expected-warning{{Untrusted data
is used to specify the buffer size}}
- // expected-note@-1{{Untrusted
data is used to specify the buffer size}}
- // expected-note@-2 {{Taint
propagated to the return value}}
- return pathbuf;
+ if (cmd){ // expected-note {{Assuming 'cmd' is non-null}}
+ // expected-note@-1 {{Taking true branch}}
+ res = system(cmd); // expected-warning{{Untrusted data is passed to a
system call}}
+ // expected-note@-1{{Untrusted data is passed to a
system call}}
+ return res;
}
return 0;
}
@@ -95,22 +94,23 @@ void testReadStdIn(){
}
void multipleTaintSources(void) {
- int x,y,z;
- scanf("%d", &x); // expected-note {{Taint originated here}}
+ char cmd[2048],file[1024];
+ scanf ("%1022[^\n] ", cmd); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd argument}}
- scanf("%d", &y); // expected-note {{Taint originated here}}
+ scanf ("%1023[^\n]", file); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd argument}}
- scanf("%d", &z);
- int* ptr = (int*) malloc(y + x); // expected-warning {{Untrusted data is
used to specify the buffer size}}
- // expected-note@-1{{Untrusted data is used
to specify the buffer size}}
- free (ptr);
+ strcat(cmd, file);// expected-note {{Taint propagated to the 1st argument}}
+ system(cmd); // expected-warning {{Untrusted data is passed to a system
call}}
+ // expected-note@-1{{Untrusted data is passed to a system call}}
}
void multipleTaintedArgs(void) {
- int x,y;
- scanf("%d %d", &x, &y); // expected-note {{Taint originated here}}
+ char cmd[1024],file[1024], buf[2048];
+ scanf("%1022s %1023s", cmd, file); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd
argument, 3rd argument}}
- int* ptr = (int*) malloc(x + y); // expected-warning {{Untrusted data is
used to specify the buffer size}}
- // expected-note@-1{{Untrusted data is used
to specify the buffer size}}
- free (ptr);
+ strcpy(buf, cmd);// expected-note {{Taint propagated to the 1st argument}}
+ strcat(buf," ");// expected-note {{Taint propagated to the 1st argument}}
+ strcat(buf, file);// expected-note {{Taint propagated to the 1st argument}}
+ system(buf); // expected-warning {{Untrusted data is passed to a system
call}}
+ // expected-note@-1{{Untrusted data is passed to a system call}}
}
diff --git a/clang/test/Analysis/taint-generic.c
b/clang/test/Analysis/taint-generic.c
index c6a01594f15abb..78305bfbb9d35f 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -305,15 +305,19 @@ void testGets_s(void) {
void testTaintedBufferSize(void) {
size_t ts;
+ // malloc, calloc, bcopy, memcpy functions are removed as unconditional sinks
+ // from the GenericTaintChecker's default configuration,
+ // because it generated too many false positives.
+ // We would need more sophisticated handling of these reports to enable
+ // these test-cases again.
scanf("%zd", &ts);
-
- int *buf1 = (int*)malloc(ts*sizeof(int)); // expected-warning {{Untrusted
data is used to specify the buffer size}}
- char *dst = (char*)calloc(ts, sizeof(char)); //expected-warning {{Untrusted
data is used to specify the buffer size}}
- bcopy(buf1, dst, ts); // expected-warning {{Untrusted data is used to
specify the buffer size}}
- __builtin_memcpy(dst, buf1, (ts + 4)*sizeof(char)); // expected-warning
{{Untrusted data is used to specify the buffer size}}
+ int *buf1 = (int*)malloc(ts*sizeof(int)); // warn here, ts is unbounded and
tainted
+ char *dst = (char*)calloc(ts, sizeof(char)); // warn here, ts is unbounded
tainted
+ bcopy(buf1, dst, ts); // no warning here, since the size of buf1, dst equals
ts. Cannot overflow.
+ __builtin_memcpy(dst, buf1, (ts + 4)*sizeof(char)); // warn here, dst
overflows (whatever the value of ts)
// If both buffers are trusted, do not issue a warning.
- char *dst2 = (char*)malloc(ts*sizeof(char)); // expected-warning {{Untrusted
data is used to specify the buffer size}}
+ char *dst2 = (char*)malloc(ts*sizeof(char)); // warn here, ts in unbounded
strncat(dst2, dst, ts); // no-warning
}
@@ -353,7 +357,7 @@ void testStruct(void) {
sock = socket(AF_INET, SOCK_STREAM, 0);
read(sock, &tainted, sizeof(tainted));
- __builtin_memcpy(buffer, tainted.buf, tainted.length); // expected-warning
{{Untrusted data is used to specify the buffer size}}
+ clang_analyzer_isTainted_int(tainted.length); // expected-warning {{YES }}
}
void testStructArray(void) {
@@ -368,17 +372,17 @@ void testStructArray(void) {
__builtin_memset(srcbuf, 0, sizeof(srcbuf));
read(sock, &tainted[0], sizeof(tainted));
- __builtin_memcpy(dstbuf, srcbuf, tainted[0].length); // expected-warning
{{Untrusted data is used to specify the buffer size}}
+ clang_analyzer_isTainted_int(tainted[0].length); // expected-warning {{YES}}
__builtin_memset(&tainted, 0, sizeof(tainted));
read(sock, &tainted, sizeof(tainted));
- __builtin_memcpy(dstbuf, srcbuf, tainted[0].length); // expected-warning
{{Untrusted data is used to specify the buffer size}}
+ clang_analyzer_isTainted_int(tainted[0].length); // expected-warning {{YES}}
__builtin_memset(&tainted, 0, sizeof(tainted));
// If we taint element 1, we should not raise an alert on taint for element
0 or element 2
read(sock, &tainted[1], sizeof(tainted));
- __builtin_memcpy(dstbuf, srcbuf, tainted[0].length); // no-warning
- __builtin_memcpy(dstbuf, srcbuf, tainted[2].length); // no-warning
+ clang_analyzer_isTainted_int(tainted[0].length); // expected-warning {{NO}}
+ clang_analyzer_isTainted_int(tainted[2].length); // expected-warning {{NO}}
}
void testUnion(void) {
From 88f226f60649dc43553f55ced123fb3f5ae5a39b Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.kr...@ericsson.com>
Date: Tue, 10 Oct 2023 11:38:18 +0200
Subject: [PATCH 2/8] fixup!
---
clang/docs/analyzer/checkers.rst | 11 +----------
.../StaticAnalyzer/Checkers/GenericTaintChecker.cpp | 11 +++++------
2 files changed, 6 insertions(+), 16 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index dbd6d778782353..f8e465302c3d8b 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2572,13 +2572,6 @@ Further examples of injection vulnerabilities this
checker can find.
sprintf(buf, s); // warn: untrusted data used as a format string
}
- void test() {
- size_t ts;
- scanf("%zd", &ts); // 'ts' marked as tainted
- int *p = (int *)malloc(ts * sizeof(int));
- // warn: untrusted data used as buffer size
- }
-
There are built-in sources, propagations and sinks even if no external taint
configuration is provided.
@@ -2606,9 +2599,7 @@ Default propagations rules:
Default sinks:
``printf``, ``setproctitle``, ``system``, ``popen``, ``execl``, ``execle``,
- ``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``,
- ``memcpy``, ``memmove``, ``strncpy``, ``strndup``, ``malloc``, ``calloc``,
- ``alloca``, ``memccpy``, ``realloc``, ``bcopy``
+ ``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``
Please note that there are no built-in filter functions.
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index b949cac504eddf..f2182878150338 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -726,7 +726,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C)
const {
{{CDF_MaybeBuiltin, {{"stpcpy"}}},
TR::Prop({{1}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"strcat"}}},
- TR::Prop({{0,1}}, {{0, ReturnValueIndex}})},
+ TR::Prop({{0, 1}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"wcsncat"}}},
TR::Prop({{1}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {{"strdup"}}}, TR::Prop({{0}},
{{ReturnValueIndex}})},
@@ -757,11 +757,10 @@ void GenericTaintChecker::initTaintRules(CheckerContext
&C) const {
{{{"execvpe"}}, TR::Sink({{0, 1, 2}}, MsgSanitizeSystemArgs)},
{{{"dlopen"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
// malloc, calloc, alloca, realloc, memccpy
- // are intentionally left out as taint sinks
- // because unconditional reporting for these functions
- // generate many false positives.
- // These taint sinks should be implemented in other checkers
- // with more sophisticated sanitation heuristics.
+ // are intentionally not marked as taint sinks because unconditional
+ // reporting for these functions generates many false positives.
+ // These taint sinks should be implemented in other checkers with more
+ // sophisticated sanitation heuristics.
{{{{"setproctitle"}}}, TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
{{{{"setproctitle_fast"}}},
TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
From 3d6c80d6dcb40f83b407f60252e8e0a18278eac0 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.kr...@ericsson.com>
Date: Tue, 10 Oct 2023 11:44:12 +0200
Subject: [PATCH 3/8] fixup!
---
.../Checkers/GenericTaintChecker.cpp | 23 ++++++-------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index f2182878150338..7e552ca1c4cf6e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -291,14 +291,6 @@ class GenericTaintRule {
return {{}, {}, std::move(SrcArgs), std::move(DstArgs)};
}
- /// Make a rule that taints all PropDstArgs if any of PropSrcArgs is tainted.
- static GenericTaintRule
- SinkProp(ArgSet &&SinkArgs, ArgSet &&SrcArgs, ArgSet &&DstArgs,
- std::optional<StringRef> Msg = std::nullopt) {
- return {
- std::move(SinkArgs), {}, std::move(SrcArgs), std::move(DstArgs), Msg};
- }
-
/// Process a function which could either be a taint source, a taint sink, a
/// taint filter or a taint propagator.
void process(const GenericTaintChecker &Checker, const CallEvent &Call,
@@ -741,8 +733,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C)
const {
TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
{{CDF_MaybeBuiltin, {BI.getName(Builtin::BIstrndup)}},
TR::Prop({{0, 1}}, {{ReturnValueIndex}})},
- {{CDF_MaybeBuiltin, {"bcopy"}},
- TR::Prop({{0, 2}}, {{1}})},
+ {{CDF_MaybeBuiltin, {"bcopy"}}, TR::Prop({{0, 2}}, {{1}})},
// Sinks
{{{"system"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
@@ -756,15 +747,15 @@ void GenericTaintChecker::initTaintRules(CheckerContext
&C) const {
{{{"execvp"}}, TR::Sink({{0, 1}}, MsgSanitizeSystemArgs)},
{{{"execvpe"}}, TR::Sink({{0, 1, 2}}, MsgSanitizeSystemArgs)},
{{{"dlopen"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
- // malloc, calloc, alloca, realloc, memccpy
- // are intentionally not marked as taint sinks because unconditional
- // reporting for these functions generates many false positives.
- // These taint sinks should be implemented in other checkers with more
- // sophisticated sanitation heuristics.
+ // malloc, calloc, alloca, realloc, memccpy
+ // are intentionally not marked as taint sinks because unconditional
+ // reporting for these functions generates many false positives.
+ // These taint sinks should be implemented in other checkers with more
+ // sophisticated sanitation heuristics.
{{{{"setproctitle"}}}, TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
{{{{"setproctitle_fast"}}},
TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
- };
+ };
// `getenv` returns taint only in untrusted environments.
if (TR::UntrustedEnv(C)) {
From 8a4f55f0013b72bb33467792d261f39704da7414 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.kr...@ericsson.com>
Date: Fri, 26 Apr 2024 13:06:24 +0200
Subject: [PATCH 4/8] fixup!
---
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp | 3 +++
clang/test/Analysis/taint-diagnostic-visitor.c | 4 ++--
clang/test/Analysis/taint-generic.c | 12 +++++++-----
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index f1539f2733298d..29df3eddb32ba0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1306,6 +1306,9 @@ void CStringChecker::evalCopyCommon(CheckerContext &C,
const CallExpr *CE,
// If the size can be nonzero, we have to check the other arguments.
if (stateNonZeroSize) {
+ // TODO: If Size is tainted and we cannot prove that it is smaller or equal
+ // to the size of the destination buffer, then emit a warning
+ // that an attacker may provoke a buffer overflow error.
state = stateNonZeroSize;
// Ensure the destination is not null. If it is NULL there will be a
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c
b/clang/test/Analysis/taint-diagnostic-visitor.c
index e3370e64dab319..625c1db2fa7a96 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -94,7 +94,7 @@ void testReadStdIn(){
}
void multipleTaintSources(void) {
- char cmd[2048],file[1024];
+ char cmd[2048], file[1024];
scanf ("%1022[^\n] ", cmd); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd argument}}
scanf ("%1023[^\n]", file); // expected-note {{Taint originated here}}
@@ -105,7 +105,7 @@ void multipleTaintSources(void) {
}
void multipleTaintedArgs(void) {
- char cmd[1024],file[1024], buf[2048];
+ char cmd[1024], file[1024], buf[2048];
scanf("%1022s %1023s", cmd, file); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd
argument, 3rd argument}}
strcpy(buf, cmd);// expected-note {{Taint propagated to the 1st argument}}
diff --git a/clang/test/Analysis/taint-generic.c
b/clang/test/Analysis/taint-generic.c
index 78305bfbb9d35f..349cbb6db2da53 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -305,11 +305,13 @@ void testGets_s(void) {
void testTaintedBufferSize(void) {
size_t ts;
- // malloc, calloc, bcopy, memcpy functions are removed as unconditional sinks
- // from the GenericTaintChecker's default configuration,
- // because it generated too many false positives.
- // We would need more sophisticated handling of these reports to enable
- // these test-cases again.
+ // The functions malloc, calloc, bcopy and memcpy are not taint sinks in the
+ // default config of GenericTaintChecker (because that would cause too many
+ // false positives).
+ // FIXME: We should generate warnings when a value passed to these functions
+ // is tainted and _can be very large_ (because that's exploitable). This
+ // functionality probably belongs to the checkers that do more detailed
+ // modeling of these functions (MallocChecker and CStringChecker).
scanf("%zd", &ts);
int *buf1 = (int*)malloc(ts*sizeof(int)); // warn here, ts is unbounded and
tainted
char *dst = (char*)calloc(ts, sizeof(char)); // warn here, ts is unbounded
tainted
From 5d7c44a7e4b338f83f791ebc6ec3297d62983c0b Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.kr...@ericsson.com>
Date: Tue, 30 Apr 2024 17:45:26 +0200
Subject: [PATCH 5/8] Fix taint propagation in strcat
---
.../Checkers/GenericTaintChecker.cpp | 31 +++++++------------
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index c69cf91bebeb93..ccd56da2ee342f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -718,12 +718,21 @@ void GenericTaintChecker::initTaintRules(CheckerContext
&C) const {
{{CDM::CLibraryMaybeHardened, {{"stpcpy"}}},
TR::Prop({{1}}, {{0, ReturnValueIndex}})},
{{CDM::CLibraryMaybeHardened, {{"strcat"}}},
- TR::Prop({{1}}, {{0, ReturnValueIndex}})},
+ TR::Prop({{0, 1}}, {{0, ReturnValueIndex}})},
{{CDM::CLibraryMaybeHardened, {{"wcsncat"}}},
- TR::Prop({{1}}, {{0, ReturnValueIndex}})},
+ TR::Prop({{0, 1}}, {{0, ReturnValueIndex}})},
{{CDM::CLibrary, {{"strdup"}}}, TR::Prop({{0}}, {{ReturnValueIndex}})},
{{CDM::CLibrary, {{"strdupa"}}}, TR::Prop({{0}}, {{ReturnValueIndex}})},
{{CDM::CLibrary, {{"wcsdup"}}}, TR::Prop({{0}}, {{ReturnValueIndex}})},
+ {{CDM::CLibrary, BI.getName(Builtin::BImemcpy)},
+ TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
+ {{CDM::CLibrary, {BI.getName(Builtin::BImemmove)}},
+ TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
+ {{CDM::CLibrary, {BI.getName(Builtin::BIstrncpy)}},
+ TR::Prop({{1, 2}}, {{0, ReturnValueIndex}})},
+ {{CDM::CLibrary, {BI.getName(Builtin::BIstrndup)}},
+ TR::Prop({{0, 1}}, {{ReturnValueIndex}})},
+ {{CDM::CLibrary, {"bcopy"}}, TR::Prop({{0, 2}}, {{1}})},
// Sinks
{{{"system"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
@@ -745,23 +754,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext
&C) const {
// sophisticated sanitation heuristics.
{{{{"setproctitle"}}}, TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
{{{{"setproctitle_fast"}}},
- TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
-
- // SinkProps
- {{CDM::CLibraryMaybeHardened, BI.getName(Builtin::BImemcpy)},
- TR::SinkProp({{2}}, {{1, 2}}, {{0, ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDM::CLibraryMaybeHardened, {BI.getName(Builtin::BImemmove)}},
- TR::SinkProp({{2}}, {{1, 2}}, {{0, ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDM::CLibraryMaybeHardened, {BI.getName(Builtin::BIstrncpy)}},
- TR::SinkProp({{2}}, {{1, 2}}, {{0, ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDM::CLibrary, {BI.getName(Builtin::BIstrndup)}},
- TR::SinkProp({{1}}, {{0, 1}}, {{ReturnValueIndex}},
- MsgTaintedBufferSize)},
- {{CDM::CLibrary, {{"bcopy"}}},
- TR::SinkProp({{2}}, {{0, 2}}, {{1}}, MsgTaintedBufferSize)}};
+ TR::Sink({{0}, 1}, MsgUncontrolledFormatString)}
};
// `getenv` returns taint only in untrusted environments.
From c90ab8a16dc131bcb2bbf206fe755c4fc8937212 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.n...@ericsson.com>
Date: Tue, 30 Apr 2024 17:51:55 +0200
Subject: [PATCH 6/8] Add space after comma in a test
---
clang/test/Analysis/taint-diagnostic-visitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c
b/clang/test/Analysis/taint-diagnostic-visitor.c
index eaa9ef4b2942d2..db32d940e330f9 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -109,7 +109,7 @@ void multipleTaintedArgs(void) {
scanf("%1022s %1023s", cmd, file); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd
argument, 3rd argument}}
strcpy(buf, cmd);// expected-note {{Taint propagated to the 1st argument}}
- strcat(buf," ");// expected-note {{Taint propagated to the 1st argument}}
+ strcat(buf, " ");// expected-note {{Taint propagated to the 1st argument}}
strcat(buf, file);// expected-note {{Taint propagated to the 1st argument}}
system(buf); // expected-warning {{Untrusted data is passed to a system
call}}
// expected-note@-1{{Untrusted data is passed to a system call}}
From b1d2495295fde617ba122b3798a5554b31d3c66f Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.kr...@ericsson.com>
Date: Thu, 2 May 2024 14:49:57 +0200
Subject: [PATCH 7/8] fixup!
---
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp | 3 +--
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 8 ++++++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index ccd56da2ee342f..d17f5ddf070553 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -754,8 +754,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C)
const {
// sophisticated sanitation heuristics.
{{{{"setproctitle"}}}, TR::Sink({{0}, 1}, MsgUncontrolledFormatString)},
{{{{"setproctitle_fast"}}},
- TR::Sink({{0}, 1}, MsgUncontrolledFormatString)}
- };
+ TR::Sink({{0}, 1}, MsgUncontrolledFormatString)}};
// `getenv` returns taint only in untrusted environments.
if (TR::UntrustedEnv(C)) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 11651fd491f743..aa7af8327014a5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1818,8 +1818,12 @@ ProgramStateRef
MallocChecker::MallocMemAux(CheckerContext &C,
// If Size is somehow undefined at this point, this line prevents a crash.
if (Size.isUndef())
Size = UnknownVal();
-
- // Set the region's extent.
+
+ // TODO: If Size is tainted and we cannot prove that it is within
+ // reasonable bounds, emit a warning that an attacker may
+ // provoke a memory exhaustion error.
+
+ // Set the region's extent.
State = setDynamicExtent(State, RetVal.getAsRegion(),
Size.castAs<DefinedOrUnknownSVal>(), SVB);
From 6e04d02fffe77b753a72d9971925967aa2e1e747 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.n...@ericsson.com>
Date: Thu, 2 May 2024 16:36:17 +0200
Subject: [PATCH 8/8] Minor tweaks in tests
---
clang/test/Analysis/taint-diagnostic-visitor.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c
b/clang/test/Analysis/taint-diagnostic-visitor.c
index db32d940e330f9..b8b3710a7013e7 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -59,7 +59,7 @@ int taintDiagnosticPropagation(){
char *cmd=getenv("CMD"); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the
return value}}
if (cmd){ // expected-note {{Assuming 'cmd' is non-null}}
- // expected-note@-1 {{Taking true branch}}
+ // expected-note@-1 {{Taking true branch}}
res = system(cmd); // expected-warning{{Untrusted data is passed to a
system call}}
// expected-note@-1{{Untrusted data is passed to a
system call}}
return res;
@@ -99,7 +99,8 @@ void multipleTaintSources(void) {
// expected-note@-1 {{Taint propagated to the 2nd argument}}
scanf ("%1023[^\n]", file); // expected-note {{Taint originated here}}
// expected-note@-1 {{Taint propagated to the 2nd argument}}
- strcat(cmd, file);// expected-note {{Taint propagated to the 1st argument}}
+ strcat(cmd, file); // expected-note {{Taint propagated to the 1st argument}}
+ strcat(cmd, " "); // expected-note {{Taint propagated to the 1st argument}}
system(cmd); // expected-warning {{Untrusted data is passed to a system
call}}
// expected-note@-1{{Untrusted data is passed to a system call}}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
