SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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, back to index

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

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox