* [PATCH] General code cleanups, sanity checks and strdup() checks
@ 2019-05-10 23:18 Jokke Hämäläinen
2019-05-16 0:50 ` William Roberts
0 siblings, 1 reply; 2+ messages in thread
From: Jokke Hämäläinen @ 2019-05-10 23:18 UTC (permalink / raw)
To: selinux
Needs to be analyzed for correctness.
---
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)
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;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] General code cleanups, sanity checks and strdup() checks
2019-05-10 23:18 [PATCH] General code cleanups, sanity checks and strdup() checks Jokke Hämäläinen
@ 2019-05-16 0:50 ` William Roberts
0 siblings, 0 replies; 2+ messages in thread
From: William Roberts @ 2019-05-16 0:50 UTC (permalink / raw)
To: Jokke Hämäläinen; +Cc: selinux
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-05-16 1:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 23:18 [PATCH] General code cleanups, sanity checks and strdup() checks Jokke Hämäläinen
2019-05-16 0:50 ` William Roberts
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).