From: Mark Wielaard Date: Wed, 16 Feb 2022 21:56:31 +0000 (+0100) Subject: Warn for execve syscall with argv or argv[0] being NULL. X-Git-Tag: VALGRIND_3_19_0~12 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8eb547054a051a00742b1b9e1b381015fafeacb9;p=thirdparty%2Fvalgrind.git Warn for execve syscall with argv or argv[0] being NULL. For execve valgrind would silently fail when argv was NULL or unadressable. Make sure that this produces a warning under memcheck. The linux kernel accepts argv[0] being NULL, but most other kernels don't since posix says it should be non-NULL and it causes argc to be zero which is unexpected and might cause security issues. This adjusts some testcases so they don't rely on execve succeeding when argv is NULL and expect warnings about argv or argv[0] being NULL or unaddressable. https://bugs.kde.org/show_bug.cgi?id=450437 --- diff --git a/NEWS b/NEWS index 730f2b5ff8..924032b3c1 100644 --- a/NEWS +++ b/NEWS @@ -93,6 +93,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 449838 sigsegv liburing the 'impossible' happened for io_uring_setup 450025 Powerc: ACC file not implemented as a logical overlay of the VSR registers. +450437 Warn for execve syscall with argv or argv[0] being NULL 450536 Powerpc: valgrind throws 'facility scv unavailable exception' 451626 Syscall param bpf(attr->raw_tracepoint.name) points to unaddressable byte(s) 451827 [ppc64le] VEX temporary storage exhausted with several vbpermq instructions diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index bc3fa6fe9f..44a60bf128 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -2933,6 +2933,7 @@ void handle_pre_sys_execve(ThreadId tid, SyscallStatus *status, Addr pathname, Bool setuid_allowed, trace_this_child; const char *str; char str2[30], str3[30]; + Addr arg_2_check = arg_2; switch (execveType) { case EXECVE: @@ -2951,15 +2952,26 @@ void handle_pre_sys_execve(ThreadId tid, SyscallStatus *status, Addr pathname, VG_(strcpy)(str2, str); VG_(strcpy)(str3, str); - if (arg_2 != 0) { - /* At least the terminating NULL must be addressable. */ - if (!ML_(safe_to_deref)((HChar **) (Addr)arg_2, sizeof(HChar *))) { - SET_STATUS_Failure(VKI_EFAULT); - return; + VG_(strcat)(str2, "(argv)"); + VG_(strcat)(str3, "(argv[0])"); + + /* argv[] should not be NULL and valid. */ + PRE_MEM_READ(str2, arg_2_check, sizeof(Addr)); + + /* argv[0] should not be NULL and valid. */ + if (ML_(safe_to_deref)((HChar **) (Addr)arg_2_check, sizeof(HChar *))) { + Addr argv0 = *(Addr*)arg_2_check; + PRE_MEM_RASCIIZ( str3, argv0 ); + /* The rest of argv can be NULL or a valid string pointer. */ + if (VG_(am_is_valid_for_client)(arg_2_check, sizeof(HChar), VKI_PROT_READ)) { + arg_2_check += sizeof(HChar*); + str3[VG_(strlen)(str)] = '\0'; + VG_(strcat)(str3, "(argv[i])"); + ML_(pre_argv_envp)( arg_2_check, tid, str2, str3 ); } - VG_(strcat)(str2, "(argv)"); - VG_(strcat)(str3, "(argv[i])"); - ML_(pre_argv_envp)( arg_2, tid, str2, str3 ); + } else { + SET_STATUS_Failure(VKI_EFAULT); + return; } // Reset helper strings to syscall name. str2[VG_(strlen)(str)] = '\0'; diff --git a/memcheck/tests/arm64-linux/scalar.stderr.exp b/memcheck/tests/arm64-linux/scalar.stderr.exp index 66975efcb2..4c81819b64 100644 --- a/memcheck/tests/arm64-linux/scalar.stderr.exp +++ b/memcheck/tests/arm64-linux/scalar.stderr.exp @@ -75,6 +75,11 @@ Syscall param execve(filename) points to unaddressable byte(s) by 0x........: main (scalar.c:91) Address 0x........ is not stack'd, malloc'd or (recently) free'd +Syscall param execve(argv) points to unaddressable byte(s) + ... + by 0x........: main (scalar.c:91) + Address 0x........ is not stack'd, malloc'd or (recently) free'd + ----------------------------------------------------- 49: __NR_chdir 1s 1m ----------------------------------------------------- @@ -576,13 +581,13 @@ Syscall param getpriority(who) contains uninitialised byte(s) ----------------------------------------------------- 140: __NR_setpriority 3s 0m ----------------------------------------------------- + +More than 100 errors detected. Subsequent errors +will still be recorded, but in less detail than before. Syscall param setpriority(which) contains uninitialised byte(s) ... by 0x........: main (scalar.c:458) - -More than 100 errors detected. Subsequent errors -will still be recorded, but in less detail than before. Syscall param setpriority(who) contains uninitialised byte(s) ... by 0x........: main (scalar.c:458) diff --git a/memcheck/tests/execve1.c b/memcheck/tests/execve1.c index 83e058a2f3..df36f145e0 100644 --- a/memcheck/tests/execve1.c +++ b/memcheck/tests/execve1.c @@ -4,7 +4,7 @@ int main(void) { char* null_filename = NULL; char* bad[2] = { (char*)1, NULL }; - char* good[1] = { NULL }; + char* good[2] = { "true", NULL }; execve(null_filename, bad, bad); execve("/bin/true", good, good); diff --git a/memcheck/tests/execve1.stderr.exp b/memcheck/tests/execve1.stderr.exp index 37a91b83a3..eebc1e5ebd 100644 --- a/memcheck/tests/execve1.stderr.exp +++ b/memcheck/tests/execve1.stderr.exp @@ -3,7 +3,7 @@ Syscall param execve(filename) points to unaddressable byte(s) by 0x........: main (execve1.c:9) Address 0x........ is not stack'd, malloc'd or (recently) free'd -Syscall param execve(argv[i]) points to unaddressable byte(s) +Syscall param execve(argv[0]) points to unaddressable byte(s) ... by 0x........: main (execve1.c:9) Address 0x........ is not stack'd, malloc'd or (recently) free'd diff --git a/memcheck/tests/execve2.stderr.exp b/memcheck/tests/execve2.stderr.exp index cd98593f7c..f9d7c35926 100644 --- a/memcheck/tests/execve2.stderr.exp +++ b/memcheck/tests/execve2.stderr.exp @@ -3,3 +3,8 @@ Syscall param execve(filename) points to unaddressable byte(s) by 0x........: main (execve2.c:9) Address 0x........ is not stack'd, malloc'd or (recently) free'd +Syscall param execve(argv) points to unaddressable byte(s) + ... + by 0x........: main (execve2.c:9) + Address 0x........ is not stack'd, malloc'd or (recently) free'd + diff --git a/memcheck/tests/linux/sys-execveat.stderr.exp b/memcheck/tests/linux/sys-execveat.stderr.exp index a58b0fb6ae..b49b9be981 100644 --- a/memcheck/tests/linux/sys-execveat.stderr.exp +++ b/memcheck/tests/linux/sys-execveat.stderr.exp @@ -17,3 +17,15 @@ Syscall param execveat(argv) points to uninitialised byte(s) at 0x........: malloc (vg_replace_malloc.c:...) by 0x........: main (sys-execveat.c:41) +Syscall param execveat(argv[0]) points to unaddressable byte(s) + ... + by 0x........: sys_execveat (sys-execveat.c:16) + by 0x........: main (sys-execveat.c:51) + Address 0x........ is not stack'd, malloc'd or (recently) free'd + +Syscall param execveat(argv) points to unaddressable byte(s) + ... + by 0x........: sys_execveat (sys-execveat.c:16) + by 0x........: main (sys-execveat.c:52) + Address 0x........ is not stack'd, malloc'd or (recently) free'd + diff --git a/memcheck/tests/x86-linux/scalar.c b/memcheck/tests/x86-linux/scalar.c index 52f0d4e353..54d0e0443a 100644 --- a/memcheck/tests/x86-linux/scalar.c +++ b/memcheck/tests/x86-linux/scalar.c @@ -95,9 +95,9 @@ int main(void) char *argv_envp[] = {(char *) (x0 + 1), NULL}; GO(__NR_execve, "4s 2m"); SY(__NR_execve, x0 + 1, x0 + argv_envp, x0); FAIL; - + char *argv_ok[] = {"frob", NULL}; GO(__NR_execve, "4s 2m"); - SY(__NR_execve, x0 + 1, x0, x0 + argv_envp); FAIL; + SY(__NR_execve, x0 + 1, x0 + argv_ok, x0 + argv_envp); FAIL; // __NR_chdir 12 GO(__NR_chdir, "1s 1m"); diff --git a/memcheck/tests/x86-linux/scalar.stderr.exp b/memcheck/tests/x86-linux/scalar.stderr.exp index 470023f0e0..b9202a8c2f 100644 --- a/memcheck/tests/x86-linux/scalar.stderr.exp +++ b/memcheck/tests/x86-linux/scalar.stderr.exp @@ -170,6 +170,11 @@ Syscall param execve(filename) points to unaddressable byte(s) by 0x........: main (scalar.c:90) Address 0x........ is not stack'd, malloc'd or (recently) free'd +Syscall param execve(argv) points to unaddressable byte(s) + ... + by 0x........: main (scalar.c:90) + Address 0x........ is not stack'd, malloc'd or (recently) free'd + ----------------------------------------------------- 11: __NR_execve 3s 1m ----------------------------------------------------- @@ -190,6 +195,11 @@ Syscall param execve(filename) points to unaddressable byte(s) by 0x........: main (scalar.c:93) Address 0x........ is not stack'd, malloc'd or (recently) free'd +Syscall param execve(argv) points to unaddressable byte(s) + ... + by 0x........: main (scalar.c:93) + Address 0x........ is not stack'd, malloc'd or (recently) free'd + ----------------------------------------------------- 11: __NR_execve 4s 2m ----------------------------------------------------- @@ -216,7 +226,7 @@ Syscall param execve(argv) points to uninitialised byte(s) Address 0x........ is on thread 1's stack in frame #1, created by main (scalar.c:29) -Syscall param execve(argv[i]) points to unaddressable byte(s) +Syscall param execve(argv[0]) points to unaddressable byte(s) ... by 0x........: main (scalar.c:97) Address 0x........ is not stack'd, malloc'd or (recently) free'd @@ -564,6 +574,9 @@ Syscall param pipe(filedes) contains uninitialised byte(s) ... by 0x........: main (scalar.c:225) + +More than 100 errors detected. Subsequent errors +will still be recorded, but in less detail than before. Syscall param pipe(filedes) points to unaddressable byte(s) ... by 0x........: main (scalar.c:225) @@ -576,9 +589,6 @@ Syscall param times(buf) contains uninitialised byte(s) ... by 0x........: main (scalar.c:229) - -More than 100 errors detected. Subsequent errors -will still be recorded, but in less detail than before. Syscall param times(buf) points to unaddressable byte(s) ... by 0x........: main (scalar.c:229) diff --git a/none/tests/execve.c b/none/tests/execve.c index 950842da29..a1af72fd9e 100644 --- a/none/tests/execve.c +++ b/none/tests/execve.c @@ -7,20 +7,42 @@ int main(int argc, char **argv) if (argc == 1) { // This tests the case where argv and envp are NULL, which is easy to - // get wrong because it's an unusual case. + // get wrong because it's an unusual case. It is also bad and only + // "worked" by accident with the linux kernel. -#if defined(VGO_solaris) - // Solaris requires non-NULL argv parameter char *const argv_exe[] = {"true", NULL}; - if (execve("/bin/true", argv_exe, NULL) < 0) + char *const v_null[] = { NULL }; + char *const v_minus_one[] = { (char *const) -1, NULL }; + +#if defined(VGO_solaris) + const char *exe = "/bin/true"; #elif defined(VGO_darwin) - if (execve("/usr/bin/true", NULL, NULL) < 0) + const char *exe = "/usr/bin/true"; #elif defined(VGO_freebsd) - char *const argv_exe[] = {"true", NULL}; - if (execve("/usr/bin/true", argv_exe, NULL) < 0) + const char *exe = "/usr/bin/true"; #else - if (execve("/bin/true", NULL, NULL) < 0) + const char *exe = "/bin/true"; #endif + + /* Try some bad argv and envp arguments, make sure the executable + doesn't actually exists, so execve doesn't accidentally succeeds. */ + if (execve("/%/", NULL, NULL) >= 0) + printf ("WHAT?"); + if (execve("/%/", (void *)-1, NULL) >= 0) + printf ("WHAT?"); + if (execve("/%/", v_null, NULL) >= 0) + printf ("WHAT?"); + if (execve("/%/", v_null, v_null) >= 0) + printf ("WHAT?"); + if (execve("/%/", v_minus_one, NULL) >= 0) + printf ("WHAT?"); + if (execve("/%/", v_minus_one, v_null) >= 0) + printf ("WHAT?"); + if (execve("/%/", v_minus_one, v_minus_one) >= 0) + printf ("WHAT?"); + + /* Finally a correct execve. */ + if (execve(exe, argv_exe, NULL) < 0) { perror("execve"); exit(1);