| Message ID | alpine.LFD.2.21.1905110216330.19888@34-41-5D-CA-59-C7 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | General code cleanups, sanity checks and strdup() checks | expand |
On Fri, May 10, 2019 at 4:21 PM Jokke Hämäläinen <jokke.hamalainen@kolttonen.fi> wrote: > > > Needs to be analyzed for correctness. I would split this up into a series of patches, and obviously do some testing before you submit. > > --- > libsemanage/src/conf-parse.y | 56 ++++++++++++++++++++++++++++++++---- > 1 file changed, 51 insertions(+), 5 deletions(-) > > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y > index b527e893..75a43cd2 100644 > --- a/libsemanage/src/conf-parse.y > +++ b/libsemanage/src/conf-parse.y > @@ -221,6 +221,9 @@ usepasswd: USEPASSWD '=' ARG { > > ignoredirs: IGNOREDIRS '=' ARG { > current_conf->ignoredirs = strdup($3); > + if (current_conf->ignoredirs == NULL) { > + yyerror("could not allocate memory for current_conf->ignoredirs"); > + } > free($3); > } > > @@ -240,7 +243,7 @@ handle_unknown: HANDLE_UNKNOWN '=' ARG { > bzip_blocksize: BZIP_BLOCKSIZE '=' ARG { > int blocksize = atoi($3); > free($3); > - if (blocksize > 9) > + if (blocksize < 0 || blocksize > 9) BZ2_bzWriteOpen() takes an int, for len why?... anyways not our problem. However, their man page says < 1 causes an error: http://linux.math.tifr.res.in/manuals/html/manual_3.html So that range check is wrong. > yyerror("bzip-blocksize can only be in the range 0-9"); > else > current_conf->bzip_blocksize = blocksize; > @@ -338,10 +341,24 @@ external_opt: PROG_PATH '=' ARG { PASSIGN(new_external->path, $3); } > static int semanage_conf_init(semanage_conf_t * conf) > { > conf->store_type = SEMANAGE_CON_DIRECT; > + > conf->store_path = strdup(basename(selinux_policy_root())); > + if (conf->store_path == NULL) { > + return -1; > + } > + > conf->ignoredirs = NULL; > + > conf->store_root_path = strdup("/var/lib/selinux"); > + if (conf->store_root_path == NULL) { > + return -1; > + } > + > conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll"); > + if (conf->compiler_directory_path == NULL) { > + return -1; > + } > + > conf->policyvers = sepol_policy_kern_vers_max(); > conf->target_platform = SEPOL_TARGET_SELINUX; > conf->expand_check = 1; > @@ -366,20 +383,24 @@ static int semanage_conf_init(semanage_conf_t * conf) > } else { > conf->load_policy->path = strdup("/usr/sbin/load_policy"); > } > + > if (conf->load_policy->path == NULL) { > return -1; > } > + > conf->load_policy->args = NULL; > > if ((conf->setfiles = > calloc(1, sizeof(*(current_conf->setfiles)))) == NULL) { > return -1; > } > + I would separate out functional changes from style updates in different commits. > if (access("/sbin/setfiles", X_OK) == 0) { > conf->setfiles->path = strdup("/sbin/setfiles"); > } else { > conf->setfiles->path = strdup("/usr/sbin/setfiles"); > } > + > if ((conf->setfiles->path == NULL) || > (conf->setfiles->args = strdup("-q -c $@ $<")) == NULL) { > return -1; > @@ -389,11 +410,13 @@ static int semanage_conf_init(semanage_conf_t * conf) > calloc(1, sizeof(*(current_conf->sefcontext_compile)))) == NULL) { > return -1; > } > + > if (access("/sbin/sefcontext_compile", X_OK) == 0) { > conf->sefcontext_compile->path = strdup("/sbin/sefcontext_compile"); > } else { > conf->sefcontext_compile->path = strdup("/usr/sbin/sefcontext_compile"); > } > + > if ((conf->sefcontext_compile->path == NULL) || > (conf->sefcontext_compile->args = strdup("$@")) == NULL) { > return -1; > @@ -413,24 +436,30 @@ semanage_conf_t *semanage_conf_parse(const char *config_filename) > if ((current_conf = calloc(1, sizeof(*current_conf))) == NULL) { > return NULL; > } > + > if (semanage_conf_init(current_conf) == -1) { > goto cleanup; > } > + > if ((semanage_in = fopen(config_filename, "r")) == NULL) { > - /* configuration file does not exist or could not be > - * read. THIS IS NOT AN ERROR. just rely on the > + /* Configuration file does not exist or could not be > + * read. THIS IS NOT AN ERROR. Just rely on the > * defaults. */ Its still an oddly formatted block comment the *'s don't even line up, typically you see: /* * foo * bar */ or /* foo * bar */ Pick the former. > return current_conf; > } > + > parse_errors = 0; > semanage_parse(); > fclose(semanage_in); > semanage_lex_destroy(); > + > if (parse_errors != 0) { > goto cleanup; > } > + > return current_conf; > - cleanup: > + > +cleanup: > semanage_conf_destroy(current_conf); > return NULL; > } > @@ -489,7 +518,9 @@ static int parse_module_store(char *arg) > if (arg == NULL) { > return -1; > } > + > free(current_conf->store_path); > + > if (strcmp(arg, "direct") == 0) { > current_conf->store_type = SEMANAGE_CON_DIRECT; > current_conf->store_path = > @@ -515,7 +546,10 @@ static int parse_module_store(char *arg) > } > } > } > - return 0; > + > + if (current_conf->store_path) > + return 0; > + return -3; > } > > static int parse_store_root_path(char *arg) > @@ -525,7 +559,12 @@ static int parse_store_root_path(char *arg) > } > > free(current_conf->store_root_path); > + > current_conf->store_root_path = strdup(arg); > + if (current_conf->store_root_path == NULL) { > + return -2; > + } > + > return 0; > } > > @@ -534,8 +573,13 @@ static int parse_compiler_path(char *arg) > if (arg == NULL) { > return -1; > } > + > free(current_conf->compiler_directory_path); > + > current_conf->compiler_directory_path = strdup(arg); > + if (current_conf->compiler_directory_path = NULL) { > + return -2; > + } > return 0; > } > > @@ -548,6 +592,7 @@ static int new_external_prog(external_prog_t ** chain) > if ((new_external = calloc(1, sizeof(*new_external))) == NULL) { > return -1; > } > + > /* hook this new external program to the end of the chain */ > if (*chain == NULL) { > *chain = new_external; > @@ -558,5 +603,6 @@ static int new_external_prog(external_prog_t ** chain) > } > prog->next = new_external; > } > + > return 0; > } > -- > 2.21.0 >
diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y index b527e893..75a43cd2 100644 --- a/libsemanage/src/conf-parse.y +++ b/libsemanage/src/conf-parse.y @@ -221,6 +221,9 @@ usepasswd: USEPASSWD '=' ARG { ignoredirs: IGNOREDIRS '=' ARG { current_conf->ignoredirs = strdup($3); + if (current_conf->ignoredirs == NULL) { + yyerror("could not allocate memory for current_conf->ignoredirs"); + } free($3); } @@ -240,7 +243,7 @@ handle_unknown: HANDLE_UNKNOWN '=' ARG { bzip_blocksize: BZIP_BLOCKSIZE '=' ARG { int blocksize = atoi($3); free($3); - if (blocksize > 9) + if (blocksize < 0 || blocksize > 9) yyerror("bzip-blocksize can only be in the range 0-9"); else current_conf->bzip_blocksize = blocksize; @@ -338,10 +341,24 @@ external_opt: PROG_PATH '=' ARG { PASSIGN(new_external->path, $3); } static int semanage_conf_init(semanage_conf_t * conf) { conf->store_type = SEMANAGE_CON_DIRECT; + conf->store_path = strdup(basename(selinux_policy_root())); + if (conf->store_path == NULL) { + return -1; + } + conf->ignoredirs = NULL; + conf->store_root_path = strdup("/var/lib/selinux"); + if (conf->store_root_path == NULL) { + return -1; + } + conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll"); + if (conf->compiler_directory_path == NULL) { + return -1; + } + conf->policyvers = sepol_policy_kern_vers_max(); conf->target_platform = SEPOL_TARGET_SELINUX; conf->expand_check = 1; @@ -366,20 +383,24 @@ static int semanage_conf_init(semanage_conf_t * conf) } else { conf->load_policy->path = strdup("/usr/sbin/load_policy"); } + if (conf->load_policy->path == NULL) { return -1; } + conf->load_policy->args = NULL; if ((conf->setfiles = calloc(1, sizeof(*(current_conf->setfiles)))) == NULL) { return -1; } + if (access("/sbin/setfiles", X_OK) == 0) { conf->setfiles->path = strdup("/sbin/setfiles"); } else { conf->setfiles->path = strdup("/usr/sbin/setfiles"); } + if ((conf->setfiles->path == NULL) || (conf->setfiles->args = strdup("-q -c $@ $<")) == NULL) { return -1; @@ -389,11 +410,13 @@ static int semanage_conf_init(semanage_conf_t * conf) calloc(1, sizeof(*(current_conf->sefcontext_compile)))) == NULL) { return -1; } + if (access("/sbin/sefcontext_compile", X_OK) == 0) { conf->sefcontext_compile->path = strdup("/sbin/sefcontext_compile"); } else { conf->sefcontext_compile->path = strdup("/usr/sbin/sefcontext_compile"); } + if ((conf->sefcontext_compile->path == NULL) || (conf->sefcontext_compile->args = strdup("$@")) == NULL) { return -1; @@ -413,24 +436,30 @@ semanage_conf_t *semanage_conf_parse(const char *config_filename) if ((current_conf = calloc(1, sizeof(*current_conf))) == NULL) { return NULL; } + if (semanage_conf_init(current_conf) == -1) { goto cleanup; } + if ((semanage_in = fopen(config_filename, "r")) == NULL) { - /* configuration file does not exist or could not be - * read. THIS IS NOT AN ERROR. just rely on the + /* Configuration file does not exist or could not be + * read. THIS IS NOT AN ERROR. Just rely on the * defaults. */ return current_conf; } + parse_errors = 0; semanage_parse(); fclose(semanage_in); semanage_lex_destroy(); + if (parse_errors != 0) { goto cleanup; } + return current_conf; - cleanup: + +cleanup: semanage_conf_destroy(current_conf); return NULL; } @@ -489,7 +518,9 @@ static int parse_module_store(char *arg) if (arg == NULL) { return -1; } + free(current_conf->store_path); + if (strcmp(arg, "direct") == 0) { current_conf->store_type = SEMANAGE_CON_DIRECT; current_conf->store_path = @@ -515,7 +546,10 @@ static int parse_module_store(char *arg) } } } - return 0; + + if (current_conf->store_path) + return 0; + return -3; } static int parse_store_root_path(char *arg) @@ -525,7 +559,12 @@ static int parse_store_root_path(char *arg) } free(current_conf->store_root_path); + current_conf->store_root_path = strdup(arg); + if (current_conf->store_root_path == NULL) { + return -2; + } + return 0; } @@ -534,8 +573,13 @@ static int parse_compiler_path(char *arg) if (arg == NULL) { return -1; } + free(current_conf->compiler_directory_path); + current_conf->compiler_directory_path = strdup(arg); + if (current_conf->compiler_directory_path = NULL) { + return -2; + } return 0; } @@ -548,6 +592,7 @@ static int new_external_prog(external_prog_t ** chain) if ((new_external = calloc(1, sizeof(*new_external))) == NULL) { return -1; } + /* hook this new external program to the end of the chain */ if (*chain == NULL) { *chain = new_external; @@ -558,5 +603,6 @@ static int new_external_prog(external_prog_t ** chain) } prog->next = new_external; } + return 0; }