From b1c238af35364746d0e231e2a8ac89c5b6b3afbc Mon Sep 17 00:00:00 2001 From: michalbiesek Date: Mon, 30 Jan 2023 13:21:13 +0100 Subject: [PATCH 1/7] Update`funchook_install` log message --- src/wrap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wrap.c b/src/wrap.c index 337b14cde..79246d6b1 100644 --- a/src/wrap.c +++ b/src/wrap.c @@ -1597,7 +1597,7 @@ initHook(int attachedFlag, bool scopedFlag) // hook 'em rc = funchook_install(funchook, 0); if (rc != 0) { - scopeLogError("ERROR: failed to install SSL_read hook. (%s)\n", + scopeLogError("ERROR: failed to install funchook. (%s)\n", funchook_error_message(funchook)); return; } From ff26e800bf753492cd1e28aff3945ffdfb6ab233 Mon Sep 17 00:00:00 2001 From: michalbiesek Date: Mon, 30 Jan 2023 13:41:34 +0100 Subject: [PATCH 2/7] Destroy funchook when `funchook_install` fails --- src/wrap.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wrap.c b/src/wrap.c index 79246d6b1..1834759d5 100644 --- a/src/wrap.c +++ b/src/wrap.c @@ -1599,6 +1599,7 @@ initHook(int attachedFlag, bool scopedFlag) if (rc != 0) { scopeLogError("ERROR: failed to install funchook. (%s)\n", funchook_error_message(funchook)); + funchook_destroy(funchook); return; } } From b75fbaaae2811352d1fa95ed934613d6eb2854cb Mon Sep 17 00:00:00 2001 From: michalbiesek Date: Mon, 30 Jan 2023 19:56:53 +0100 Subject: [PATCH 3/7] Add `funchook_destroy` in wrap_go.c --- src/wrap_go.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/wrap_go.c b/src/wrap_go.c index 237e4a7ca..0093f4d3b 100644 --- a/src/wrap_go.c +++ b/src/wrap_go.c @@ -938,6 +938,7 @@ initGoHook(elf_buf_t *ebuf) if (ehdr->e_type == ET_DYN && (scopeGetGoAppStateStatic() == FALSE)) { if (getBaseAddress(&base) != 0) { sysprint("ERROR: can't get the base address\n"); + funchook_destroy(funchook); return; // don't install our hooks } Elf64_Shdr* textSec = getElfSection(ebuf->buf, ".text"); @@ -977,9 +978,11 @@ initGoHook(elf_buf_t *ebuf) } else { scopeLogWarn("%s was either compiled with a version of go older than go1.4, or symbols have been stripped. AppScope can only instrument go1.%d or newer, and requires symbols if compiled with a version of go older than go1.13. Continuing without AppScope.", ebuf->cmd, MIN_SUPPORTED_GO_VER); } + funchook_destroy(funchook); return; // don't install our hooks } else if (g_go_minor_ver > MAX_SUPPORTED_GO_VER) { scopeLogWarn("%s was compiled with go version `%s`. Versions newer than Go 1.%d are not yet supported. Continuing without AppScope.", ebuf->cmd, go_runtime_version, MAX_SUPPORTED_GO_VER); + funchook_destroy(funchook); return; // don't install our hooks } @@ -1021,6 +1024,7 @@ initGoHook(elf_buf_t *ebuf) g_go_schema = &go_17_schema_arm; } else { scopeLogWarn("Architecture not supported. Continuing without AppScope."); + funchook_destroy(funchook); return; } } @@ -1077,7 +1081,7 @@ initGoHook(elf_buf_t *ebuf) if (rc != 0) { sysprint("ERROR: funchook_install failed. (%s)\n", funchook_error_message(funchook)); - return; + funchook_destroy(funchook); } } From 4a5480e609e31d281ee63973ee0476e65c1f688b Mon Sep 17 00:00:00 2001 From: michalbiesek Date: Tue, 31 Jan 2023 13:09:33 +0100 Subject: [PATCH 4/7] (#1293) Verify `funchook_install` permission - the funhook install can fail in scenario where systemd `MemoryDenyWriteExecute` setting is on ``` ...Attempts to create memory mappings that are writable and executable at the same time, or to change existing memory mappings to become executable, or mapping shared memory segments as executable, are prohibited... ``` Ref: https://www.freedesktop.org/software/systemd/man/systemd.exec.html Closes: #1293 --- os/linux/os.c | 22 ++++++++++++++++++++++ os/linux/os.h | 2 ++ os/macOS/os.c | 10 ++++++++++ os/macOS/os.h | 2 ++ src/scopeelf.c | 10 +++++----- src/wrap.c | 16 ++++++++++++++++ src/wrap_go.c | 14 ++++++++------ 7 files changed, 65 insertions(+), 11 deletions(-) diff --git a/os/linux/os.c b/os/linux/os.c index 5c750a907..9ea5c20fd 100644 --- a/os/linux/os.c +++ b/os/linux/os.c @@ -950,3 +950,25 @@ osFindFd(pid_t pid, const char *fname) free(cwd); return fd; } + +/* + * Change protection for specified memory region using + * specified combintation flags and extraflags. + * Returns TRUE in case of operation success, FALSE otherwise + * Note: in case of success the `osMemPermRestore` should be called later + * with flags argument + */ +bool +osMemPermAllow(void *addr, size_t len, int flags, int extraflags) { + return scope_mprotect(addr, len, flags | extraflags) == 0; +} + +/* + * Restore permission for specified memory region. + * Returns TRUE in case of operation success, FALSE otherwise + * Note: This function should be called in case of success of `osMemPermAllow` + */ +bool +osMemPermRestore(void *addr, size_t len, int flags) { + return scope_mprotect(addr, len, flags) == 0; +} diff --git a/os/linux/os.h b/os/linux/os.h index 799d6abfe..16a370042 100644 --- a/os/linux/os.h +++ b/os/linux/os.h @@ -68,5 +68,7 @@ extern long long osGetProcCPU(void); extern uint64_t osFindLibrary(const char *, pid_t, bool); extern int osFindFd(pid_t, const char *); extern void osCreateSM(proc_id_t *, unsigned long); +extern bool osMemPermAllow(void *, size_t, int, int); +extern bool osMemPermRestore(void *, size_t, int); #endif //__OS_H__ diff --git a/os/macOS/os.c b/os/macOS/os.c index c9897dc7c..217cf5ea2 100644 --- a/os/macOS/os.c +++ b/os/macOS/os.c @@ -193,3 +193,13 @@ long long osGetProcCPU(void) { return -1; } + +bool +osMemPermAllow(void *addr, size_t len, int flags, int extraflags) { + return FALSE; +} + +bool +osMemPermRestore(void *addr, size_t len, int flags) { + return FALSE; +} diff --git a/os/macOS/os.h b/os/macOS/os.h index 6b2f8d86d..a6bd197a4 100644 --- a/os/macOS/os.h +++ b/os/macOS/os.h @@ -48,3 +48,5 @@ extern int osNeedsConnect(int); extern const char *osGetUserName(unsigned); extern const char *osGetGroupName(unsigned); extern long long osGetProcCPU(void); +extern bool osMemPermAllow(void *, size_t, int, int); +extern bool osMemPermRestore(void *, size_t, int); diff --git a/src/scopeelf.c b/src/scopeelf.c index 6a360759d..41099ec43 100644 --- a/src/scopeelf.c +++ b/src/scopeelf.c @@ -201,9 +201,9 @@ doGotcha(struct link_map *lm, got_list_t *hook, Elf64_Rela *rel, Elf64_Sym *sym, if (prot != -1) { if ((prot & PROT_WRITE) == 0) { - // mprotect if write perms are not set - if (scope_mprotect((void *)saddr, (size_t)16, PROT_WRITE | prot) == -1) { - scopeLog(CFG_LOG_DEBUG, "doGotcha: mprotect failed"); + // allow for write permission it write permission are not set + if (osMemPermAllow((void *)saddr, 16, prot, PROT_WRITE) == FALSE) { + scopeLog(CFG_LOG_DEBUG, "doGotcha: osMemPermAllow add write protection flag failed"); return -1; } } @@ -239,8 +239,8 @@ doGotcha(struct link_map *lm, got_list_t *hook, Elf64_Rela *rel, Elf64_Sym *sym, if ((prot & PROT_WRITE) == 0) { // if we didn't mod above leave prot settings as is - if (scope_mprotect((void *)saddr, (size_t)16, prot) == -1) { - scopeLog(CFG_LOG_DEBUG, "doGotcha: mprotect failed"); + if (osMemPermRestore((void *)saddr, 16, prot) == FALSE) { + scopeLog(CFG_LOG_DEBUG, "doGotcha: osMemPermRestore remove write memory protection flags failed"); return -1; } } diff --git a/src/wrap.c b/src/wrap.c index 1834759d5..4fe5986ca 100644 --- a/src/wrap.c +++ b/src/wrap.c @@ -1541,6 +1541,22 @@ initHook(int attachedFlag, bool scopedFlag) if (should_we_patch || g_fn.__write_libc || g_fn.__write_pthread || ((g_ismusl == FALSE) && g_fn.sendmmsg) || ((g_ismusl == TRUE) && (g_fn.sendto || g_fn.recvfrom))) { + + /* + * Check if we have proper permission to install hook function + * We need to have abilites to change permission of region memory + * using (PROT_WRITE + PROT_EXEC) flags + */ + size_t testSize = 16; + void *ptr = scope_malloc(testSize); + if (osMemPermAllow(ptr, testSize, PROT_READ | PROT_WRITE, PROT_EXEC) == FALSE) { + scope_free(ptr); + scopeLogError("Interpose functions are limited (DNS, Console I/O). Please verify the MemoryDenyWriteExecute setting for following service: %s", g_proc.procname); + return; + } + scope_free(ptr); + + funchook = funchook_create(); if (logLevel(g_log) <= CFG_LOG_TRACE) { diff --git a/src/wrap_go.c b/src/wrap_go.c index 0093f4d3b..e618556d0 100644 --- a/src/wrap_go.c +++ b/src/wrap_go.c @@ -818,9 +818,11 @@ patchClone() size_t pageSize = scope_getpagesize(); void *addr = (void *)((ptrdiff_t) clone & ~(pageSize - 1)); - // set write perms on the page - if (scope_mprotect(addr, pageSize, PROT_WRITE | PROT_READ | PROT_EXEC)) { - scopeLogError("ERROR: patchCLone: mprotect failed\n"); + const int perm = PROT_READ | PROT_EXEC; + + // Add write permission on the page + if (osMemPermAllow(addr, pageSize, perm, PROT_WRITE) == FALSE) { + scopeLogError("ERROR: patchClone: osMemPermAllow failed\n"); return; } @@ -832,9 +834,9 @@ patchClone() scopeLog(CFG_LOG_DEBUG, "patchClone: CLONE PATCHED\n"); - // restore perms to the page - if (scope_mprotect(addr, pageSize, PROT_READ | PROT_EXEC)) { - scopeLogError("ERROR: patchCLone: mprotect restore failed\n"); + // restore original permission to the page + if (osMemPermRestore(addr, pageSize, perm) == FALSE) { + scopeLogError("ERROR: patchClone: osMemPermRestore failed\n"); return; } } From 5660b1f014b5e546b1dff2e74b852e785e77834c Mon Sep 17 00:00:00 2001 From: michalbiesek Date: Tue, 31 Jan 2023 11:53:37 +0100 Subject: [PATCH 5/7] Extend unit test with `ostest` - validate `osMemPermAllowWrite` and `osMemPermRestore` --- os/linux/Makefile | 1 + test/unit/execute.sh | 1 + test/unit/library/ostest.c | 45 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 test/unit/library/ostest.c diff --git a/os/linux/Makefile b/os/linux/Makefile index 160ee0a18..29542be51 100644 --- a/os/linux/Makefile +++ b/os/linux/Makefile @@ -109,6 +109,7 @@ libtest: $(LIBRARY_C_FILES) $(LIBRARY_TEST_C_FILES) $(YAML_AR) $(JSON_AR) $(TEST $(CC) $(TEST_CFLAGS) -o test/$(OS)/coredumptest coredumptest.o coredump.o scopestdlib.o dbg.o utils.o fn.o plattime.o os.o test.o $(TEST_AR) $(TEST_LD_FLAGS) $(CC) $(TEST_CFLAGS) -o test/$(OS)/ipctest ipctest.o ipc.o ipc_resp.o cfgutils.o cfg.o mtc.o log.o evtformat.o ctl.o transport.o backoff.o mtcformat.o strset.o com.o scopestdlib.o dbg.o circbuf.o linklist.o fn.o utils.o os.o test.o report.o search.o httpagg.o state.o httpstate.o metriccapture.o plattime.o $(TEST_AR) $(TEST_LD_FLAGS) -Wl,--wrap=jsonConfigurationObject -Wl,--wrap=doAndReplaceConfig $(CC) $(TEST_CFLAGS) -o test/$(OS)/snapshottest snapshottest.o snapshot.o coredump.o scopestdlib.o dbg.o utils.o fn.o plattime.o os.o test.o $(TEST_AR) $(TEST_LD_FLAGS) + $(CC) $(TEST_CFLAGS) -o test/$(OS)/ostest ostest.o scopestdlib.o dbg.o fn.o utils.o plattime.o os.o test.o $(TEST_AR) $(TEST_LD_FLAGS) $(CC) $(TEST_CFLAGS) -o test/$(OS)/strsettest strsettest.o strset.o scopestdlib.o dbg.o test.o $(TEST_AR) $(TEST_LD_FLAGS) $(CC) $(TEST_CFLAGS) -o test/$(OS)/cfgutilstest cfgutilstest.o cfgutils.o cfg.o mtc.o log.o evtformat.o ctl.o transport.o backoff.o mtcformat.o strset.o com.o scopestdlib.o dbg.o circbuf.o linklist.o fn.o utils.o os.o test.o report.o search.o httpagg.o state.o httpstate.o metriccapture.o plattime.o $(TEST_AR) $(TEST_LD_FLAGS) $(CC) $(TEST_CFLAGS) -o test/$(OS)/cfgtest cfgtest.o cfg.o scopestdlib.o dbg.o test.o $(TEST_AR) $(TEST_LD_FLAGS) diff --git a/test/unit/execute.sh b/test/unit/execute.sh index 7f28e5c5d..68ac3210a 100755 --- a/test/unit/execute.sh +++ b/test/unit/execute.sh @@ -68,6 +68,7 @@ run_test test/${OS}/vdsotest run_test test/${OS}/coredumptest run_test test/${OS}/ipctest run_test test/${OS}/snapshottest +run_test test/${OS}/ostest run_test test/${OS}/strsettest run_test test/${OS}/cfgutilstest run_test test/${OS}/cfgtest diff --git a/test/unit/library/ostest.c b/test/unit/library/ostest.c new file mode 100644 index 000000000..9cc1b54b1 --- /dev/null +++ b/test/unit/library/ostest.c @@ -0,0 +1,45 @@ +#define _GNU_SOURCE + +#include "os.h" +#include "scopestdlib.h" +#include "test.h" + +static void +osWritePermSuccess(void **state) { + int perm = PROT_READ | PROT_EXEC; + size_t len = 4096; + void *addr = scope_mmap(NULL, len, perm, MAP_ANONYMOUS | MAP_SHARED, -1, 0); + assert_ptr_not_equal(addr, MAP_FAILED); + bool res = osMemPermAllow(addr, len, perm, PROT_WRITE); + assert_true(res); + res = osMemPermRestore(addr, len, perm); + assert_true(res); + scope_munmap(addr ,len); +} + +static void +osWritePermFailure(void **state) { + int perm = PROT_READ; + size_t len = 4096; + + // Open file as read only + int fd = scope_open("/etc/passwd", O_RDONLY); + void *addr = scope_mmap(NULL, len, perm, MAP_SHARED, fd, 0); + assert_ptr_not_equal(addr, MAP_FAILED); + bool res = osMemPermAllow(addr, len, perm, PROT_WRITE); + assert_false(res); + scope_munmap(addr ,len); + scope_close(fd); +} + +int +main(int argc, char* argv[]) { + printf("running %s\n", argv[0]); + + const struct CMUnitTest tests[] = { + cmocka_unit_test(osWritePermSuccess), + cmocka_unit_test(osWritePermFailure), + cmocka_unit_test(dbgHasNoUnexpectedFailures), + }; + return cmocka_run_group_tests(tests, groupSetup, groupTeardown); +} From e8378d8427db645c8505eaf30392de7ae23fbf39 Mon Sep 17 00:00:00 2001 From: michalbiesek Date: Tue, 31 Jan 2023 14:41:31 +0100 Subject: [PATCH 6/7] Fix typo `initally` ==> `initially` --- os/linux/os.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/os/linux/os.c b/os/linux/os.c index 9ea5c20fd..220a0c78e 100644 --- a/os/linux/os.c +++ b/os/linux/os.c @@ -869,7 +869,7 @@ osCreateSM(proc_id_t *proc, unsigned long addr) return; } - // size is initally 0 and needs to be increased + // size is initially 0 and needs to be increased if (scope_ftruncate(proc->smfd, sizeof(export_sm_t)) == -1) { scope_close(proc->smfd); return; From de19873a1c319b1a60fe741ef033c2bbd10dae8d Mon Sep 17 00:00:00 2001 From: Abe Raher Date: Fri, 3 Feb 2023 10:02:13 +0100 Subject: [PATCH 7/7] Modify msg in case `osMemPermAllow` fails --- src/wrap.c | 2 +- src/wrap_go.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wrap.c b/src/wrap.c index 4fe5986ca..307039140 100644 --- a/src/wrap.c +++ b/src/wrap.c @@ -1551,7 +1551,7 @@ initHook(int attachedFlag, bool scopedFlag) void *ptr = scope_malloc(testSize); if (osMemPermAllow(ptr, testSize, PROT_READ | PROT_WRITE, PROT_EXEC) == FALSE) { scope_free(ptr); - scopeLogError("Interpose functions are limited (DNS, Console I/O). Please verify the MemoryDenyWriteExecute setting for following service: %s", g_proc.procname); + scopeLogError("The system is not allowing processes related to DNS or console I/O to be scoped. Try setting MemoryDenyWriteExecute to false for the %s service.", g_proc.procname); return; } scope_free(ptr); diff --git a/src/wrap_go.c b/src/wrap_go.c index e618556d0..d8353eacd 100644 --- a/src/wrap_go.c +++ b/src/wrap_go.c @@ -811,7 +811,7 @@ patch_addrs(funchook_t *funchook, } static void -patchClone() +patchClone(void) { void *clone = dlsym(RTLD_DEFAULT, "__clone"); if (clone) { @@ -822,7 +822,7 @@ patchClone() // Add write permission on the page if (osMemPermAllow(addr, pageSize, perm, PROT_WRITE) == FALSE) { - scopeLogError("ERROR: patchClone: osMemPermAllow failed\n"); + scopeLogError("The system is not allowing processes to be scoped. Try setting MemoryDenyWriteExecute to false for the Go service."); return; }