From cf5e764c7f69bdce8dd76cb0d6c464b7f245ef94 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Sat, 15 Apr 2017 17:13:28 +0300 Subject: [PATCH 1/6] Disambiguate get_*_policy() and get_*_policy_mask() --- include/sway/security.h | 6 +++--- sway/commands.c | 2 +- sway/extensions.c | 8 ++++---- sway/handlers.c | 10 +++++----- sway/ipc-server.c | 2 +- sway/security.c | 6 +++--- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/include/sway/security.h b/include/sway/security.h index c3a5cfd4b..d60f264a0 100644 --- a/include/sway/security.h +++ b/include/sway/security.h @@ -3,9 +3,9 @@ #include #include "sway/config.h" -uint32_t get_feature_policy(pid_t pid); -uint32_t get_ipc_policy(pid_t pid); -uint32_t get_command_policy(const char *cmd); +uint32_t get_feature_policy_mask(pid_t pid); +uint32_t get_ipc_policy_mask(pid_t pid); +uint32_t get_command_policy_mask(const char *cmd); const char *command_policy_str(enum command_context context); diff --git a/sway/commands.c b/sway/commands.c index 17c7d7176..4d7af3017 100644 --- a/sway/commands.c +++ b/sway/commands.c @@ -437,7 +437,7 @@ struct cmd_results *handle_command(char *_exec, enum command_context context) { free_argv(argc, argv); goto cleanup; } - if (!(get_command_policy(argv[0]) & context)) { + if (!(get_command_policy_mask(argv[0]) & context)) { if (results) { free_cmd_results(results); } diff --git a/sway/extensions.c b/sway/extensions.c index 15d2f9715..96957dbff 100644 --- a/sway/extensions.c +++ b/sway/extensions.c @@ -86,7 +86,7 @@ static void set_background(struct wl_client *client, struct wl_resource *resourc struct wl_resource *_output, struct wl_resource *surface) { pid_t pid; wl_client_get_credentials(client, &pid, NULL, NULL); - if (!(get_feature_policy(pid) & FEATURE_BACKGROUND)) { + if (!(get_feature_policy_mask(pid) & FEATURE_BACKGROUND)) { sway_log(L_INFO, "Denying background feature to %d", pid); return; } @@ -114,7 +114,7 @@ static void set_panel(struct wl_client *client, struct wl_resource *resource, struct wl_resource *_output, struct wl_resource *surface) { pid_t pid; wl_client_get_credentials(client, &pid, NULL, NULL); - if (!(get_feature_policy(pid) & FEATURE_PANEL)) { + if (!(get_feature_policy_mask(pid) & FEATURE_PANEL)) { sway_log(L_INFO, "Denying panel feature to %d", pid); return; } @@ -152,7 +152,7 @@ static void desktop_ready(struct wl_client *client, struct wl_resource *resource static void set_panel_position(struct wl_client *client, struct wl_resource *resource, uint32_t position) { pid_t pid; wl_client_get_credentials(client, &pid, NULL, NULL); - if (!(get_feature_policy(pid) & FEATURE_PANEL)) { + if (!(get_feature_policy_mask(pid) & FEATURE_PANEL)) { sway_log(L_INFO, "Denying panel feature to %d", pid); return; } @@ -191,7 +191,7 @@ static void set_lock_surface(struct wl_client *client, struct wl_resource *resou struct wl_resource *_output, struct wl_resource *surface) { pid_t pid; wl_client_get_credentials(client, &pid, NULL, NULL); - if (!(get_feature_policy(pid) & FEATURE_LOCK)) { + if (!(get_feature_policy_mask(pid) & FEATURE_LOCK)) { sway_log(L_INFO, "Denying lock feature to %d", pid); return; } diff --git a/sway/handlers.c b/sway/handlers.c index b61c0a19b..a8de135fd 100644 --- a/sway/handlers.c +++ b/sway/handlers.c @@ -595,7 +595,7 @@ static void handle_view_state_request(wlc_handle view, enum wlc_view_state_bit s pid_t pid = wlc_view_get_pid(view); switch (state) { case WLC_BIT_FULLSCREEN: - if (!(get_feature_policy(pid) & FEATURE_FULLSCREEN)) { + if (!(get_feature_policy_mask(pid) & FEATURE_FULLSCREEN)) { sway_log(L_INFO, "Denying fullscreen to %d (%s)", pid, c->name); break; } @@ -811,7 +811,7 @@ static bool handle_key(wlc_handle view, uint32_t time, const struct wlc_modifier swayc_t *focused = get_focused_container(&root_container); if (focused->type == C_VIEW) { pid_t pid = wlc_view_get_pid(focused->handle); - if (!(get_feature_policy(pid) & FEATURE_KEYBOARD)) { + if (!(get_feature_policy_mask(pid) & FEATURE_KEYBOARD)) { return EVENT_HANDLED; } } @@ -875,7 +875,7 @@ static bool handle_pointer_motion(wlc_handle handle, uint32_t time, const struct swayc_t *focused = get_focused_container(&root_container); if (focused->type == C_VIEW) { pid_t pid = wlc_view_get_pid(focused->handle); - if (!(get_feature_policy(pid) & FEATURE_MOUSE)) { + if (!(get_feature_policy_mask(pid) & FEATURE_MOUSE)) { return EVENT_HANDLED; } } @@ -953,7 +953,7 @@ static bool handle_pointer_button(wlc_handle view, uint32_t time, const struct w if (swayc_is_fullscreen(focused)) { if (focused->type == C_VIEW) { pid_t pid = wlc_view_get_pid(focused->handle); - if (!(get_feature_policy(pid) & FEATURE_MOUSE)) { + if (!(get_feature_policy_mask(pid) & FEATURE_MOUSE)) { return EVENT_HANDLED; } } @@ -1001,7 +1001,7 @@ static bool handle_pointer_button(wlc_handle view, uint32_t time, const struct w if (focused->type == C_VIEW) { pid_t pid = wlc_view_get_pid(focused->handle); - if (!(get_feature_policy(pid) & FEATURE_MOUSE)) { + if (!(get_feature_policy_mask(pid) & FEATURE_MOUSE)) { return EVENT_HANDLED; } } diff --git a/sway/ipc-server.c b/sway/ipc-server.c index 67a3cdc82..dca881fa5 100644 --- a/sway/ipc-server.c +++ b/sway/ipc-server.c @@ -181,7 +181,7 @@ int ipc_handle_connection(int fd, uint32_t mask, void *data) { client->event_source = wlc_event_loop_add_fd(client_fd, WLC_EVENT_READABLE, ipc_client_handle_readable, client); pid_t pid = get_client_pid(client->fd); - client->security_policy = get_ipc_policy(pid); + client->security_policy = get_ipc_policy_mask(pid); list_add(ipc_client_list, client); diff --git a/sway/security.c b/sway/security.c index f8a96ba7f..5b762b07a 100644 --- a/sway/security.c +++ b/sway/security.c @@ -94,7 +94,7 @@ static const char *get_pid_exe(pid_t pid) { return link; } -uint32_t get_feature_policy(pid_t pid) { +uint32_t get_feature_policy_mask(pid_t pid) { uint32_t default_policy = 0; const char *link = get_pid_exe(pid); @@ -111,7 +111,7 @@ uint32_t get_feature_policy(pid_t pid) { return default_policy; } -uint32_t get_ipc_policy(pid_t pid) { +uint32_t get_ipc_policy_mask(pid_t pid) { uint32_t default_policy = 0; const char *link = get_pid_exe(pid); @@ -128,7 +128,7 @@ uint32_t get_ipc_policy(pid_t pid) { return default_policy; } -uint32_t get_command_policy(const char *cmd) { +uint32_t get_command_policy_mask(const char *cmd) { uint32_t default_policy = 0; for (int i = 0; i < config->command_policies->length; ++i) { From b4357a8eb6b9a5c0452fc0d127e37e54dc09e1a0 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Sat, 15 Apr 2017 17:04:04 +0300 Subject: [PATCH 2/6] Rename get_policy to get_feature_policy --- sway/commands/permit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sway/commands/permit.c b/sway/commands/permit.c index e2bec2e24..4a78ef0db 100644 --- a/sway/commands/permit.c +++ b/sway/commands/permit.c @@ -38,7 +38,7 @@ static enum secure_feature get_features(int argc, char **argv, return features; } -static struct feature_policy *get_policy(const char *name) { +static struct feature_policy *get_feature_policy(const char *name) { struct feature_policy *policy = NULL; for (int i = 0; i < config->feature_policies->length; ++i) { struct feature_policy *p = config->feature_policies->items[i]; @@ -66,7 +66,7 @@ struct cmd_results *cmd_permit(int argc, char **argv) { return error; } - struct feature_policy *policy = get_policy(argv[0]); + struct feature_policy *policy = get_feature_policy(argv[0]); policy->features |= get_features(argc, argv, &error); sway_log(L_DEBUG, "Permissions granted to %s for features %d", @@ -84,7 +84,7 @@ struct cmd_results *cmd_reject(int argc, char **argv) { return error; } - struct feature_policy *policy = get_policy(argv[0]); + struct feature_policy *policy = get_feature_policy(argv[0]); policy->features &= ~get_features(argc, argv, &error); sway_log(L_DEBUG, "Permissions granted to %s for features %d", From bfb99235e323e31689e280867103d3bc2e39ac22 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Sat, 15 Apr 2017 17:16:32 +0300 Subject: [PATCH 3/6] Move get_feature_policy to sway/security.c --- include/sway/security.h | 2 ++ sway/commands/permit.c | 19 ------------------- sway/security.c | 20 ++++++++++++++++++++ 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/include/sway/security.h b/include/sway/security.h index d60f264a0..0edffdfa7 100644 --- a/include/sway/security.h +++ b/include/sway/security.h @@ -7,6 +7,8 @@ uint32_t get_feature_policy_mask(pid_t pid); uint32_t get_ipc_policy_mask(pid_t pid); uint32_t get_command_policy_mask(const char *cmd); +struct feature_policy *get_feature_policy(const char *name); + const char *command_policy_str(enum command_context context); struct feature_policy *alloc_feature_policy(const char *program); diff --git a/sway/commands/permit.c b/sway/commands/permit.c index 4a78ef0db..c55f46d87 100644 --- a/sway/commands/permit.c +++ b/sway/commands/permit.c @@ -38,25 +38,6 @@ static enum secure_feature get_features(int argc, char **argv, return features; } -static struct feature_policy *get_feature_policy(const char *name) { - struct feature_policy *policy = NULL; - for (int i = 0; i < config->feature_policies->length; ++i) { - struct feature_policy *p = config->feature_policies->items[i]; - if (strcmp(p->program, name) == 0) { - policy = p; - break; - } - } - if (!policy) { - policy = alloc_feature_policy(name); - if (!policy) { - sway_abort("Unable to allocate security policy"); - } - list_add(config->feature_policies, policy); - } - return policy; -} - struct cmd_results *cmd_permit(int argc, char **argv) { struct cmd_results *error = NULL; if ((error = checkarg(argc, "permit", EXPECTED_MORE_THAN, 1))) { diff --git a/sway/security.c b/sway/security.c index 5b762b07a..96af2b88b 100644 --- a/sway/security.c +++ b/sway/security.c @@ -94,6 +94,26 @@ static const char *get_pid_exe(pid_t pid) { return link; } +struct feature_policy *get_feature_policy(const char *name) { + struct feature_policy *policy = NULL; + + for (int i = 0; i < config->feature_policies->length; ++i) { + struct feature_policy *p = config->feature_policies->items[i]; + if (strcmp(p->program, name) == 0) { + policy = p; + break; + } + } + if (!policy) { + policy = alloc_feature_policy(name); + if (!policy) { + sway_abort("Unable to allocate security policy"); + } + list_add(config->feature_policies, policy); + } + return policy; +} + uint32_t get_feature_policy_mask(pid_t pid) { uint32_t default_policy = 0; const char *link = get_pid_exe(pid); From bcf9338ce7dec6c83564c92567d14443d0467806 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Sat, 15 Apr 2017 18:54:42 +0300 Subject: [PATCH 4/6] Add validate_ipc_target() --- sway/security.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/sway/security.c b/sway/security.c index 96af2b88b..8eab61261 100644 --- a/sway/security.c +++ b/sway/security.c @@ -1,4 +1,6 @@ #define _XOPEN_SOURCE 500 +#include +#include #include #include #include @@ -6,8 +8,46 @@ #include "sway/security.h" #include "log.h" +static bool validate_ipc_target(const char *program) { + struct stat sb; + + sway_log(L_DEBUG, "Validating IPC target '%s'", program); + + if (!strcmp(program, "*")) { + return true; + } + if (lstat(program, &sb) == -1) { + return false; + } + if (!S_ISREG(sb.st_mode)) { + sway_log(L_ERROR, + "IPC target '%s' MUST be/point at an existing regular file", + program); + return false; + } + if (sb.st_uid != 0) { +#ifdef NDEBUG + sway_log(L_ERROR, "IPC target '%s' MUST be owned by root", program); + return false; +#else + sway_log(L_INFO, "IPC target '%s' MUST be owned by root (waived for debug build)", program); + return true; +#endif + } + if (sb.st_mode & S_IWOTH) { + sway_log(L_ERROR, "IPC target '%s' MUST NOT be world writable", program); + return false; + } + + return true; +} + struct feature_policy *alloc_feature_policy(const char *program) { uint32_t default_policy = 0; + + if (!validate_ipc_target(program)) { + return NULL; + } for (int i = 0; i < config->feature_policies->length; ++i) { struct feature_policy *policy = config->feature_policies->items[i]; if (strcmp(policy->program, "*") == 0) { @@ -26,11 +66,16 @@ struct feature_policy *alloc_feature_policy(const char *program) { return NULL; } policy->features = default_policy; + return policy; } struct ipc_policy *alloc_ipc_policy(const char *program) { uint32_t default_policy = 0; + + if (!validate_ipc_target(program)) { + return NULL; + } for (int i = 0; i < config->ipc_policies->length; ++i) { struct ipc_policy *policy = config->ipc_policies->items[i]; if (strcmp(policy->program, "*") == 0) { @@ -49,6 +94,7 @@ struct ipc_policy *alloc_ipc_policy(const char *program) { return NULL; } policy->features = default_policy; + return policy; } From c9694ee63d451da62dc50b234b3080a35a40e844 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Fri, 14 Apr 2017 23:37:43 +0300 Subject: [PATCH 5/6] Add resolve_path() to utils --- common/util.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/util.h | 8 ++++++++ 2 files changed, 49 insertions(+) diff --git a/common/util.c b/common/util.c index 12ed0cdc5..a9e6a9c2a 100644 --- a/common/util.c +++ b/common/util.c @@ -1,3 +1,7 @@ +#define _XOPEN_SOURCE 500 +#include +#include +#include #include #include #include @@ -118,3 +122,40 @@ uint32_t parse_color(const char *color) { } return res; } + +char* resolve_path(const char* path) { + struct stat sb; + ssize_t r; + int i; + char *current = NULL; + char *resolved = NULL; + + if(!(current = strdup(path))) { + return NULL; + } + for (i = 0; i < 16; ++i) { + if (lstat(current, &sb) == -1) { + goto failed; + } + if((sb.st_mode & S_IFMT) != S_IFLNK) { + return current; + } + if (!(resolved = malloc(sb.st_size + 1))) { + goto failed; + } + r = readlink(current, resolved, sb.st_size); + if (r == -1 || r > sb.st_size) { + goto failed; + } + resolved[r] = '\0'; + free(current); + current = strdup(resolved); + free(resolved); + resolved = NULL; + } + +failed: + free(resolved); + free(current); + return NULL; +} \ No newline at end of file diff --git a/include/util.h b/include/util.h index 839af265b..e53654583 100644 --- a/include/util.h +++ b/include/util.h @@ -49,4 +49,12 @@ pid_t get_parent_pid(pid_t pid); */ uint32_t parse_color(const char *color); +/** + * Given a path string, recurseively resolves any symlinks to their targets + * (which may be a file, directory) and returns the result. + * argument is returned. Caller must free the returned buffer. + * If an error occures, if the path does not exist or if the path corresponds + * to a dangling symlink, NULL is returned. + */ +char* resolve_path(const char* path); #endif From 2ad8850398693cb572152e6d97c59de371996273 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Sun, 16 Apr 2017 09:30:14 +0300 Subject: [PATCH 6/6] Handle symlinks as IPC security targets - When policies are allocated, the ipc target path goes through symlink resolution. The result is used as the canonical for matching pids to policies at runtime. In particular, this matches up with the target of the `/proc//exe`. - There's a possible race condition if this isn't done correctly, read below. Originally, validate_ipc_target() always tried to resolve its argument for symlinks, and returned a parogram target string if it validates. This created a possible race condition with security implications. The problem is that get_feature_policy() first independently resolved the policy target in order to check whether a policy already exists. If it didn't find any, it called alloc_feature_policy() which called validate_ipc_target() which resolved the policy target again. In the time between the two checks, the symlink could be altered, and a lucky attacker could fool the program into thinking that a policy doesn't exist for a target, and then switch the symlink to point at another file. At the very least this could allow him to create two policies for the same program target, and possibly to bypass security by associating the permissions for one target with another, or force default permissions to apply to a target for which a more specific rule has been configured. So we don't that. Instead, the policy target is resolved once and that result is used for the rest of the lookup/creation process. --- sway/commands/ipc.c | 10 +++++++++- sway/commands/permit.c | 39 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/sway/commands/ipc.c b/sway/commands/ipc.c index 8a7b849f9..f0b3035af 100644 --- a/sway/commands/ipc.c +++ b/sway/commands/ipc.c @@ -1,3 +1,4 @@ +#define _XOPEN_SOURCE 500 #include #include #include "sway/security.h" @@ -18,8 +19,14 @@ struct cmd_results *cmd_ipc(int argc, char **argv) { return error; } - const char *program = argv[0]; + char *program = NULL; + if (!strcmp(argv[0], "*")) { + program = strdup(argv[0]); + } else if (!(program = resolve_path(argv[0]))) { + return cmd_results_new( + CMD_INVALID, "ipc", "Unable to resolve IPC Policy target."); + } if (config->reading && strcmp("{", argv[1]) != 0) { return cmd_results_new(CMD_INVALID, "ipc", "Expected '{' at start of IPC config definition."); @@ -32,6 +39,7 @@ struct cmd_results *cmd_ipc(int argc, char **argv) { current_policy = alloc_ipc_policy(program); list_add(config->ipc_policies, current_policy); + free(program); return cmd_results_new(CMD_BLOCK_IPC, NULL, NULL); } diff --git a/sway/commands/permit.c b/sway/commands/permit.c index c55f46d87..66fa4e2a7 100644 --- a/sway/commands/permit.c +++ b/sway/commands/permit.c @@ -1,7 +1,9 @@ +#define _XOPEN_SOURCE 500 #include #include "sway/commands.h" #include "sway/config.h" #include "sway/security.h" +#include "util.h" #include "log.h" static enum secure_feature get_features(int argc, char **argv, @@ -47,12 +49,29 @@ struct cmd_results *cmd_permit(int argc, char **argv) { return error; } - struct feature_policy *policy = get_feature_policy(argv[0]); - policy->features |= get_features(argc, argv, &error); + bool assign_perms = true; + char *program = NULL; + if (!strcmp(argv[0], "*")) { + program = strdup(argv[0]); + } else { + program = resolve_path(argv[0]); + } + if (!program) { + sway_assert(program, "Unable to resolve IPC permit target '%s'." + " will issue empty policy", argv[0]); + assign_perms = false; + program = strdup(argv[0]); + } + + struct feature_policy *policy = get_feature_policy(program); + if (assign_perms) { + policy->features |= get_features(argc, argv, &error); + } sway_log(L_DEBUG, "Permissions granted to %s for features %d", policy->program, policy->features); + free(program); return cmd_results_new(CMD_SUCCESS, NULL, NULL); } @@ -65,11 +84,25 @@ struct cmd_results *cmd_reject(int argc, char **argv) { return error; } - struct feature_policy *policy = get_feature_policy(argv[0]); + char *program = NULL; + if (!strcmp(argv[0], "*")) { + program = strdup(argv[0]); + } else { + program = resolve_path(argv[0]); + } + if (!program) { + // Punt + sway_log(L_INFO, "Unable to resolve IPC reject target '%s'." + " Will use provided path", argv[0]); + program = strdup(argv[0]); + } + + struct feature_policy *policy = get_feature_policy(program); policy->features &= ~get_features(argc, argv, &error); sway_log(L_DEBUG, "Permissions granted to %s for features %d", policy->program, policy->features); + free(program); return cmd_results_new(CMD_SUCCESS, NULL, NULL); }