From 1c0c923ad185c6c639f2a35b14581012283cc86b Mon Sep 17 00:00:00 2001 From: Daniel Richard G Date: Sat, 13 May 2023 01:54:32 -0400 Subject: [PATCH 1/3] Split g_file_open() into _ro() and _rw() variants Rename g_file_open() to g_file_open_rw(), and add a new g_file_open_ro() call that wraps the common g_file_open_ex(file, 1, 0, 0, 0) idiom. This will make the file access mode more explicit in the code. Change all calls to g_file_open() to the _ro() or _rw() variant as appropriate, and replace g_file_open_ex(file, 1, 0, 0, 0) with the _ro() call. Lastly, add tests for the two new calls to test_os_calls.c (code courteously provided by matt335672). --- common/file.c | 4 +-- common/log.c | 2 +- common/os_calls.c | 22 ++++++------- common/os_calls.h | 3 +- fontutils/fv1.c | 2 +- fontutils/windows/fontdump.c | 2 +- keygen/keygen.c | 2 +- libxrdp/xrdp_sec.c | 2 +- sesman/chansrv/chansrv_config.c | 2 +- sesman/chansrv/clipboard_file.c | 2 +- sesman/libsesman/sesman_config.c | 2 +- sesman/lock_uds.c | 2 +- sesman/sesman.c | 10 +++--- tests/common/test_os_calls.c | 56 ++++++++++++++++++++++++++++---- tests/libipm/test_libipm_main.c | 2 +- xrdp/lang.c | 2 +- xrdp/xrdp.c | 12 +++---- xrdp/xrdp_bitmap_load.c | 2 +- xrdp/xrdp_font.c | 2 +- xrdp/xrdp_listen.c | 2 +- xrdp/xrdp_login_wnd.c | 4 +-- xrdp/xrdp_mm.c | 4 +-- xrdp/xrdp_wm.c | 6 ++-- xrdp/xrdpwin.c | 14 ++++---- 24 files changed, 103 insertions(+), 60 deletions(-) diff --git a/common/file.c b/common/file.c index a0ef888a72..64a1fff8eb 100644 --- a/common/file.c +++ b/common/file.c @@ -349,7 +349,7 @@ file_by_name_read_sections(const char *file_name, struct list *names) return 1; } - fd = g_file_open_ex(file_name, 1, 0, 0, 0); + fd = g_file_open_ro(file_name); if (fd < 0) { @@ -390,7 +390,7 @@ file_by_name_read_section(const char *file_name, const char *section, return 1; } - fd = g_file_open_ex(file_name, 1, 0, 0, 0); + fd = g_file_open_ro(file_name); if (fd < 0) { diff --git a/common/log.c b/common/log.c index b24ac04399..fe8bbcb55c 100644 --- a/common/log.c +++ b/common/log.c @@ -689,7 +689,7 @@ log_config_init_from_config(const char *iniFilename, return NULL; } - fd = g_file_open_ex(iniFilename, 1, 0, 0, 0); + fd = g_file_open_ro(iniFilename); if (-1 == fd) { diff --git a/common/os_calls.c b/common/os_calls.c index 1ce059249a..8f4ec7074f 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -2086,24 +2086,14 @@ g_memcmp(const void *s1, const void *s2, int len) /*****************************************************************************/ /* returns -1 on error, else return handle or file descriptor */ int -g_file_open(const char *file_name) +g_file_open_rw(const char *file_name) { #if defined(_WIN32) return (int)CreateFileA(file_name, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, 0, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0); #else - int rv; - - rv = open(file_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); - - if (rv == -1) - { - /* can't open read / write, try to open read only */ - rv = open(file_name, O_RDONLY); - } - - return rv; + return open(file_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); #endif } @@ -2145,6 +2135,14 @@ g_file_open_ex(const char *file_name, int aread, int awrite, #endif } +/*****************************************************************************/ +/* returns -1 on error, else return handle or file descriptor */ +int +g_file_open_ro(const char *file_name) +{ + return g_file_open_ex(file_name, 1, 0, 0, 0); +} + /*****************************************************************************/ /* returns error, always 0 */ int diff --git a/common/os_calls.h b/common/os_calls.h index dcf4eb3382..dfbc85cb4d 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -206,9 +206,10 @@ int g_obj_wait(tintptr *read_objs, int rcount, tintptr *write_objs, void g_random(char *data, int len); int g_abs(int i); int g_memcmp(const void *s1, const void *s2, int len); -int g_file_open(const char *file_name); +int g_file_open_rw(const char *file_name); int g_file_open_ex(const char *file_name, int aread, int awrite, int acreate, int atrunc); +int g_file_open_ro(const char *file_name); int g_file_close(int fd); /** * Returns 1 if a file is open (i.e. the file descriptor is valid) diff --git a/fontutils/fv1.c b/fontutils/fv1.c index af154c1ded..a532c96648 100644 --- a/fontutils/fv1.c +++ b/fontutils/fv1.c @@ -170,7 +170,7 @@ fv1_file_load(const char *filename) int fd; make_stream(s); init_stream(s, file_size + 1024); - fd = g_file_open(filename); + fd = g_file_open_ro(filename); if (fd < 0) { diff --git a/fontutils/windows/fontdump.c b/fontutils/windows/fontdump.c index 8379e692d3..286bcb8503 100644 --- a/fontutils/windows/fontdump.c +++ b/fontutils/windows/fontdump.c @@ -155,7 +155,7 @@ font_dump(void) g_snprintf(filename, 255, "%s-%d.fv1", g_font_name, g_font_size); msg("creating file %s", filename); g_file_delete(filename); - fd = g_file_open(filename); + fd = g_file_open_rw(filename); g_file_write(fd, "FNT1", 4); strlen1 = g_strlen(g_font_name); g_file_write(fd, g_font_name, strlen1); diff --git a/keygen/keygen.c b/keygen/keygen.c index e1c1e95c0e..9b477530b1 100644 --- a/keygen/keygen.c +++ b/keygen/keygen.c @@ -367,7 +367,7 @@ save_all(const char *e_data, int e_len, const char *n_data, int n_len, } } - fd = g_file_open(filename); + fd = g_file_open_rw(filename); if (fd != -1) { diff --git a/libxrdp/xrdp_sec.c b/libxrdp/xrdp_sec.c index 77e86401e2..92aea0d9e0 100644 --- a/libxrdp/xrdp_sec.c +++ b/libxrdp/xrdp_sec.c @@ -420,7 +420,7 @@ xrdp_load_keyboard_layout(struct xrdp_client_info *client_info) g_snprintf(keyboard_cfg_file, 255, "%s/xrdp_keyboard.ini", XRDP_CFG_PATH); LOG(LOG_LEVEL_DEBUG, "keyboard_cfg_file %s", keyboard_cfg_file); - fd = g_file_open(keyboard_cfg_file); + fd = g_file_open_ro(keyboard_cfg_file); if (fd >= 0) { diff --git a/sesman/chansrv/chansrv_config.c b/sesman/chansrv/chansrv_config.c index 7ee7bf0be3..57a106728e 100644 --- a/sesman/chansrv/chansrv_config.c +++ b/sesman/chansrv/chansrv_config.c @@ -231,7 +231,7 @@ config_read(int use_logger, const char *sesman_ini) log_func_t logmsg = (use_logger) ? log_message : log_to_stdout; int fd; - fd = g_file_open_ex(sesman_ini, 1, 0, 0, 0); + fd = g_file_open_ro(sesman_ini); if (fd < 0) { logmsg(LOG_LEVEL_ERROR, "Can't open config file %s", sesman_ini); diff --git a/sesman/chansrv/clipboard_file.c b/sesman/chansrv/clipboard_file.c index 9400848b1f..990cffa3d9 100644 --- a/sesman/chansrv/clipboard_file.c +++ b/sesman/chansrv/clipboard_file.c @@ -462,7 +462,7 @@ clipboard_send_file_data(int streamId, int lindex, "nPositionLow %d cbRequested %d", streamId, lindex, nPositionLow, cbRequested); g_snprintf(full_fn, 255, "%s/%s", cfi->pathname, cfi->filename); - fd = g_file_open_ex(full_fn, 1, 0, 0, 0); + fd = g_file_open_ro(full_fn); if (fd == -1) { LOG(LOG_LEVEL_ERROR, "clipboard_send_file_data: file open [%s] failed: %s", diff --git a/sesman/libsesman/sesman_config.c b/sesman/libsesman/sesman_config.c index 8494563b08..0721612149 100644 --- a/sesman/libsesman/sesman_config.c +++ b/sesman/libsesman/sesman_config.c @@ -583,7 +583,7 @@ config_read(const char *sesman_ini) if ((cfg->sesman_ini = g_strdup(sesman_ini)) != NULL) { int fd; - if ((fd = g_file_open_ex(cfg->sesman_ini, 1, 0, 0, 0)) != -1) + if ((fd = g_file_open_ro(cfg->sesman_ini)) != -1) { struct list *sec; struct list *param_n; diff --git a/sesman/lock_uds.c b/sesman/lock_uds.c index daab99d123..916bc571fd 100644 --- a/sesman/lock_uds.c +++ b/sesman/lock_uds.c @@ -87,7 +87,7 @@ lock_uds(const char *sockname) *p++ = '\0'; saved_umask = g_umask_hex(0x77); - fd = g_file_open(filename); + fd = g_file_open_rw(filename); g_umask_hex(saved_umask); if (fd < 0) { diff --git a/sesman/sesman.c b/sesman/sesman.c index 312574c8d0..0f780bd85f 100644 --- a/sesman/sesman.c +++ b/sesman/sesman.c @@ -658,7 +658,7 @@ read_pid_file(const char *pid_file, int *pid) { g_printf("sesman is not running (pid file not found - %s)\n", pid_file); } - else if ((fd = g_file_open(pid_file)) < 0) + else if ((fd = g_file_open_ro(pid_file)) < 0) { g_printf("error opening pid file[%s]: %s\n", pid_file, g_get_strerror()); } @@ -838,15 +838,15 @@ main(int argc, char **argv) g_file_close(1); g_file_close(2); - if (g_file_open("/dev/null") < 0) + if (g_file_open_rw("/dev/null") < 0) { } - if (g_file_open("/dev/null") < 0) + if (g_file_open_rw("/dev/null") < 0) { } - if (g_file_open("/dev/null") < 0) + if (g_file_open_rw("/dev/null") < 0) { } } @@ -905,7 +905,7 @@ main(int argc, char **argv) { /* writing pid file */ char pid_s[32]; - int fd = g_file_open(pid_file); + int fd = g_file_open_rw(pid_file); if (-1 == fd) { diff --git a/tests/common/test_os_calls.c b/tests/common/test_os_calls.c index fb49dbe93d..e0f44bbc1b 100644 --- a/tests/common/test_os_calls.c +++ b/tests/common/test_os_calls.c @@ -17,6 +17,9 @@ #define TOP_SRCDIR "." #endif +// File for testing ro/rw opens +#define RO_RW_FILE "./test_ro_rw" + /******************************************************************************/ /*** * Gets the number of open file descriptors for the current process */ @@ -199,12 +202,51 @@ START_TEST(test_g_file_get_size__5GiB) END_TEST #endif +/******************************************************************************/ +/* Test we can write to a file which is opened for write */ +START_TEST(test_g_file_rw) +{ + const char data[] = "File data\n"; + + int fd = g_file_open_rw(RO_RW_FILE); + ck_assert(fd >= 0); + + int status = g_file_write(fd, data, sizeof(data) - 1); + g_file_close(fd); + + // Assume no signals have occurred + ck_assert_int_eq(status, sizeof(data) - 1); + + // Leave file in place for test_g_file_ro +} +END_TEST + +/******************************************************************************/ +/* Test we can't write to a file which is opened read only */ +START_TEST(test_g_file_ro) +{ + const char data[] = "File data\n"; + + int fd = g_file_open_ro(RO_RW_FILE); + ck_assert(fd >= 0); + + int status = g_file_write(fd, data, sizeof(data) - 1); + g_file_close(fd); + + // Write must fail + ck_assert_int_lt(status, 0); + + // Tidy-up (not checked) + g_file_delete(RO_RW_FILE); +} +END_TEST + /******************************************************************************/ /* Just test we can set and clear the flag. We don't test its operation */ START_TEST(test_g_file_cloexec) { int flag; - int devzerofd = g_file_open("/dev/zero"); + int devzerofd = g_file_open_ro("/dev/zero"); ck_assert(devzerofd >= 0); (void)g_file_set_cloexec(devzerofd, 1); @@ -230,7 +272,7 @@ START_TEST(test_g_file_get_open_fds) ck_assert_int_eq(start_list->count, fd_count); // Open another file - int devzerofd = g_file_open("/dev/zero"); + int devzerofd = g_file_open_ro("/dev/zero"); ck_assert(devzerofd >= 0); // Have we now got one more open file? @@ -266,7 +308,7 @@ END_TEST /******************************************************************************/ START_TEST(test_g_file_is_open) { - int devzerofd = g_file_open("/dev/zero"); + int devzerofd = g_file_open_ro("/dev/zero"); ck_assert(devzerofd >= 0); // Check open file comes up as open @@ -287,7 +329,7 @@ START_TEST(test_g_sck_fd_passing) int istatus; unsigned int fdcount; - int devzerofd = g_file_open("/dev/zero"); + int devzerofd = g_file_open_ro("/dev/zero"); ck_assert(devzerofd >= 0); if (g_sck_local_socketpair(sck) != 0) @@ -369,8 +411,8 @@ START_TEST(test_g_sck_fd_overflow) unsigned int proc_fd_count; // Open a couple of file descriptors to /dev/zero - devzerofd[0] = g_file_open("/dev/zero"); - devzerofd[1] = g_file_open("/dev/zero"); + devzerofd[0] = g_file_open_ro("/dev/zero"); + devzerofd[1] = g_file_open_ro("/dev/zero"); ck_assert(devzerofd[0] >= 0); ck_assert(devzerofd[1] >= 0); proc_fd_count = get_open_fd_count(); @@ -463,6 +505,8 @@ make_suite_test_os_calls(void) tcase_add_test(tc_os_calls, test_g_file_get_size__2GiB); tcase_add_test(tc_os_calls, test_g_file_get_size__5GiB); #endif + tcase_add_test(tc_os_calls, test_g_file_rw); + tcase_add_test(tc_os_calls, test_g_file_ro); // Must follow test_g_file_rw tcase_add_test(tc_os_calls, test_g_file_cloexec); tcase_add_test(tc_os_calls, test_g_file_get_open_fds); tcase_add_test(tc_os_calls, test_g_file_is_open); diff --git a/tests/libipm/test_libipm_main.c b/tests/libipm/test_libipm_main.c index 6d43aa7268..3cf7233f0b 100644 --- a/tests/libipm/test_libipm_main.c +++ b/tests/libipm/test_libipm_main.c @@ -58,7 +58,7 @@ suite_test_libipm_calls_start(void) const char *errstr = g_get_strerror(); LOG(LOG_LEVEL_ERROR, "Can't create test transport 3 [%s]", errstr); } - else if ((fd = g_file_open("/dev/zero")) < 0) + else if ((fd = g_file_open_rw("/dev/zero")) < 0) { const char *errstr = g_get_strerror(); LOG(LOG_LEVEL_ERROR, "Can't open /dev/zero [%s]", errstr); diff --git a/xrdp/lang.c b/xrdp/lang.c index 234a2d5f26..ba4e9c8453 100644 --- a/xrdp/lang.c +++ b/xrdp/lang.c @@ -288,7 +288,7 @@ int km_load_file(const char *filename, struct xrdp_keymap *keymap) int fd; LOG(LOG_LEVEL_INFO, "Loading keymap file %s", filename); - fd = g_file_open(filename); + fd = g_file_open_ro(filename); if (fd != -1) { diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c index f8f2234794..22065982e3 100644 --- a/xrdp/xrdp.c +++ b/xrdp/xrdp.c @@ -375,7 +375,7 @@ main(int argc, char **argv) if (g_file_exist(pid_file)) /* xrdp.pid */ { - fd = g_file_open(pid_file); /* xrdp.pid */ + fd = g_file_open_ro(pid_file); /* xrdp.pid */ } if (fd == -1) @@ -450,7 +450,7 @@ main(int argc, char **argv) g_create_path(pid_file); /* make sure we can write to pid file */ - fd = g_file_open(pid_file); /* xrdp.pid */ + fd = g_file_open_rw(pid_file); /* xrdp.pid */ if (fd == -1) { @@ -509,7 +509,7 @@ main(int argc, char **argv) g_sleep(1000); /* write the pid to file */ pid = g_getpid(); - fd = g_file_open(pid_file); /* xrdp.pid */ + fd = g_file_open_rw(pid_file); /* xrdp.pid */ if (fd == -1) { @@ -528,15 +528,15 @@ main(int argc, char **argv) g_file_close(1); g_file_close(2); - if (g_file_open("/dev/null") < 0) + if (g_file_open_rw("/dev/null") < 0) { } - if (g_file_open("/dev/null") < 0) + if (g_file_open_rw("/dev/null") < 0) { } - if (g_file_open("/dev/null") < 0) + if (g_file_open_rw("/dev/null") < 0) { } diff --git a/xrdp/xrdp_bitmap_load.c b/xrdp/xrdp_bitmap_load.c index 3f48232cb5..f51d9f9d24 100644 --- a/xrdp/xrdp_bitmap_load.c +++ b/xrdp/xrdp_bitmap_load.c @@ -542,7 +542,7 @@ xrdp_bitmap_load_bmp(struct xrdp_bitmap *self, const char *filename, return 1; } - fd = g_file_open(filename); + fd = g_file_open_ro(filename); if (fd == -1) { diff --git a/xrdp/xrdp_font.c b/xrdp/xrdp_font.c index 6fe81d8c46..1224338ec5 100644 --- a/xrdp/xrdp_font.c +++ b/xrdp/xrdp_font.c @@ -210,7 +210,7 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi) self->wm = wm; make_stream(s); init_stream(s, file_size + 1024); - fd = g_file_open(file_path); + fd = g_file_open_ro(file_path); if (fd != -1) { diff --git a/xrdp/xrdp_listen.c b/xrdp/xrdp_listen.c index e91f98bfec..3543bac400 100644 --- a/xrdp/xrdp_listen.c +++ b/xrdp/xrdp_listen.c @@ -170,7 +170,7 @@ xrdp_listen_get_startup_params(struct xrdp_listen *self) startup_params = self->startup_params; port_override = startup_params->port[0] != 0; fork_override = startup_params->fork; - fd = g_file_open(startup_params->xrdp_ini); + fd = g_file_open_ro(startup_params->xrdp_ini); if (fd != -1) { names = list_create(); diff --git a/xrdp/xrdp_login_wnd.c b/xrdp/xrdp_login_wnd.c index 8b6062f4ce..6c31868a68 100644 --- a/xrdp/xrdp_login_wnd.c +++ b/xrdp/xrdp_login_wnd.c @@ -607,7 +607,7 @@ xrdp_wm_login_fill_in_combo(struct xrdp_wm *self, struct xrdp_bitmap *b) section_names->auto_free = 1; section_values = list_create(); section_values->auto_free = 1; - fd = g_file_open(xrdp_ini); + fd = g_file_open_ro(xrdp_ini); if (fd < 0) { @@ -1107,7 +1107,7 @@ load_xrdp_config(struct xrdp_config *config, const char *xrdp_ini, int bpp) globals->ls_unscaled.help_wnd_height = DEFAULT_WND_HELP_H; /* open xrdp.ini file */ - if ((fd = g_file_open(xrdp_ini)) < 0) + if ((fd = g_file_open_ro(xrdp_ini)) < 0) { LOG(LOG_LEVEL_ERROR, "load_config: Could not read " "xrdp.ini file %s", xrdp_ini); diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 8aec3f9ca4..4fc2c97e1a 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -2073,7 +2073,7 @@ xrdp_mm_get_sesman_port(char *port, int port_bytes) g_strncpy(port, "3350", port_bytes - 1); /* see if port is in sesman.ini file */ g_snprintf(cfg_file, 255, "%s/sesman.ini", XRDP_CFG_PATH); - fd = g_file_open(cfg_file); + fd = g_file_open_ro(cfg_file); if (fd >= 0) { @@ -2861,7 +2861,7 @@ xrdp_mm_dump_jpeg(struct xrdp_mm *self, XRDP_ENC_DATA_DONE *enc_done) header.bytes_follow = enc_done->comp_bytes - (2 + pheader_bytes[0]); if (ii == 0) { - ii = g_file_open("/tmp/jpeg.beef.bin"); + ii = g_file_open_rw("/tmp/jpeg.beef.bin"); if (ii == -1) { ii = 0; diff --git a/xrdp/xrdp_wm.c b/xrdp/xrdp_wm.c index 858059fa2c..b2c9dc6aa8 100644 --- a/xrdp/xrdp_wm.c +++ b/xrdp/xrdp_wm.c @@ -245,7 +245,7 @@ xrdp_wm_load_pointer(struct xrdp_wm *self, char *file_name, char *data, make_stream(fs); init_stream(fs, 8192); - fd = g_file_open(file_name); + fd = g_file_open_ro(file_name); if (fd < 0) { @@ -414,7 +414,7 @@ xrdp_wm_load_static_colors_plus(struct xrdp_wm *self, char *autorun_name) self->background = HCOLOR(self->screen->bpp, 0x000000); /* now load them from the globals in xrdp.ini if defined */ - fd = g_file_open(self->session->xrdp_ini); + fd = g_file_open_ro(self->session->xrdp_ini); if (fd >= 0) { @@ -638,7 +638,7 @@ xrdp_wm_init(struct xrdp_wm *self) * NOTE: this should eventually be accessed from self->xrdp_config */ - fd = g_file_open(self->session->xrdp_ini); + fd = g_file_open_ro(self->session->xrdp_ini); if (fd != -1) { names = list_create(); diff --git a/xrdp/xrdpwin.c b/xrdp/xrdpwin.c index f9a57ee8e4..be06c3861e 100644 --- a/xrdp/xrdpwin.c +++ b/xrdp/xrdpwin.c @@ -214,7 +214,7 @@ MyServiceMain(DWORD dwArgc, LPTSTR *lpszArgv) // int fd; // char text[256]; - // fd = g_file_open("c:\\temp\\xrdp\\log.txt"); + // fd = g_file_open_rw("c:\\temp\\xrdp\\log.txt"); // g_file_write(fd, "hi\r\n", 4); //event_han = RegisterEventSource(0, "xrdp"); //log_event(event_han, "hi xrdp log"); @@ -452,7 +452,7 @@ main(int argc, char **argv) if (g_file_exist(pid_file)) /* xrdp.pid */ { - fd = g_file_open(pid_file); /* xrdp.pid */ + fd = g_file_open_ro(pid_file); /* xrdp.pid */ } if (fd == -1) @@ -539,7 +539,7 @@ main(int argc, char **argv) if (!no_daemon) { /* make sure we can write to pid file */ - fd = g_file_open(pid_file); /* xrdp.pid */ + fd = g_file_open_rw(pid_file); /* xrdp.pid */ if (fd == -1) { @@ -579,9 +579,9 @@ main(int argc, char **argv) g_file_close(0); g_file_close(1); g_file_close(2); - g_file_open("/dev/null"); - g_file_open("/dev/null"); - g_file_open("/dev/null"); + g_file_open_rw("/dev/null"); + g_file_open_rw("/dev/null"); + g_file_open_rw("/dev/null"); /* end of daemonizing code */ } @@ -589,7 +589,7 @@ main(int argc, char **argv) { /* write the pid to file */ pid = g_getpid(); - fd = g_file_open(pid_file); /* xrdp.pid */ + fd = g_file_open_rw(pid_file); /* xrdp.pid */ if (fd == -1) { From b191d87e338ac11c373f4553a2ba3ccd47e6c998 Mon Sep 17 00:00:00 2001 From: Daniel Richard G Date: Sat, 13 May 2023 02:06:26 -0400 Subject: [PATCH 2/3] Move Linux's no_new_privs call into os_calls This helps keep the application code free of platform-specific cruft. Also remove a needless #include from sesman/session_list.c. --- common/os_calls.c | 19 +++++++++++++++++++ common/os_calls.h | 1 + sesman/sesexec/session.c | 13 +------------ sesman/session_list.c | 4 ---- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/common/os_calls.c b/common/os_calls.c index 8f4ec7074f..53dcf4a12f 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -53,6 +53,9 @@ #include #include #include +#if defined(HAVE_SYS_PRCTL_H) +#include +#endif #include #include #include @@ -3954,3 +3957,19 @@ g_tcp6_bind_address(int sck, const char *port, const char *address) return -1; #endif } + +/*****************************************************************************/ +/* returns error, zero is success, non zero is error */ +/* only works in linux */ +int +g_no_new_privs(void) +{ +#if defined(HAVE_SYS_PRCTL_H) && defined(PR_SET_NO_NEW_PRIVS) + /* + * PR_SET_NO_NEW_PRIVS requires Linux kernel 3.5 and newer. + */ + return prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); +#else + return 0; +#endif +} diff --git a/common/os_calls.h b/common/os_calls.h index dfbc85cb4d..8e095d32f4 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -333,6 +333,7 @@ int g_tcp4_socket(void); int g_tcp4_bind_address(int sck, const char *port, const char *address); int g_tcp6_socket(void); int g_tcp6_bind_address(int sck, const char *port, const char *address); +int g_no_new_privs(void); /* glib-style wrappers */ #define g_new(struct_type, n_structs) \ diff --git a/sesman/sesexec/session.c b/sesman/sesexec/session.c index d9a0c54ce2..6d45e994c7 100644 --- a/sesman/sesexec/session.c +++ b/sesman/sesexec/session.c @@ -33,10 +33,6 @@ #include "config_ac.h" #endif -#ifdef HAVE_SYS_PRCTL_H -#include -#endif - #include #include "arch.h" @@ -56,10 +52,6 @@ #include "xwait.h" #include "xrdp_sockets.h" -#ifndef PR_SET_NO_NEW_PRIVS -#define PR_SET_NO_NEW_PRIVS 38 -#endif - struct session_data { pid_t x_server; ///< PID of X server @@ -347,21 +339,18 @@ prepare_xorg_xserver_params(const struct session_parameters *s, { params->auto_free = 1; -#ifdef HAVE_SYS_PRCTL_H /* * Make sure Xorg doesn't run setuid root. Root access is not * needed. Xorg can fail when run as root and the user has no * console permissions. - * PR_SET_NO_NEW_PRIVS requires Linux kernel 3.5 and newer. */ - if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) < 0) + if (g_no_new_privs() != 0) { LOG(LOG_LEVEL_WARNING, "[session start] (display %u): Failed to disable " "setuid on X server: %s", s->display, g_get_strerror()); } -#endif g_snprintf(screen, sizeof(screen), ":%u", s->display); diff --git a/sesman/session_list.c b/sesman/session_list.c index 36a6c9f93c..497c6eb86d 100644 --- a/sesman/session_list.c +++ b/sesman/session_list.c @@ -33,10 +33,6 @@ #include "config_ac.h" #endif -#ifdef HAVE_SYS_PRCTL_H -#include -#endif - #include "arch.h" #include "session_list.h" #include "trans.h" From fdfe47668b7118238b1bb4fa83c0a4c483094d18 Mon Sep 17 00:00:00 2001 From: Daniel Richard G Date: Sat, 13 May 2023 02:14:54 -0400 Subject: [PATCH 3/3] Add XorgNoNewPrivileges configuration option This allows Linux's no_new_privs restriction to be disabled when starting the X server, which may be desirable if xrdp is running inside a kernel confinement framework such as AppArmor or SELinux. --- docs/man/sesman.ini.5.in | 9 +++++++++ sesman/libsesman/sesman_config.c | 10 ++++++++++ sesman/libsesman/sesman_config.h | 6 ++++++ sesman/sesexec/session.c | 2 +- sesman/sesman.ini.in | 5 +++++ 5 files changed, 31 insertions(+), 1 deletion(-) diff --git a/docs/man/sesman.ini.5.in b/docs/man/sesman.ini.5.in index 474591c002..1933893a59 100644 --- a/docs/man/sesman.ini.5.in +++ b/docs/man/sesman.ini.5.in @@ -303,6 +303,15 @@ if the group specified in \fBTerminalServerUsers\fR doesn't exist. \fBAllowAlternateShell\fR=\fI[true|false]\fR If set to \fB0\fR, \fBfalse\fR or \fBno\fR, prevent usage of alternate shells by users. +.TP +\fBXorgNoNewPrivileges\fR=\fI[true|false]\fR +Only applicable on Linux. If set to \fB0\fR, \fBfalse\fR or \fBno\fR, do +not use the kernel's \fIno_new_privs\fR restriction when invoking the Xorg +X11 server. The use of \fIno_new_privs\fR is intended to prevent issues due +to a setuid Xorg executable. However, if a kernel security module (such as +AppArmor) is used to confine xrdp, \fIno_new_privs\fR may interfere with +transitions between confinement domains. + .SH "X11 SERVER" Following parameters can be used in the \fB[Xvnc]\fR and \fB[Xorg]\fR sections. diff --git a/sesman/libsesman/sesman_config.c b/sesman/libsesman/sesman_config.c index 0721612149..b18bfdb487 100644 --- a/sesman/libsesman/sesman_config.c +++ b/sesman/libsesman/sesman_config.c @@ -70,6 +70,7 @@ #define SESMAN_CFG_SEC_RESTRICT_OUTBOUND_CLIPBOARD "RestrictOutboundClipboard" #define SESMAN_CFG_SEC_RESTRICT_INBOUND_CLIPBOARD "RestrictInboundClipboard" #define SESMAN_CFG_SEC_ALLOW_ALTERNATE_SHELL "AllowAlternateShell" +#define SESMAN_CFG_SEC_XORG_NO_NEW_PRIVILEGES "XorgNoNewPrivileges" #define SESMAN_CFG_SESSIONS "Sessions" #define SESMAN_CFG_SESS_MAX "MaxSessions" @@ -310,6 +311,7 @@ config_read_security(int file, struct config_security *sc, sc->restrict_outbound_clipboard = 0; sc->restrict_inbound_clipboard = 0; sc->allow_alternate_shell = 1; + sc->xorg_no_new_privileges = 1; file_read_section(file, SESMAN_CFG_SECURITY, param_n, param_v); @@ -383,6 +385,11 @@ config_read_security(int file, struct config_security *sc, g_text2bool((char *)list_get_item(param_v, i)); } + if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_XORG_NO_NEW_PRIVILEGES)) + { + sc->xorg_no_new_privileges = + g_text2bool((char *)list_get_item(param_v, i)); + } } return 0; @@ -670,6 +677,9 @@ config_dump(struct config_sesman *config) g_writeln(" MaxLoginRetry: %d", sc->login_retry); g_writeln(" AlwaysGroupCheck: %d", sc->ts_always_group_check); g_writeln(" AllowAlternateShell: %d", sc->allow_alternate_shell); +#ifdef HAVE_SYS_PRCTL_H + g_writeln(" XorgNoNewPrivileges: %d", sc->xorg_no_new_privileges); +#endif sesman_clip_restrict_mask_to_string(sc->restrict_outbound_clipboard, restrict_s, sizeof(restrict_s)); g_writeln(" RestrictOutboundClipboard: %s", restrict_s); diff --git a/sesman/libsesman/sesman_config.h b/sesman/libsesman/sesman_config.h index c3b4871a98..30bd79d6b8 100644 --- a/sesman/libsesman/sesman_config.h +++ b/sesman/libsesman/sesman_config.h @@ -103,6 +103,12 @@ struct config_security * If not specified, 'YES' is assumed. */ int allow_alternate_shell; + + /* + * @var xorg_no_new_privileges + * @brief if the Xorg X11 server should be started with no_new_privs (Linux only) + */ + int xorg_no_new_privileges; }; /** diff --git a/sesman/sesexec/session.c b/sesman/sesexec/session.c index 6d45e994c7..485631d847 100644 --- a/sesman/sesexec/session.c +++ b/sesman/sesexec/session.c @@ -344,7 +344,7 @@ prepare_xorg_xserver_params(const struct session_parameters *s, * needed. Xorg can fail when run as root and the user has no * console permissions. */ - if (g_no_new_privs() != 0) + if (g_cfg->sec.xorg_no_new_privileges && g_no_new_privs() != 0) { LOG(LOG_LEVEL_WARNING, "[session start] (display %u): Failed to disable " diff --git a/sesman/sesman.ini.in b/sesman/sesman.ini.in index dd615858d0..01a1aa5b71 100644 --- a/sesman/sesman.ini.in +++ b/sesman/sesman.ini.in @@ -39,6 +39,11 @@ RestrictOutboundClipboard=none RestrictInboundClipboard=none ; Set to 'no' to prevent users from logging in with alternate shells #AllowAlternateShell=true +; On Linux systems, the Xorg X11 server is normally invoked using +; no_new_privs to avoid problems if the executable is suid. This may, +; however, interfere with the use of security modules such as AppArmor. +; Leave this unset unless you need to disable it. +#XorgNoNewPrivileges=true [Sessions] ;; X11DisplayOffset - x11 display number offset