From d433c5b5c4835fe80ba836b22fc4fb08c0e94110 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Fri, 21 Apr 2017 02:09:01 +0300 Subject: [PATCH 1/5] Refactor IPC target validation --- include/sway/config.h | 2 ++ sway/commands/permit.c | 14 ++++++++++---- sway/security.c | 8 ++------ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/include/sway/config.h b/include/sway/config.h index 2de904344..1ee849305 100644 --- a/include/sway/config.h +++ b/include/sway/config.h @@ -208,6 +208,7 @@ enum secure_feature { struct feature_policy { char *program; uint32_t features; + bool validated; }; enum ipc_feature { @@ -235,6 +236,7 @@ enum ipc_feature { struct ipc_policy { char *program; uint32_t features; + bool validated; }; /** diff --git a/sway/commands/permit.c b/sway/commands/permit.c index 66fa4e2a7..11918efd9 100644 --- a/sway/commands/permit.c +++ b/sway/commands/permit.c @@ -49,7 +49,6 @@ struct cmd_results *cmd_permit(int argc, char **argv) { return error; } - bool assign_perms = true; char *program = NULL; if (!strcmp(argv[0], "*")) { @@ -65,11 +64,14 @@ struct cmd_results *cmd_permit(int argc, char **argv) { } struct feature_policy *policy = get_feature_policy(program); - if (assign_perms) { + if (policy->validated) { policy->features |= get_features(argc, argv, &error); + sway_log(L_DEBUG, "Permissions granted to %s for features %d", + policy->program, policy->features); + } else { + sway_log(L_ERROR, "Unable to validate IPC permit target '%s'." + " will issue empty policy", argv[0]); } - 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); @@ -98,6 +100,10 @@ struct cmd_results *cmd_reject(int argc, char **argv) { } struct feature_policy *policy = get_feature_policy(program); + if (!policy->validated) { + sway_log(L_ERROR, "Unable to validate IPC reject target '%s'." + " Allowing `reject` directive anyway", argv[0]); + } policy->features &= ~get_features(argc, argv, &error); sway_log(L_DEBUG, "Permissions granted to %s for features %d", diff --git a/sway/security.c b/sway/security.c index 8eab61261..0c12bc32b 100644 --- a/sway/security.c +++ b/sway/security.c @@ -45,9 +45,6 @@ static bool validate_ipc_target(const char *program) { 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) { @@ -60,6 +57,7 @@ struct feature_policy *alloc_feature_policy(const char *program) { if (!policy) { return NULL; } + policy->validated = validate_ipc_target (program); policy->program = strdup(program); if (!policy->program) { free(policy); @@ -73,9 +71,6 @@ struct feature_policy *alloc_feature_policy(const char *program) { 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) { @@ -88,6 +83,7 @@ struct ipc_policy *alloc_ipc_policy(const char *program) { if (!policy) { return NULL; } + policy->validated = validate_ipc_target (program); policy->program = strdup(program); if (!policy->program) { free(policy); From 04c222119033a5f64410c8f7a9737b7f293e9f4e Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Fri, 21 Apr 2017 14:01:38 +0300 Subject: [PATCH 2/5] Fix leak in cmd_ipc --- sway/commands/ipc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sway/commands/ipc.c b/sway/commands/ipc.c index f0b3035af..eacf7e94c 100644 --- a/sway/commands/ipc.c +++ b/sway/commands/ipc.c @@ -19,14 +19,6 @@ struct cmd_results *cmd_ipc(int argc, char **argv) { return error; } - 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."); @@ -36,6 +28,14 @@ struct cmd_results *cmd_ipc(int argc, char **argv) { return cmd_results_new(CMD_FAILURE, "ipc", "Can only be used in config file."); } + 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."); + } current_policy = alloc_ipc_policy(program); list_add(config->ipc_policies, current_policy); From 403dccabce3e78508aa6d8e099b0307dbacb89d7 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Fri, 21 Apr 2017 14:02:40 +0300 Subject: [PATCH 3/5] Refactor code into resolve_ipc_path --- include/sway/security.h | 1 + sway/commands/ipc.c | 14 +++++--------- sway/commands/permit.c | 25 ++++--------------------- sway/security.c | 19 +++++++++++++++++++ 4 files changed, 29 insertions(+), 30 deletions(-) diff --git a/include/sway/security.h b/include/sway/security.h index 0edffdfa7..02f22e41d 100644 --- a/include/sway/security.h +++ b/include/sway/security.h @@ -15,4 +15,5 @@ struct feature_policy *alloc_feature_policy(const char *program); struct ipc_policy *alloc_ipc_policy(const char *program); struct command_policy *alloc_command_policy(const char *command); +char *resolve_ipc_path(const char* program); #endif diff --git a/sway/commands/ipc.c b/sway/commands/ipc.c index eacf7e94c..efcfad569 100644 --- a/sway/commands/ipc.c +++ b/sway/commands/ipc.c @@ -18,24 +18,20 @@ struct cmd_results *cmd_ipc(int argc, char **argv) { if ((error = check_security_config())) { return error; } - if (config->reading && strcmp("{", argv[1]) != 0) { return cmd_results_new(CMD_INVALID, "ipc", "Expected '{' at start of IPC config definition."); } - if (!config->reading) { return cmd_results_new(CMD_FAILURE, "ipc", "Can only be used in config file."); } - char *program = NULL; + 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 (!(program = resolve_ipc_path(argv[0]))) { + return cmd_results_new( + CMD_FAILURE, "ipc", "Memory allocation error occured."); + } current_policy = alloc_ipc_policy(program); list_add(config->ipc_policies, current_policy); diff --git a/sway/commands/permit.c b/sway/commands/permit.c index 11918efd9..599e25431 100644 --- a/sway/commands/permit.c +++ b/sway/commands/permit.c @@ -50,17 +50,8 @@ struct cmd_results *cmd_permit(int argc, char **argv) { } 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]); + if (!(program = resolve_ipc_path(argv[0]))) { + sway_abort("memory allocation failed"); } struct feature_policy *policy = get_feature_policy(program); @@ -87,16 +78,8 @@ struct cmd_results *cmd_reject(int argc, char **argv) { } 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]); + if (!(program = resolve_ipc_path(argv[0]))) { + sway_abort("memory allocation failed"); } struct feature_policy *policy = get_feature_policy(program); diff --git a/sway/security.c b/sway/security.c index 0c12bc32b..c947342a5 100644 --- a/sway/security.c +++ b/sway/security.c @@ -6,6 +6,7 @@ #include #include "sway/config.h" #include "sway/security.h" +#include "util.h" #include "log.h" static bool validate_ipc_target(const char *program) { @@ -222,3 +223,21 @@ const char *command_policy_str(enum command_context context) { return "unknown"; } } + +/** + * An IPC-specific version of util.c:resolve_path() + * Always returns the "best" path It can unless an ENOMEM occurs , + * in which case it returns NULL. + */ +char *resolve_ipc_path(const char* name) { + char *program = NULL; + if (!strcmp(name, "*")) { + program = strdup(name); + } else { + program = resolve_path(name); + if (!program) { + program = strdup(name); + } + } + return program; +} From 02dfa15d30d4f780e3222a795f1cfc624e9ae35f Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Fri, 21 Apr 2017 02:17:55 +0300 Subject: [PATCH 4/5] Fix NULL deref in cmd_ipc --- sway/commands/ipc.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sway/commands/ipc.c b/sway/commands/ipc.c index efcfad569..e3e62b422 100644 --- a/sway/commands/ipc.c +++ b/sway/commands/ipc.c @@ -32,8 +32,12 @@ struct cmd_results *cmd_ipc(int argc, char **argv) { return cmd_results_new( CMD_FAILURE, "ipc", "Memory allocation error occured."); } - current_policy = alloc_ipc_policy(program); - list_add(config->ipc_policies, current_policy); + if ((current_policy = alloc_ipc_policy(program))) { + list_add(config->ipc_policies, current_policy); + } else { + return cmd_results_new( + CMD_FAILURE, "ipc", "Failed to allocate security policy."); + } free(program); return cmd_results_new(CMD_BLOCK_IPC, NULL, NULL); From 6833c898bfbcf37de727a73d656a13609f662460 Mon Sep 17 00:00:00 2001 From: Jerzi Kaminsky Date: Fri, 21 Apr 2017 02:09:59 +0300 Subject: [PATCH 5/5] Fix Some Log messages --- sway/security.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sway/security.c b/sway/security.c index c947342a5..1b6b79588 100644 --- a/sway/security.c +++ b/sway/security.c @@ -22,7 +22,7 @@ static bool validate_ipc_target(const char *program) { } if (!S_ISREG(sb.st_mode)) { sway_log(L_ERROR, - "IPC target '%s' MUST be/point at an existing regular file", + "IPC target '%s' MUST point at an existing file", program); return false; } @@ -31,7 +31,7 @@ static bool validate_ipc_target(const char *program) { 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); + sway_log(L_ERROR, "IPC target '%s' MUST be owned by root (waived for debug build)", program); return true; #endif }