From 25528cf4a5d2b037d4d51d6163bbde412b5bc33c Mon Sep 17 00:00:00 2001 From: William Roberts Date: Fri, 29 Jan 2016 10:32:34 -0800 Subject: [PATCH 1/5] checkseapp: declare internal function as static Change-Id: Ic4dc59650ca849b950cb145fedafdf4fc250f009 Signed-off-by: William Roberts --- tools/check_seapp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/check_seapp.c b/tools/check_seapp.c index 5a03b7f36..84e28536a 100644 --- a/tools/check_seapp.c +++ b/tools/check_seapp.c @@ -305,7 +305,7 @@ log_msg(FILE *out, const char *prefix, const char *fmt, ...) { * statically to this executable and LINK_SEPOL_STATIC is not * defined. */ -int check_type(sepol_policydb_t *db, char *type) { +static int check_type(sepol_policydb_t *db, char *type) { int rc = 1; #if defined(LINK_SEPOL_STATIC) From efebf97e23ce6a170fbea5f3376af0d9f0a63858 Mon Sep 17 00:00:00 2001 From: William Roberts Date: Fri, 29 Jan 2016 10:34:04 -0800 Subject: [PATCH 2/5] checkseapp: update error message output Change the final error message to be consistent with the others. From: Error: reading /home/wcrobert/workspace/aosp/external/sepolicy/seapp_contexts, line 82, name domain, value system_server To: Error: Reading file: "/home/wcrobert/workspace/aosp/external/sepolicy/seapp_contexts" line: 82 name: "domain" value: "system_server" Change-Id: Idf791d28fbba95fbeed8b9ccec9a296eea33afb9 Signed-off-by: William Roberts --- tools/check_seapp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/check_seapp.c b/tools/check_seapp.c index 84e28536a..33469e7ac 100644 --- a/tools/check_seapp.c +++ b/tools/check_seapp.c @@ -1095,7 +1095,7 @@ static void parse_file(file_info *in_file) { return; err: - log_error("reading %s, line %zu, name %s, value %s\n", + log_error("Reading file: \"%s\" line: %zu name: \"%s\" value: \"%s\"\n", in_file->name, lineno, name, value); if(found_whitespace && name && !strcasecmp(name, "neverallow")) { log_error("perhaps whitespace before neverallow\n"); From 696a66ba202780d757bd8a2d47db72473b7d558b Mon Sep 17 00:00:00 2001 From: William Roberts Date: Fri, 29 Jan 2016 10:41:50 -0800 Subject: [PATCH 3/5] checkseapp: generalize input validation Input validation was hard-coded into a validation routine that would check against type and key names in a scattered, order dependent conditional code block. This makes it harder than it should be to add new key value pairs and types into checkseapp. To correct this, we add a validation callback into the static mapping. If the validation callback is set, the existing validation routine will call this for input validation. On failure, a validation specific error message is returned to be displayed. Change-Id: I92cf1cdf4ddbcfae19168b621f47169a3cf551ac Signed-off-by: William Roberts --- tools/check_seapp.c | 141 ++++++++++++++++++++++++++------------------ 1 file changed, 83 insertions(+), 58 deletions(-) diff --git a/tools/check_seapp.c b/tools/check_seapp.c index 33469e7ac..50f18374c 100644 --- a/tools/check_seapp.c +++ b/tools/check_seapp.c @@ -113,6 +113,7 @@ struct key_map { data_type type; char *data; key_map_regex regex; + bool (*fn_validate)(char *value, char **errmsg); }; /** @@ -197,25 +198,31 @@ static list line_order_list = list_init(line_order_list_freefn); */ static list nallow_list = list_init(line_order_list_freefn); +/* validation call backs */ +static bool validate_bool(char *value, char **errmsg); +static bool validate_levelFrom(char *value, char **errmsg); +static bool validate_selinux_type(char *value, char **errmsg); +static bool validate_selinux_level(char *value, char **errmsg); + /** * The heart of the mapping process, this must be updated if a new key value pair is added * to a rule. */ key_map rules[] = { /*Inputs*/ - { .name = "isSystemServer", .type = dt_bool, .dir = dir_in, .data = NULL }, - { .name = "isOwner", .type = dt_bool, .dir = dir_in, .data = NULL }, - { .name = "user", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "seinfo", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "name", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "path", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "isPrivApp", .type = dt_bool, .dir = dir_in, .data = NULL }, + { .name = "isSystemServer", .type = dt_bool, .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, + { .name = "isOwner", .type = dt_bool, .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, + { .name = "user", .type = dt_string, .dir = dir_in, .data = NULL }, + { .name = "seinfo", .type = dt_string, .dir = dir_in, .data = NULL }, + { .name = "name", .type = dt_string, .dir = dir_in, .data = NULL }, + { .name = "path", .type = dt_string, .dir = dir_in, .data = NULL }, + { .name = "isPrivApp", .type = dt_bool, .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, /*Outputs*/ - { .name = "domain", .type = dt_string, .dir = dir_out, .data = NULL }, - { .name = "type", .type = dt_string, .dir = dir_out, .data = NULL }, - { .name = "levelFromUid", .type = dt_bool, .dir = dir_out, .data = NULL }, - { .name = "levelFrom", .type = dt_string, .dir = dir_out, .data = NULL }, - { .name = "level", .type = dt_string, .dir = dir_out, .data = NULL }, + { .name = "domain", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, + { .name = "type", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, + { .name = "levelFromUid", .type = dt_bool, .dir = dir_out, .data = NULL, .fn_validate = validate_bool }, + { .name = "levelFrom", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_levelFrom }, + { .name = "level", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_level }, }; /** @@ -352,6 +359,63 @@ static bool compile_regex(key_map *km, const char **errbuf, int *erroff) { return true; } +static bool validate_bool(char *value, char **errmsg) { + + if (!strcmp("true", value) || !strcmp("false", value)) { + return true; + } + + *errmsg = "Expecting \"true\" or \"false\""; + return false; +} + +static bool validate_levelFrom(char *value, char **errmsg) { + + if(strcasecmp(value, "none") && strcasecmp(value, "all") && + strcasecmp(value, "app") && strcasecmp(value, "user")) { + *errmsg = "Expecting one of: \"none\", \"all\", \"app\" or \"user\""; + return false; + } + return true; +} + +static bool validate_selinux_type(char *value, char **errmsg) { + + /* + * No policy file present means we cannot check + * SE Linux types + */ + if (!pol.policy_file) { + return true; + } + + if(!check_type(pol.db, value)) { + *errmsg = "Expecting a valid SELinux type"; + return false; + } + + return true; +} + +static bool validate_selinux_level(char *value, char **errmsg) { + + /* + * No policy file present means we cannot check + * SE Linux MLS + */ + if (!pol.policy_file) { + return true; + } + + int ret = sepol_mls_check(pol.handle, pol.db, value); + if (ret < 0) { + *errmsg = "Expecting a valid SELinux MLS value"; + return false; + } + + return true; +} + /** * Validates a key_map against a set of enforcement rules, this * function exits the application on a type that cannot be properly @@ -370,10 +434,9 @@ static bool key_map_validate(key_map *m, const char *filename, int lineno, int erroff; const char *errbuf; bool rc = true; - int ret = 1; char *key = m->name; char *value = m->data; - data_type type = m->type; + char *errmsg = NULL; log_info("Validating %s=%s\n", key, value); @@ -392,51 +455,13 @@ static bool key_map_validate(key_map *m, const char *filename, int lineno, goto out; } - /* Booleans can always be checked for sanity */ - if (type == dt_bool && (!strcmp("true", value) || !strcmp("false", value))) { - goto out; - } - else if (type == dt_bool) { - log_error("Expected boolean value got: %s=%s on line: %d in file: %s\n", - key, value, lineno, filename); - rc = false; - goto out; - } + /* If the key has a validation routine, call it */ + if (m->fn_validate) { + rc = m->fn_validate(value, &errmsg); - if (!strcasecmp(key, "levelFrom") && - (strcasecmp(value, "none") && strcasecmp(value, "all") && - strcasecmp(value, "app") && strcasecmp(value, "user"))) { - log_error("Unknown levelFrom=%s on line: %d in file: %s\n", - value, lineno, filename); - rc = false; - goto out; - } - - /* - * If there is no policy file present, - * then it is not going to enforce the types against the policy so just return. - * User and name cannot really be checked. - */ - if (!pol.policy_file) { - goto out; - } - else if (!strcasecmp(key, "type") || !strcasecmp(key, "domain")) { - - if(!check_type(pol.db, value)) { - log_error("Could not find selinux type \"%s\" on line: %d in file: %s\n", value, - lineno, filename); - rc = false; - } - goto out; - } - else if (!strcasecmp(key, "level")) { - - ret = sepol_mls_check(pol.handle, pol.db, value); - if (ret < 0) { - log_error("Could not find selinux level \"%s\", on line: %d in file: %s\n", value, - lineno, filename); - rc = false; - goto out; + if (!rc) { + log_error("Could not validate key \"%s\" for value \"%s\" on line: %d in file: \"%s\": %s\n", key, value, + lineno, filename, errmsg); } } From c92dae9807e890e7b8bdc8f293ef35143ad51ca5 Mon Sep 17 00:00:00 2001 From: William Roberts Date: Fri, 29 Jan 2016 11:03:40 -0800 Subject: [PATCH 4/5] checkseapp: remove data types form static map Data type tracking is no longer needed now that per key validation routines are supported. Change-Id: I2f1d0d5b1713e0477996479b0f279a58f43f15c7 Signed-off-by: William Roberts --- tools/check_seapp.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/tools/check_seapp.c b/tools/check_seapp.c index 50f18374c..aca294fbe 100644 --- a/tools/check_seapp.c +++ b/tools/check_seapp.c @@ -80,14 +80,6 @@ enum key_dir { dir_in, dir_out }; -/** - * The expected "type" of data the value in the key - * value pair should be. - */ -enum data_type { - dt_bool, dt_string -}; - struct list_element { list_element *next; }; @@ -110,7 +102,6 @@ struct key_map_regex { struct key_map { char *name; key_dir dir; - data_type type; char *data; key_map_regex regex; bool (*fn_validate)(char *value, char **errmsg); @@ -210,19 +201,19 @@ static bool validate_selinux_level(char *value, char **errmsg); */ key_map rules[] = { /*Inputs*/ - { .name = "isSystemServer", .type = dt_bool, .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, - { .name = "isOwner", .type = dt_bool, .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, - { .name = "user", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "seinfo", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "name", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "path", .type = dt_string, .dir = dir_in, .data = NULL }, - { .name = "isPrivApp", .type = dt_bool, .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, + { .name = "isSystemServer", .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, + { .name = "isOwner", .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, + { .name = "user", .dir = dir_in, .data = NULL }, + { .name = "seinfo", .dir = dir_in, .data = NULL }, + { .name = "name", .dir = dir_in, .data = NULL }, + { .name = "path", .dir = dir_in, .data = NULL }, + { .name = "isPrivApp", .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, /*Outputs*/ - { .name = "domain", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, - { .name = "type", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, - { .name = "levelFromUid", .type = dt_bool, .dir = dir_out, .data = NULL, .fn_validate = validate_bool }, - { .name = "levelFrom", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_levelFrom }, - { .name = "level", .type = dt_string, .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_level }, + { .name = "domain", .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, + { .name = "type", .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, + { .name = "levelFromUid", .dir = dir_out, .data = NULL, .fn_validate = validate_bool }, + { .name = "levelFrom", .dir = dir_out, .data = NULL, .fn_validate = validate_levelFrom }, + { .name = "level", .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_level }, }; /** @@ -518,9 +509,6 @@ static map_match rule_map_cmp(rule_map *rmA, rule_map *rmB) { mB = &(rmB->m[j]); input_mode = 0; - if (mA->type != mB->type) - continue; - if (strcmp(mA->name, mB->name)) continue; From 29adea51edfe190e29fd24397e0b8d7abbf5f5f6 Mon Sep 17 00:00:00 2001 From: William Roberts Date: Fri, 29 Jan 2016 15:12:58 -0800 Subject: [PATCH 5/5] checkseapp: remove .data = NULL assignments Remove the .data=NULL assignments that were pushing the static keymap mapping horizontal. Change-Id: I2e6e78930ac8d1d8b9bd61d9dedb59f4859ea13c Signed-off-by: William Roberts --- tools/check_seapp.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/check_seapp.c b/tools/check_seapp.c index aca294fbe..69db3889e 100644 --- a/tools/check_seapp.c +++ b/tools/check_seapp.c @@ -201,19 +201,19 @@ static bool validate_selinux_level(char *value, char **errmsg); */ key_map rules[] = { /*Inputs*/ - { .name = "isSystemServer", .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, - { .name = "isOwner", .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, - { .name = "user", .dir = dir_in, .data = NULL }, - { .name = "seinfo", .dir = dir_in, .data = NULL }, - { .name = "name", .dir = dir_in, .data = NULL }, - { .name = "path", .dir = dir_in, .data = NULL }, - { .name = "isPrivApp", .dir = dir_in, .data = NULL, .fn_validate = validate_bool }, + { .name = "isSystemServer", .dir = dir_in, .fn_validate = validate_bool }, + { .name = "isOwner", .dir = dir_in, .fn_validate = validate_bool }, + { .name = "user", .dir = dir_in, }, + { .name = "seinfo", .dir = dir_in, }, + { .name = "name", .dir = dir_in, }, + { .name = "path", .dir = dir_in, }, + { .name = "isPrivApp", .dir = dir_in, .fn_validate = validate_bool }, /*Outputs*/ - { .name = "domain", .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, - { .name = "type", .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_type }, - { .name = "levelFromUid", .dir = dir_out, .data = NULL, .fn_validate = validate_bool }, - { .name = "levelFrom", .dir = dir_out, .data = NULL, .fn_validate = validate_levelFrom }, - { .name = "level", .dir = dir_out, .data = NULL, .fn_validate = validate_selinux_level }, + { .name = "domain", .dir = dir_out, .fn_validate = validate_selinux_type }, + { .name = "type", .dir = dir_out, .fn_validate = validate_selinux_type }, + { .name = "levelFromUid", .dir = dir_out, .fn_validate = validate_bool }, + { .name = "levelFrom", .dir = dir_out, .fn_validate = validate_levelFrom }, + { .name = "level", .dir = dir_out, .fn_validate = validate_selinux_level }, }; /**