Skip to content

Commit f90e063

Browse files
steakhalsteakhal
authored
[analyzer] Fix taint sink rules for exec-like functions (#66358)
Variadic arguments were not considered as taint sink arguments. I also decided to extend the list of exec-like functions. (Juliet CWE78_OS_Command_Injection__char_connect_socket_execl)
1 parent 1d0b7b6 commit f90e063

File tree

2 files changed

+167
-8
lines changed

2 files changed

+167
-8
lines changed

‎clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -744,12 +744,14 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
744744
// Sinks
745745
{{{"system"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
746746
{{{"popen"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
747-
{{{"execl"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
748-
{{{"execle"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
749-
{{{"execlp"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
750-
{{{"execvp"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
751-
{{{"execvP"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
752-
{{{"execve"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
747+
{{{"execl"}}, TR::Sink({{}, {0}}, MsgSanitizeSystemArgs)},
748+
{{{"execle"}}, TR::Sink({{}, {0}}, MsgSanitizeSystemArgs)},
749+
{{{"execlp"}}, TR::Sink({{}, {0}}, MsgSanitizeSystemArgs)},
750+
{{{"execv"}}, TR::Sink({{0, 1}}, MsgSanitizeSystemArgs)},
751+
{{{"execve"}}, TR::Sink({{0, 1, 2}}, MsgSanitizeSystemArgs)},
752+
{{{"fexecve"}}, TR::Sink({{0, 1, 2}}, MsgSanitizeSystemArgs)},
753+
{{{"execvp"}}, TR::Sink({{0, 1}}, MsgSanitizeSystemArgs)},
754+
{{{"execvpe"}}, TR::Sink({{0, 1, 2}}, MsgSanitizeSystemArgs)},
753755
{{{"dlopen"}}, TR::Sink({{0}}, MsgSanitizeSystemArgs)},
754756
{{CDF_MaybeBuiltin, {{"malloc"}}}, TR::Sink({{0}}, MsgTaintedBufferSize)},
755757
{{CDF_MaybeBuiltin, {{"calloc"}}}, TR::Sink({{0}}, MsgTaintedBufferSize)},

‎clang/test/Analysis/taint-generic.c

Lines changed: 159 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ void clang_analyzer_isTainted_wchar(wchar_t);
6464
void clang_analyzer_isTainted_charp(char*);
6565
void clang_analyzer_isTainted_int(int);
6666

67+
int coin();
68+
6769
int scanf(const char *restrict format, ...);
6870
char *gets(char *str);
6971
char *gets_s(char *str, rsize_t n);
@@ -118,6 +120,41 @@ void *malloc(size_t);
118120
void *calloc(size_t nmemb, size_t size);
119121
void bcopy(void *s1, void *s2, size_t n);
120122

123+
124+
// function | pathname | filename | fd | arglist | argv[] | envp[]
125+
// ===============================================================
126+
// 1 execl | X | | | X | |
127+
// 2 execle | X | | | X | | X
128+
// 3 execlp | | X | | X | |
129+
// 4 execv | X | | | | X |
130+
// 5 execve | X | | | | X | X
131+
// 6 execvp | | X | | | X |
132+
// 7 execvpe | | X | | | X | X
133+
// 8 fexecve | | | X | | X | X
134+
// ===============================================================
135+
// letter | | p | f | l | v | e
136+
//
137+
// legend:
138+
// - pathname: rel/abs path to the binary
139+
// - filename: file name searched in PATH to execute the binary
140+
// - fd: accepts a file descriptor
141+
// - arglist: accepts variadic arguments
142+
// - argv: accepts a pointer to array, denoting the new argv
143+
// - envp: accepts a pointer to array, denoting the new envp
144+
145+
int execl(const char *path, const char *arg, ...);
146+
int execle(const char *path, const char *arg, ...);
147+
int execlp(const char *file, const char *arg, ...);
148+
int execv(const char *path, char *const argv[]);
149+
int execve(const char *path, char *const argv[], char *const envp[]);
150+
int execvp(const char *file, char *const argv[]);
151+
int execvpe(const char *file, char *const argv[], char *const envp[]);
152+
int fexecve(int fd, char *const argv[], char *const envp[]);
153+
FILE *popen(const char *command, const char *type);
154+
int pclose(FILE *stream);
155+
int system(const char *command);
156+
157+
121158
typedef size_t socklen_t;
122159

123160
struct sockaddr {
@@ -224,7 +261,6 @@ void testUncontrolledFormatString(char **p) {
224261

225262
}
226263

227-
int system(const char *command);
228264
void testTaintSystemCall(void) {
229265
char buffer[156];
230266
char addr[128];
@@ -287,7 +323,6 @@ void testTaintedBufferSize(void) {
287323
#define SOCK_STREAM 1
288324
int socket(int, int, int);
289325
size_t read(int, void *, size_t);
290-
int execl(const char *, const char *, ...);
291326

292327
void testSocket(void) {
293328
int sock;
@@ -1129,6 +1164,128 @@ void testConfigurationSinks(void) {
11291164
// expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}
11301165
}
11311166

1167+
int test_exec_like_functions() {
1168+
char buf[100] = {0};
1169+
scanf("%99s", buf);
1170+
clang_analyzer_isTainted_char(buf[0]); // expected-warning {{YES}}
1171+
1172+
char *cleanArray[] = {"ENV1=V1", "ENV2=V2", NULL};
1173+
char *taintedArray[] = {buf, "ENV2=V2", NULL};
1174+
clang_analyzer_isTainted_char(taintedArray[0][0]); // expected-warning {{YES}}
1175+
clang_analyzer_isTainted_char(*(char*)taintedArray[0]); // expected-warning {{YES}}
1176+
clang_analyzer_isTainted_char(*(char*)taintedArray); // expected-warning {{NO}} We should have YES here.
1177+
// FIXME: Above the triple pointer indirection will confuse the checker,
1178+
// as we only check two levels. The results would be worse, if the tainted
1179+
// subobject ("buf") would not be at the beginning of the enclosing object,
1180+
// for the same reason.
1181+
1182+
switch (coin()) {
1183+
default: break;
1184+
// Execute `path` with all arguments after `path` until a NULL pointer
1185+
// and environment from `environ'.
1186+
case 0: return execl("path", "arg0", "arg1", "arg2", NULL); // no-warning
1187+
case 1: return execl(buf, "arg0", "arg1", "arg2", NULL); // expected-warning {{Untrusted data is passed to a system call}}
1188+
case 2: return execl("path", buf, "arg1", "arg2", NULL); // expected-warning {{Untrusted data is passed to a system call}}
1189+
case 3: return execl("path", "arg0", buf, "arg2", NULL); // expected-warning {{Untrusted data is passed to a system call}}
1190+
case 4: return execl("path", "arg0", "arg1", buf, NULL); // expected-warning {{Untrusted data is passed to a system call}}
1191+
}
1192+
1193+
switch (coin()) {
1194+
default: break;
1195+
// Execute `path` with all arguments after `PATH` until a NULL pointer,
1196+
// and the argument after that for environment.
1197+
case 0: return execle("path", "arg0", "arg1", NULL, cleanArray); // no-warning
1198+
case 1: return execle( buf, "arg0", "arg1", NULL, cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1199+
case 2: return execle("path", buf, "arg1", NULL, cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1200+
case 3: return execle("path", "arg0", buf, NULL, cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1201+
case 4: return execle("path", "arg0", "arg1", NULL, buf); // expected-warning {{Untrusted data is passed to a system call}}
1202+
case 5: return execle("path", "arg0", "arg1", NULL, taintedArray); // FIXME: We might wanna have a report here.
1203+
}
1204+
1205+
switch (coin()) {
1206+
default: break;
1207+
// Execute `file`, searching in the `PATH' environment variable if it
1208+
// contains no slashes, with all arguments after `file` until a NULL
1209+
// pointer and environment from `environ'.
1210+
case 0: return execlp("file", "arg0", "arg1", "arg2", NULL); // no-warning
1211+
case 1: return execlp( buf, "arg0", "arg1", "arg2", NULL); // expected-warning {{Untrusted data is passed to a system call}}
1212+
case 2: return execlp("file", buf, "arg1", "arg2", NULL); // expected-warning {{Untrusted data is passed to a system call}}
1213+
case 3: return execlp("file", "arg0", buf, "arg2", NULL); // expected-warning {{Untrusted data is passed to a system call}}
1214+
case 4: return execlp("file", "arg0", "arg1", buf, NULL); // expected-warning {{Untrusted data is passed to a system call}}
1215+
}
1216+
1217+
switch (coin()) {
1218+
default: break;
1219+
// Execute `path` with arguments `ARGV` and environment from `environ'.
1220+
case 0: return execv("path", /*argv=*/ cleanArray); // no-warning
1221+
case 1: return execv( buf, /*argv=*/ cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1222+
case 2: return execv("path", /*argv=*/taintedArray); // FIXME: We might wanna have a report here.
1223+
}
1224+
1225+
switch (coin()) {
1226+
default: break;
1227+
// Replace the current process, executing `path` with arguments `ARGV`
1228+
// and environment `ENVP`. `ARGV` and `ENVP` are terminated by NULL pointers.
1229+
case 0: return execve("path", /*argv=*/ cleanArray, /*envp=*/cleanArray); // no-warning
1230+
case 1: return execve( buf, /*argv=*/ cleanArray, /*envp=*/cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1231+
case 2: return execve("path", /*argv=*/taintedArray, /*envp=*/cleanArray); // FIXME: We might wanna have a report here.
1232+
case 3: return execve("path", /*argv=*/cleanArray, /*envp=*/taintedArray); // FIXME: We might wanna have a report here.
1233+
}
1234+
1235+
switch (coin()) {
1236+
default: break;
1237+
// Execute `file`, searching in the `PATH' environment variable if it
1238+
// contains no slashes, with arguments `ARGV` and environment from `environ'.
1239+
case 0: return execvp("file", /*argv=*/ cleanArray); // no-warning
1240+
case 1: return execvp( buf, /*argv=*/ cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1241+
case 2: return execvp("file", /*argv=*/taintedArray); // FIXME: We might wanna have a report here.
1242+
}
1243+
1244+
// execvpe
1245+
switch (coin()) {
1246+
default: break;
1247+
// Execute `file`, searching in the `PATH' environment variable if it
1248+
// contains no slashes, with arguments `ARGV` and environment `ENVP`.
1249+
// `ARGV` and `ENVP` are terminated by NULL pointers.
1250+
case 0: return execvpe("file", /*argv=*/ cleanArray, /*envp=*/ cleanArray); // no-warning
1251+
case 1: return execvpe( buf, /*argv=*/ cleanArray, /*envp=*/ cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1252+
case 2: return execvpe("file", /*argv=*/taintedArray, /*envp=*/ cleanArray); // FIXME: We might wanna have a report here.
1253+
case 3: return execvpe("file", /*argv=*/ cleanArray, /*envp=*/taintedArray); // FIXME: We might wanna have a report here.
1254+
}
1255+
1256+
int cleanFD = coin();
1257+
int taintedFD;
1258+
scanf("%d", &taintedFD);
1259+
clang_analyzer_isTainted_int(taintedFD); // expected-warning {{YES}}
1260+
1261+
switch (coin()) {
1262+
default: break;
1263+
// Execute the file `FD` refers to, overlaying the running program image.
1264+
// `ARGV` and `ENVP` are passed to the new program, as for `execve'.
1265+
case 0: return fexecve( cleanFD, /*argv=*/ cleanArray, /*envp=*/ cleanArray); // no-warning
1266+
case 1: return fexecve(taintedFD, /*argv=*/ cleanArray, /*envp=*/ cleanArray); // expected-warning {{Untrusted data is passed to a system call}}
1267+
case 2: return fexecve( cleanFD, /*argv=*/taintedArray, /*envp=*/ cleanArray); // FIXME: We might wanna have a report here.
1268+
case 3: return fexecve( cleanFD, /*argv=*/ cleanArray, /*envp=*/taintedArray); // FIXME: We might wanna have a report here.
1269+
}
1270+
1271+
switch (coin()) {
1272+
default: break;
1273+
// Create a new stream connected to a pipe running the given `command`.
1274+
case 0: return pclose(popen("command", /*mode=*/"r")); // no-warning
1275+
case 1: return pclose(popen( buf, /*mode=*/"r")); // expected-warning {{Untrusted data is passed to a system call}}
1276+
case 2: return pclose(popen("command", /*mode=*/buf)); // 'mode' is not a taint sink.
1277+
}
1278+
1279+
switch (coin()) {
1280+
default: break;
1281+
// Execute the given line as a shell command.
1282+
case 0: return system("command"); // no-warning
1283+
case 1: return system( buf); // expected-warning {{Untrusted data is passed to a system call}}
1284+
}
1285+
1286+
return 0;
1287+
}
1288+
11321289
void testUnknownFunction(void (*foo)(void)) {
11331290
foo(); // no-crash
11341291
}

0 commit comments

Comments
 (0)