SELinux Archive on lore.kernel.org
 help / Atom feed
* [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan
@ 2019-01-31 13:22 Petr Lautrbach
  2019-01-31 13:22 ` [PATCH 2/3] libselinux: " Petr Lautrbach
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Petr Lautrbach @ 2019-01-31 13:22 UTC (permalink / raw)
  To: selinux; +Cc: Petr Lautrbach

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
 libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
 libsepol/cil/src/cil_symtab.c      |  1 +
 libsepol/src/expand.c              |  3 +++
 libsepol/src/kernel_to_cil.c       |  2 ++
 libsepol/src/kernel_to_conf.c      |  2 ++
 6 files changed, 30 insertions(+)

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 0cc6eeb1..a645c95d 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -912,6 +912,8 @@ int cil_sensalias_to_policydb(policydb_t *pdb, struct cil_alias *cil_alias)
 	key = cil_strdup(cil_alias->datum.fqn);
 	rc = symtab_insert(pdb, SYM_LEVELS, key, sepol_alias, SCOPE_DECL, 0, NULL);
 	if (rc != SEPOL_OK) {
+		if (rc == 1)
+			free(sepol_alias);
 		goto exit;
 	}
 
@@ -1763,11 +1765,13 @@ int __cil_avrulex_ioctl_to_hashtable(hashtab_t h, uint16_t kind, uint32_t src, u
 		hashtab_xperms = cil_malloc(sizeof(*hashtab_xperms));
 		rc = ebitmap_cpy(hashtab_xperms, xperms);
 		if (rc != SEPOL_OK) {
+			free(hashtab_xperms);
 			free(avtab_key);
 			goto exit;
 		}
 		rc = hashtab_insert(h, (hashtab_key_t)avtab_key, hashtab_xperms);
 		if (rc != SEPOL_OK) {
+			free(hashtab_xperms);
 			free(avtab_key);
 			goto exit;
 		}
@@ -2157,6 +2161,7 @@ static int __cil_cond_expr_to_sepol_expr_helper(policydb_t *pdb, struct cil_list
 			op->expr_type = COND_NEQ;
 			break;
 		default:
+			free(op);
 			goto exit;
 		}
 
@@ -2283,6 +2288,7 @@ int cil_booleanif_to_policydb(policydb_t *pdb, const struct cil_db *db, struct c
 
 	cond_expr_destroy(tmp_cond->expr);
 	free(tmp_cond);
+	tmp_cond = NULL;
 
 	for (cb_node = node->cl_head; cb_node != NULL; cb_node = cb_node->next) {
 		if (cb_node->flavor == CIL_CONDBLOCK) {
@@ -2327,6 +2333,11 @@ int cil_booleanif_to_policydb(policydb_t *pdb, const struct cil_db *db, struct c
 	return SEPOL_OK;
 
 exit:
+	if (tmp_cond) {
+		if (tmp_cond->expr)
+			cond_expr_destroy(tmp_cond->expr);
+		free(tmp_cond);
+	}
 	return rc;
 }
 
@@ -4797,6 +4808,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
 	return cp_list;
 
 exit:
+	free(cp);
 	cil_log(CIL_ERR,"Failed to create CIL class-permissions from sepol values\n");
 	return NULL;
 }
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index fb9d9174..91187ed7 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -1522,6 +1522,7 @@ int cil_resolve_sidorder(struct cil_tree_node *current, void *extra_args)
 		rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SIDS, extra_args, &datum);
 		if (rc != SEPOL_OK) {
 			cil_log(CIL_ERR, "Failed to resolve sid %s in sidorder\n", (char *)curr->data);
+			free(new);
 			goto exit;
 		}
 		cil_list_append(new, CIL_SID, datum);
@@ -1591,6 +1592,8 @@ int cil_resolve_catorder(struct cil_tree_node *current, void *extra_args)
 	return SEPOL_OK;
 
 exit:
+	if (new)
+		free(new);
 	return rc;
 }
 
@@ -1624,6 +1627,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
 	return SEPOL_OK;
 
 exit:
+	free(new);
 	return rc;
 }
 
@@ -2853,6 +2857,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 					rc = cil_fill_cats(pc, &catset->cats);
 					if (rc != SEPOL_OK) {
 						cil_destroy_catset(catset);
+						cil_destroy_args(new_arg);
 						goto exit;
 					}
 					cil_tree_node_init(&cat_node);
@@ -2877,6 +2882,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 					if (rc != SEPOL_OK) {
 						cil_log(CIL_ERR, "Failed to create anonymous level, rc: %d\n", rc);
 						cil_destroy_level(level);
+						cil_destroy_args(new_arg);
 						goto exit;
 					}
 					cil_tree_node_init(&lvl_node);
@@ -2901,6 +2907,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 					if (rc != SEPOL_OK) {
 						cil_log(CIL_ERR, "Failed to create anonymous levelrange, rc: %d\n", rc);
 						cil_destroy_levelrange(range);
+						cil_destroy_args(new_arg);
 						goto exit;
 					}
 					cil_tree_node_init(&range_node);
@@ -2925,6 +2932,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 					if (rc != SEPOL_OK) {
 						cil_log(CIL_ERR, "Failed to create anonymous ip address, rc: %d\n", rc);
 						cil_destroy_ipaddr(ipaddr);
+						cil_destroy_args(new_arg);
 						goto exit;
 					}
 					cil_tree_node_init(&addr_node);
@@ -2955,6 +2963,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 					if (rc != SEPOL_OK) {
 						cil_log(CIL_ERR, "Failed to create anonymous classpermission\n");
 						cil_destroy_classpermission(cp);
+						cil_destroy_args(new_arg);
 						goto exit;
 					}
 					cil_tree_node_init(&cp_node);
@@ -2970,6 +2979,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 			default:
 				cil_log(CIL_ERR, "Unexpected flavor: %d\n", 
 						(((struct cil_param*)item->data)->flavor));
+				cil_destroy_args(new_arg);
 				rc = SEPOL_ERR;
 				goto exit;
 			}
diff --git a/libsepol/cil/src/cil_symtab.c b/libsepol/cil/src/cil_symtab.c
index 3769979b..2970b863 100644
--- a/libsepol/cil/src/cil_symtab.c
+++ b/libsepol/cil/src/cil_symtab.c
@@ -185,6 +185,7 @@ int cil_complex_symtab_insert(struct cil_complex_symtab *symtab,
 			ckey->key2 == curr->ckey->key2 &&
 			ckey->key3 == curr->ckey->key3 &&
 			ckey->key4 == curr->ckey->key4) {
+			free(node);
 			return SEPOL_EEXIST;
 		}
 
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 6f1b235e..125a6809 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1451,6 +1451,7 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
 				new_trans->name = strdup(cur_rule->name);
 				if (!new_trans->name) {
 					ERR(state->handle, "Out of memory!");
+					free(new_trans);
 					return -1;
 				}
 				new_trans->stype = i + 1;
@@ -1460,6 +1461,8 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
 				otype = calloc(1, sizeof(*otype));
 				if (!otype) {
 					ERR(state->handle, "Out of memory!");
+					free(new_trans->name);
+					free(new_trans);
 					return -1;
 				}
 				otype->otype = mapped_otype;
diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 63e4d489..ec82f597 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -2031,6 +2031,8 @@ static int write_cond_av_list_to_cil(FILE *out, struct policydb *pdb, cond_av_li
 	return 0;
 
 exit:
+	strs_free_all(strs);
+	strs_destroy(&strs);
 	return rc;
 }
 
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 215d940c..afadca6b 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -1999,6 +1999,8 @@ static int write_cond_av_list_to_conf(FILE *out, struct policydb *pdb, cond_av_l
 	return 0;
 
 exit:
+	strs_free_all(strs);
+	strs_destroy(&strs);
 	return rc;
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3] libselinux: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 13:22 [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan Petr Lautrbach
@ 2019-01-31 13:22 ` " Petr Lautrbach
  2019-01-31 21:47   ` Nicolas Iooss
  2019-01-31 13:22 ` [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE " Petr Lautrbach
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Petr Lautrbach @ 2019-01-31 13:22 UTC (permalink / raw)
  To: selinux; +Cc: Petr Lautrbach

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 libselinux/src/checkAccess.c        |  4 +++-
 libselinux/src/label_db.c           |  3 +++
 libselinux/src/label_file.c         |  4 +++-
 libselinux/src/load_policy.c        |  4 +++-
 libselinux/src/selinux_config.c     | 17 +++++++++--------
 libselinux/src/selinux_restorecon.c | 12 ++++++++++--
 6 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
index 8de57477..16bfcfb6 100644
--- a/libselinux/src/checkAccess.c
+++ b/libselinux/src/checkAccess.c
@@ -89,8 +89,10 @@ int selinux_check_passwd_access(access_vector_t requested)
 		int retval;
 
 		passwd_class = string_to_security_class("passwd");
-		if (passwd_class == 0)
+		if (passwd_class == 0) {
+			freecon(user_context);
 			return 0;
+		}
 
 		retval = security_compute_av_raw(user_context,
 						     user_context,
diff --git a/libselinux/src/label_db.c b/libselinux/src/label_db.c
index c46d0a1d..fa481e04 100644
--- a/libselinux/src/label_db.c
+++ b/libselinux/src/label_db.c
@@ -283,10 +283,12 @@ db_init(const struct selinux_opt *opts, unsigned nopts,
 	}
 	if (fstat(fileno(filp), &sb) < 0) {
 		free(catalog);
+    fclose(filp);
 		return NULL;
 	}
 	if (!S_ISREG(sb.st_mode)) {
 		free(catalog);
+    fclose(filp);
 		errno = EINVAL;
 		return NULL;
 	}
@@ -340,6 +342,7 @@ out_error:
 		free(spec->lr.ctx_trans);
 	}
 	free(catalog);
+	fclose(filp);
 
 	return NULL;
 }
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index dbf51a93..b81fd552 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -317,8 +317,10 @@ end_arch_check:
 			goto out;
 		}
 		rc = next_entry(str_buf, mmap_area, entry_len);
-		if (rc < 0)
+		if (rc < 0) {
+			free(str_buf);
 			goto out;
+		}
 
 		if (str_buf[entry_len - 1] != '\0') {
 			free(str_buf);
diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index e9f1264a..20052beb 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -262,8 +262,10 @@ checkbool:
 			rc = security_get_boolean_names(&names, &len);
 			if (!rc) {
 				values = malloc(sizeof(int) * len);
-				if (!values)
+				if (!values) {
+					free(names);
 					goto unmap;
+				}
 				for (i = 0; i < len; i++)
 					values[i] =
 						security_get_boolean_active(names[i]);
diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
index 292728f3..b06cb63b 100644
--- a/libselinux/src/selinux_config.c
+++ b/libselinux/src/selinux_config.c
@@ -177,8 +177,7 @@ static void init_selinux_config(void)
 
 			if (!strncasecmp(buf_p, SELINUXTYPETAG,
 					 sizeof(SELINUXTYPETAG) - 1)) {
-				selinux_policytype = type =
-				    strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
+				type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
 				if (!type)
 					return;
 				end = type + strlen(type) - 1;
@@ -187,6 +186,11 @@ static void init_selinux_config(void)
 					*end = 0;
 					end--;
 				}
+				if (setpolicytype(type) != 0) {
+					free(type);
+					return;
+				}
+				free(type);
 				continue;
 			} else if (!strncmp(buf_p, SETLOCALDEFS,
 					    sizeof(SETLOCALDEFS) - 1)) {
@@ -212,13 +216,10 @@ static void init_selinux_config(void)
 		fclose(fp);
 	}
 
-	if (!type) {
-		selinux_policytype = type = strdup(SELINUXDEFAULT);
-		if (!type)
-			return;
-	}
+	if (!selinux_policytype && setpolicytype(SELINUXDEFAULT) != 0)
+		return;
 
-	if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, type) == -1)
+	if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, selinux_policytype) == -1)
 		return;
 
 	for (i = 0; i < NEL; i++)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 42a48f5a..955732e5 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -351,12 +351,19 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch,
 	new_entry->next = NULL;
 
 	new_entry->directory = strdup(directory);
-	if (!new_entry->directory)
+	if (!new_entry->directory) {
+		free(new_entry);
+		free(sha1_buf);
 		goto oom;
+	}
 
 	new_entry->digest = strdup(sha1_buf);
-	if (!new_entry->digest)
+	if (!new_entry->digest) {
+		free(new_entry->directory);
+		free(new_entry);
+		free(sha1_buf);
 		goto oom;
+	}
 
 	new_entry->result = digest_result;
 
@@ -850,6 +857,7 @@ int selinux_restorecon(const char *pathname_orig,
 
 	if (lstat(pathname, &sb) < 0) {
 		if (flags.ignore_noent && errno == ENOENT) {
+			free(xattr_value);
 			free(pathdnamer);
 			free(pathname);
 			return 0;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE defects reported by coverity scan
  2019-01-31 13:22 [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan Petr Lautrbach
  2019-01-31 13:22 ` [PATCH 2/3] libselinux: " Petr Lautrbach
@ 2019-01-31 13:22 ` " Petr Lautrbach
  2019-01-31 21:57   ` Nicolas Iooss
  2019-01-31 21:17 ` [PATCH 1/3] libsepol: Fix RESOURCE_LEAK " Nicolas Iooss
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Petr Lautrbach @ 2019-01-31 13:22 UTC (permalink / raw)
  To: selinux; +Cc: Petr Lautrbach

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 libsemanage/src/direct_api.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index c58961be..8e4d116d 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -1028,7 +1028,7 @@ static int semanage_direct_write_langext(semanage_handle_t *sh,
 
 	fp = NULL;
 
-	ret = 0;
+	return 0;
 
 cleanup:
 	if (fp != NULL) fclose(fp);
@@ -2177,7 +2177,6 @@ cleanup:
 	semanage_module_info_destroy(sh, modinfo);
 	free(modinfo);
 
-	if (fp != NULL) fclose(fp);
 	return status;
 }
 
@@ -2342,16 +2341,6 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
 	free(tmp);
 	tmp = NULL;
 
-	if (fclose(fp) != 0) {
-		ERR(sh,
-		    "Unable to close %s module lang ext file.",
-		    (*modinfo)->name);
-		status = -1;
-		goto cleanup;
-	}
-
-	fp = NULL;
-
 	/* lookup enabled/disabled status */
 	ret = semanage_module_get_path(sh,
 				       *modinfo,
@@ -2395,7 +2384,13 @@ cleanup:
 		free(modinfos);
 	}
 
-	if (fp != NULL) fclose(fp);
+	if (fp != NULL && fclose(fp) != 0) {
+		ERR(sh,
+		    "Unable to close %s module lang ext file.",
+		    (*modinfo)->name);
+		status = -1;
+	}
+
 	return status;
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 13:22 [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan Petr Lautrbach
  2019-01-31 13:22 ` [PATCH 2/3] libselinux: " Petr Lautrbach
  2019-01-31 13:22 ` [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE " Petr Lautrbach
@ 2019-01-31 21:17 ` " Nicolas Iooss
  2019-02-01 18:19   ` [Non-DoD Source] " jwcart2
  2019-01-31 21:33 ` Nicolas Iooss
  2019-02-01 20:07 ` [Non-DoD Source] " jwcart2
  4 siblings, 1 reply; 14+ messages in thread
From: Nicolas Iooss @ 2019-01-31 21:17 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>  libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>  libsepol/cil/src/cil_symtab.c      |  1 +
>  libsepol/src/expand.c              |  3 +++
>  libsepol/src/kernel_to_cil.c       |  2 ++
>  libsepol/src/kernel_to_conf.c      |  2 ++
>  6 files changed, 30 insertions(+)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 0cc6eeb1..a645c95d 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -912,6 +912,8 @@ int cil_sensalias_to_policydb(policydb_t *pdb, struct cil_alias *cil_alias)
>         key = cil_strdup(cil_alias->datum.fqn);
>         rc = symtab_insert(pdb, SYM_LEVELS, key, sepol_alias, SCOPE_DECL, 0, NULL);
>         if (rc != SEPOL_OK) {
> +               if (rc == 1)
> +                       free(sepol_alias);
>                 goto exit;
>         }

There is something weird here. The exit label starts with
"level_datum_destroy(sepol_alias);". This is not a serious issue
because level_datum_destroy() does not do anything, but after this
patch, cil_sensalias_to_policydb()'s code seems to use sepol_alias
after freeing it.

Should the call to level_datum_destroy(sepol_alias) be removed, or
moved before free(sepol_alias)?

Thanks,
Nicolas


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 13:22 [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan Petr Lautrbach
                   ` (2 preceding siblings ...)
  2019-01-31 21:17 ` [PATCH 1/3] libsepol: Fix RESOURCE_LEAK " Nicolas Iooss
@ 2019-01-31 21:33 ` Nicolas Iooss
  2019-02-01 19:32   ` [Non-DoD Source] " jwcart2
  2019-02-01 20:07 ` [Non-DoD Source] " jwcart2
  4 siblings, 1 reply; 14+ messages in thread
From: Nicolas Iooss @ 2019-01-31 21:33 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>  libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>  libsepol/cil/src/cil_symtab.c      |  1 +
>  libsepol/src/expand.c              |  3 +++
>  libsepol/src/kernel_to_cil.c       |  2 ++
>  libsepol/src/kernel_to_conf.c      |  2 ++
>  6 files changed, 30 insertions(+)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 0cc6eeb1..a645c95d 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
...
> @@ -4797,6 +4808,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
>         return cp_list;
>
>  exit:
> +       free(cp);
>         cil_log(CIL_ERR,"Failed to create CIL class-permissions from sepol values\n");
>         return NULL;
>  }

Before free(cp), should cp->perms be destroyed with a call to
cil_list_destroy(&cp->perms, CIL_FALSE)?

> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index fb9d9174..91187ed7 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -1522,6 +1522,7 @@ int cil_resolve_sidorder(struct cil_tree_node *current, void *extra_args)
>                 rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SIDS, extra_args, &datum);
>                 if (rc != SEPOL_OK) {
>                         cil_log(CIL_ERR, "Failed to resolve sid %s in sidorder\n", (char *)curr->data);
> +                       free(new);
>                         goto exit;
>                 }
>                 cil_list_append(new, CIL_SID, datum);

Here, free(new) will not free the items that were already appended.
Would cil_list_destroy(&new, CIL_FALSE) work? (I have not tested it)

> @@ -1591,6 +1592,8 @@ int cil_resolve_catorder(struct cil_tree_node *current, void *extra_args)
>         return SEPOL_OK;
>
>  exit:
> +       if (new)
> +               free(new);
>         return rc;
>  }

Same comment

> @@ -1624,6 +1627,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
>         return SEPOL_OK;
>
>  exit:
> +       free(new);
>         return rc;
>  }

Why is there a "if(new)" in the previous chunk and not here? As
cil_list_init() fails hard when the memory allocation failed, new can
never be NULL so the previous if(new) is not needed.

All the other changes in this patch looked good to me.

Nicolas


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] libselinux: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 13:22 ` [PATCH 2/3] libselinux: " Petr Lautrbach
@ 2019-01-31 21:47   ` Nicolas Iooss
  2019-02-06 20:33     ` [PATCH v2] " Petr Lautrbach
  0 siblings, 1 reply; 14+ messages in thread
From: Nicolas Iooss @ 2019-01-31 21:47 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libselinux/src/checkAccess.c        |  4 +++-
>  libselinux/src/label_db.c           |  3 +++
>  libselinux/src/label_file.c         |  4 +++-
>  libselinux/src/load_policy.c        |  4 +++-
>  libselinux/src/selinux_config.c     | 17 +++++++++--------
>  libselinux/src/selinux_restorecon.c | 12 ++++++++++--
>  6 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
> index 8de57477..16bfcfb6 100644
> --- a/libselinux/src/checkAccess.c
> +++ b/libselinux/src/checkAccess.c
> @@ -89,8 +89,10 @@ int selinux_check_passwd_access(access_vector_t requested)
>                 int retval;
>
>                 passwd_class = string_to_security_class("passwd");
> -               if (passwd_class == 0)
> +               if (passwd_class == 0) {
> +                       freecon(user_context);
>                         return 0;
> +               }
>
>                 retval = security_compute_av_raw(user_context,
>                                                      user_context,
> diff --git a/libselinux/src/label_db.c b/libselinux/src/label_db.c
> index c46d0a1d..fa481e04 100644
> --- a/libselinux/src/label_db.c
> +++ b/libselinux/src/label_db.c
> @@ -283,10 +283,12 @@ db_init(const struct selinux_opt *opts, unsigned nopts,
>         }
>         if (fstat(fileno(filp), &sb) < 0) {
>                 free(catalog);
> +    fclose(filp);
>                 return NULL;
>         }
>         if (!S_ISREG(sb.st_mode)) {
>                 free(catalog);
> +    fclose(filp);
>                 errno = EINVAL;
>                 return NULL;
>         }

Please indent with tabs instead of spaces, like the other lines.
All the other changes in this patch look good to me.

Nicolas


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE defects reported by coverity scan
  2019-01-31 13:22 ` [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE " Petr Lautrbach
@ 2019-01-31 21:57   ` Nicolas Iooss
  2019-02-01 12:45     ` Petr Lautrbach
  0 siblings, 1 reply; 14+ messages in thread
From: Nicolas Iooss @ 2019-01-31 21:57 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libsemanage/src/direct_api.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index c58961be..8e4d116d 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -1028,7 +1028,7 @@ static int semanage_direct_write_langext(semanage_handle_t *sh,
>
>         fp = NULL;
>
> -       ret = 0;
> +       return 0;
>
>  cleanup:
>         if (fp != NULL) fclose(fp);
> @@ -2177,7 +2177,6 @@ cleanup:
>         semanage_module_info_destroy(sh, modinfo);
>         free(modinfo);
>
> -       if (fp != NULL) fclose(fp);
>         return status;
>  }
>
> @@ -2342,16 +2341,6 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>         free(tmp);
>         tmp = NULL;
>
> -       if (fclose(fp) != 0) {
> -               ERR(sh,
> -                   "Unable to close %s module lang ext file.",
> -                   (*modinfo)->name);
> -               status = -1;
> -               goto cleanup;
> -       }
> -
> -       fp = NULL;
> -
>         /* lookup enabled/disabled status */
>         ret = semanage_module_get_path(sh,
>                                        *modinfo,
> @@ -2395,7 +2384,13 @@ cleanup:
>                 free(modinfos);
>         }
>
> -       if (fp != NULL) fclose(fp);
> +       if (fp != NULL && fclose(fp) != 0) {
> +               ERR(sh,
> +                   "Unable to close %s module lang ext file.",
> +                   (*modinfo)->name);
> +               status = -1;
> +       }
> +
>         return status;
>  }
>
> --
> 2.20.1

After reading this patch, I am wondering what the USE_AFTER_FREE
defects were about. Were there real use-after-fclose bugs that are
fixed by this patch? Or is this patch more about silencing Coverity's
false positives due to difficulties in parsing constructions such as
"fclose(fp);fp = NULL; if(f != NULL)fclose(fp);"? It would be useful
if the patch description contains answers to these questions.

Otherwise the content of the patch looks good to me.

Nicolas


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE defects reported by coverity scan
  2019-01-31 21:57   ` Nicolas Iooss
@ 2019-02-01 12:45     ` Petr Lautrbach
  0 siblings, 0 replies; 14+ messages in thread
From: Petr Lautrbach @ 2019-02-01 12:45 UTC (permalink / raw)
  To: selinux; +Cc: Nicolas Iooss


Nicolas Iooss <nicolas.iooss@m4x.org> writes:

> On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach 
> <plautrba@redhat.com> wrote:
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>  libsemanage/src/direct_api.c | 21 ++++++++-------------
>>  1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/libsemanage/src/direct_api.c 
>> b/libsemanage/src/direct_api.c
>> index c58961be..8e4d116d 100644
>> --- a/libsemanage/src/direct_api.c
>> +++ b/libsemanage/src/direct_api.c
>> @@ -1028,7 +1028,7 @@ static int 
>> semanage_direct_write_langext(semanage_handle_t *sh,
>>
>>         fp = NULL;
>>
>> -       ret = 0;
>> +       return 0;
>>
>>  cleanup:
>>         if (fp != NULL) fclose(fp);
>> @@ -2177,7 +2177,6 @@ cleanup:
>>         semanage_module_info_destroy(sh, modinfo);
>>         free(modinfo);
>>
>> -       if (fp != NULL) fclose(fp);
>>         return status;
>>  }
>>
>> @@ -2342,16 +2341,6 @@ static int 
>> semanage_direct_get_module_info(semanage_handle_t *sh,
>>         free(tmp);
>>         tmp = NULL;
>>
>> -       if (fclose(fp) != 0) {
>> -               ERR(sh,
>> -                   "Unable to close %s module lang ext file.",
>> -                   (*modinfo)->name);
>> -               status = -1;
>> -               goto cleanup;
>> -       }
>> -
>> -       fp = NULL;
>> -
>>         /* lookup enabled/disabled status */
>>         ret = semanage_module_get_path(sh,
>>                                        *modinfo,
>> @@ -2395,7 +2384,13 @@ cleanup:
>>                 free(modinfos);
>>         }
>>
>> -       if (fp != NULL) fclose(fp);
>> +       if (fp != NULL && fclose(fp) != 0) {
>> +               ERR(sh,
>> +                   "Unable to close %s module lang ext file.",
>> +                   (*modinfo)->name);
>> +               status = -1;
>> +       }
>> +
>>         return status;
>>  }
>>
>> --
>> 2.20.1
>
> After reading this patch, I am wondering what the USE_AFTER_FREE
> defects were about. Were there real use-after-fclose bugs that 
> are
> fixed by this patch? Or is this patch more about silencing 
> Coverity's
> false positives due to difficulties in parsing constructions 
> such as
> "fclose(fp);fp = NULL; if(f != NULL)fclose(fp);"? It would be 
> useful
> if the patch description contains answers to these questions.


Diff between scans before and after shows that it fixes at least 
the two
following defects:

Error: USE_AFTER_FREE (CWE-672):
libsemanage-2.8/src/direct_api.c:2142: freed_arg: "fclose" frees 
"fp".
libsemanage-2.8/src/direct_api.c:2180: use_closed_file: Calling 
"fclose" uses file handle "fp" after closing it.
# 2178|   	free(modinfo);
# 2179|   
# 2180|-> 	if (fp != NULL) fclose(fp);
# 2181|   	return status;
# 2182|   }

Error: USE_AFTER_FREE (CWE-672):
libsemanage-2.8/src/direct_api.c:2345: freed_arg: "fclose" frees 
"fp".
libsemanage-2.8/src/direct_api.c:2398: use_closed_file: Calling 
"fclose" uses file handle "fp" after closing it.
# 2396|   	}
# 2397|   
# 2398|-> 	if (fp != NULL) fclose(fp);
# 2399|   	return status;
# 2400|   }


But it was supposed to fix more defects and reading it again it 
would be
better to assign NULL to fp on line 2349 instead of moving the 
whole
code block to cleanup.

I'll resend new set of patches with better commit messages.

Thanks!

> Otherwise the content of the patch looks good to me.
>
> Nicolas


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Non-DoD Source] Re: [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 21:17 ` [PATCH 1/3] libsepol: Fix RESOURCE_LEAK " Nicolas Iooss
@ 2019-02-01 18:19   ` " jwcart2
  0 siblings, 0 replies; 14+ messages in thread
From: jwcart2 @ 2019-02-01 18:19 UTC (permalink / raw)
  To: Nicolas Iooss, Petr Lautrbach; +Cc: selinux

On 1/31/19 4:17 PM, Nicolas Iooss wrote:
> On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>   libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>>   libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>>   libsepol/cil/src/cil_symtab.c      |  1 +
>>   libsepol/src/expand.c              |  3 +++
>>   libsepol/src/kernel_to_cil.c       |  2 ++
>>   libsepol/src/kernel_to_conf.c      |  2 ++
>>   6 files changed, 30 insertions(+)
>>
>> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
>> index 0cc6eeb1..a645c95d 100644
>> --- a/libsepol/cil/src/cil_binary.c
>> +++ b/libsepol/cil/src/cil_binary.c
>> @@ -912,6 +912,8 @@ int cil_sensalias_to_policydb(policydb_t *pdb, struct cil_alias *cil_alias)
>>          key = cil_strdup(cil_alias->datum.fqn);
>>          rc = symtab_insert(pdb, SYM_LEVELS, key, sepol_alias, SCOPE_DECL, 0, NULL);
>>          if (rc != SEPOL_OK) {
>> +               if (rc == 1)
>> +                       free(sepol_alias);
>>                  goto exit;
>>          }
> 
> There is something weird here. The exit label starts with
> "level_datum_destroy(sepol_alias);". This is not a serious issue
> because level_datum_destroy() does not do anything, but after this
> patch, cil_sensalias_to_policydb()'s code seems to use sepol_alias
> after freeing it.
> 
> Should the call to level_datum_destroy(sepol_alias) be removed, or
> moved before free(sepol_alias)?
> 

The real problem is the statement after the "level_datum_destroy(sepol_alias);".
The "free(sepol_level);" should be "free(sepol_alias);".
The sepol_level references a datum passed back by hashtab_search() in 
__cil_get_sepol_class_datum() and is not a new object.

Jim

> Thanks,
> Nicolas
> 
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Non-DoD Source] Re: [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 21:33 ` Nicolas Iooss
@ 2019-02-01 19:32   ` " jwcart2
  0 siblings, 0 replies; 14+ messages in thread
From: jwcart2 @ 2019-02-01 19:32 UTC (permalink / raw)
  To: Nicolas Iooss, Petr Lautrbach; +Cc: selinux

On 1/31/19 4:33 PM, Nicolas Iooss wrote:
> On Thu, Jan 31, 2019 at 2:22 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>   libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>>   libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>>   libsepol/cil/src/cil_symtab.c      |  1 +
>>   libsepol/src/expand.c              |  3 +++
>>   libsepol/src/kernel_to_cil.c       |  2 ++
>>   libsepol/src/kernel_to_conf.c      |  2 ++
>>   6 files changed, 30 insertions(+)
>>
>> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
>> index 0cc6eeb1..a645c95d 100644
>> --- a/libsepol/cil/src/cil_binary.c
>> +++ b/libsepol/cil/src/cil_binary.c
> ...
>> @@ -4797,6 +4808,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
>>          return cp_list;
>>
>>   exit:
>> +       free(cp);
>>          cil_log(CIL_ERR,"Failed to create CIL class-permissions from sepol values\n");
>>          return NULL;
>>   }
> 
> Before free(cp), should cp->perms be destroyed with a call to
> cil_list_destroy(&cp->perms, CIL_FALSE)?

Instead of "free(cp);" use "cil_destroy_classperms(cp);" That will destroy the 
permissions list as well.

> 
>> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
>> index fb9d9174..91187ed7 100644
>> --- a/libsepol/cil/src/cil_resolve_ast.c
>> +++ b/libsepol/cil/src/cil_resolve_ast.c
>> @@ -1522,6 +1522,7 @@ int cil_resolve_sidorder(struct cil_tree_node *current, void *extra_args)
>>                  rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_SIDS, extra_args, &datum);
>>                  if (rc != SEPOL_OK) {
>>                          cil_log(CIL_ERR, "Failed to resolve sid %s in sidorder\n", (char *)curr->data);
>> +                       free(new);
>>                          goto exit;
>>                  }
>>                  cil_list_append(new, CIL_SID, datum);
> 
> Here, free(new) will not free the items that were already appended.
> Would cil_list_destroy(&new, CIL_FALSE) work? (I have not tested it)
> 

Yes, "cil_list_destroy(&new, CIL_FALSE)" is what is needed here. Using CIL_FALSE 
means that the datums will not be destroyed which is what we would want.

>> @@ -1591,6 +1592,8 @@ int cil_resolve_catorder(struct cil_tree_node *current, void *extra_args)
>>          return SEPOL_OK;
>>
>>   exit:
>> +       if (new)
>> +               free(new);
>>          return rc;
>>   }
> 
> Same comment

Should use "cil_list_destroy(&new, CIL_FALSE)" here as well.
The "if (new)" is not needed since cil_list_destroy() will return if new is NULL.
> 
>> @@ -1624,6 +1627,7 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
>>          return SEPOL_OK;
>>
>>   exit:
>> +       free(new);
>>          return rc;
>>   }
> 
> Why is there a "if(new)" in the previous chunk and not here? As
> cil_list_init() fails hard when the memory allocation failed, new can
> never be NULL so the previous if(new) is not needed.
> 
> All the other changes in this patch looked good to me.
> 
Should use "cil_list_destroy(&new, CIL_FALSE)" here as well.

> Nicolas
> 
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Non-DoD Source] [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 13:22 [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan Petr Lautrbach
                   ` (3 preceding siblings ...)
  2019-01-31 21:33 ` Nicolas Iooss
@ 2019-02-01 20:07 ` " jwcart2
  4 siblings, 0 replies; 14+ messages in thread
From: jwcart2 @ 2019-02-01 20:07 UTC (permalink / raw)
  To: Petr Lautrbach, selinux

On 1/31/19 8:22 AM, Petr Lautrbach wrote:
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>   libsepol/cil/src/cil_binary.c      | 12 ++++++++++++
>   libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++++
>   libsepol/cil/src/cil_symtab.c      |  1 +
>   libsepol/src/expand.c              |  3 +++
>   libsepol/src/kernel_to_cil.c       |  2 ++
>   libsepol/src/kernel_to_conf.c      |  2 ++
>   6 files changed, 30 insertions(+)
> 

...

> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 6f1b235e..125a6809 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1451,6 +1451,7 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
>   				new_trans->name = strdup(cur_rule->name);
>   				if (!new_trans->name) {
>   					ERR(state->handle, "Out of memory!");
> +					free(new_trans);
>   					return -1;
>   				}
>   				new_trans->stype = i + 1;
> @@ -1460,6 +1461,8 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
>   				otype = calloc(1, sizeof(*otype));
>   				if (!otype) {
>   					ERR(state->handle, "Out of memory!");
> +					free(new_trans->name);
> +					free(new_trans);
>   					return -1;
>   				}
>   				otype->otype = mapped_otype;

I believe that you need the following in the "if (rc) {" block a few lines down.
free(new_trans->name);
free(new_tran);
free(otype);


Everything else that I didn't comment on in this email or my last looks good.

-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] libselinux: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-01-31 21:47   ` Nicolas Iooss
@ 2019-02-06 20:33     ` " Petr Lautrbach
  2019-02-06 22:40       ` Nicolas Iooss
  0 siblings, 1 reply; 14+ messages in thread
From: Petr Lautrbach @ 2019-02-06 20:33 UTC (permalink / raw)
  To: selinux; +Cc: Petr Lautrbach

Fixes:

libselinux/src/checkAccess.c:93: leaked_storage: Variable "user_context" going out of scope leaks the storage it points to.
libselinux/src/label_db.c:286: leaked_storage: Variable "filp" going out of scope leaks the storage it points to.
libselinux/src/label_db.c:291: leaked_storage: Variable "filp" going out of scope leaks the storage it points to.
libselinux/src/label_file.c:405: leaked_storage: Variable "str_buf" going out of scope leaks the storage it points to.
libselinux/src/load_policy.c:266: leaked_storage: Variable "names" going out of scope leaks the storage it points to.
libselinux/src/selinux_config.c:183: leaked_storage: Variable "end" going out of scope leaks the storage it points to.
libselinux/src/selinux_config.c:184: overwrite_var: Overwriting "end" in "end = type + strlen(type) - 1" leaks the storage that "end" points to.
libselinux/src/selinux_restorecon.c:376: leaked_storage: Variable "new_entry" going out of scope leaks the storage it points to.
libselinux/src/selinux_restorecon.c:855: leaked_storage: Variable "xattr_value" going out of scope leaks the storage it points to.

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 libselinux/src/checkAccess.c        |  4 +++-
 libselinux/src/label_db.c           |  3 +++
 libselinux/src/label_file.c         |  4 +++-
 libselinux/src/load_policy.c        |  4 +++-
 libselinux/src/selinux_config.c     | 17 +++++++++--------
 libselinux/src/selinux_restorecon.c | 12 ++++++++++--
 6 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
index 8de57477..16bfcfb6 100644
--- a/libselinux/src/checkAccess.c
+++ b/libselinux/src/checkAccess.c
@@ -89,8 +89,10 @@ int selinux_check_passwd_access(access_vector_t requested)
 		int retval;
 
 		passwd_class = string_to_security_class("passwd");
-		if (passwd_class == 0)
+		if (passwd_class == 0) {
+			freecon(user_context);
 			return 0;
+		}
 
 		retval = security_compute_av_raw(user_context,
 						     user_context,
diff --git a/libselinux/src/label_db.c b/libselinux/src/label_db.c
index c46d0a1d..fba96c92 100644
--- a/libselinux/src/label_db.c
+++ b/libselinux/src/label_db.c
@@ -283,10 +283,12 @@ db_init(const struct selinux_opt *opts, unsigned nopts,
 	}
 	if (fstat(fileno(filp), &sb) < 0) {
 		free(catalog);
+		fclose(filp);
 		return NULL;
 	}
 	if (!S_ISREG(sb.st_mode)) {
 		free(catalog);
+		fclose(filp);
 		errno = EINVAL;
 		return NULL;
 	}
@@ -340,6 +342,7 @@ out_error:
 		free(spec->lr.ctx_trans);
 	}
 	free(catalog);
+	fclose(filp);
 
 	return NULL;
 }
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index dbf51a93..b81fd552 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -317,8 +317,10 @@ end_arch_check:
 			goto out;
 		}
 		rc = next_entry(str_buf, mmap_area, entry_len);
-		if (rc < 0)
+		if (rc < 0) {
+			free(str_buf);
 			goto out;
+		}
 
 		if (str_buf[entry_len - 1] != '\0') {
 			free(str_buf);
diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index e9f1264a..20052beb 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -262,8 +262,10 @@ checkbool:
 			rc = security_get_boolean_names(&names, &len);
 			if (!rc) {
 				values = malloc(sizeof(int) * len);
-				if (!values)
+				if (!values) {
+					free(names);
 					goto unmap;
+				}
 				for (i = 0; i < len; i++)
 					values[i] =
 						security_get_boolean_active(names[i]);
diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
index 292728f3..b06cb63b 100644
--- a/libselinux/src/selinux_config.c
+++ b/libselinux/src/selinux_config.c
@@ -177,8 +177,7 @@ static void init_selinux_config(void)
 
 			if (!strncasecmp(buf_p, SELINUXTYPETAG,
 					 sizeof(SELINUXTYPETAG) - 1)) {
-				selinux_policytype = type =
-				    strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
+				type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
 				if (!type)
 					return;
 				end = type + strlen(type) - 1;
@@ -187,6 +186,11 @@ static void init_selinux_config(void)
 					*end = 0;
 					end--;
 				}
+				if (setpolicytype(type) != 0) {
+					free(type);
+					return;
+				}
+				free(type);
 				continue;
 			} else if (!strncmp(buf_p, SETLOCALDEFS,
 					    sizeof(SETLOCALDEFS) - 1)) {
@@ -212,13 +216,10 @@ static void init_selinux_config(void)
 		fclose(fp);
 	}
 
-	if (!type) {
-		selinux_policytype = type = strdup(SELINUXDEFAULT);
-		if (!type)
-			return;
-	}
+	if (!selinux_policytype && setpolicytype(SELINUXDEFAULT) != 0)
+		return;
 
-	if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, type) == -1)
+	if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, selinux_policytype) == -1)
 		return;
 
 	for (i = 0; i < NEL; i++)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 0f0fd6ca..924d9538 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -351,12 +351,19 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch,
 	new_entry->next = NULL;
 
 	new_entry->directory = strdup(directory);
-	if (!new_entry->directory)
+	if (!new_entry->directory) {
+		free(new_entry);
+		free(sha1_buf);
 		goto oom;
+	}
 
 	new_entry->digest = strdup(sha1_buf);
-	if (!new_entry->digest)
+	if (!new_entry->digest) {
+		free(new_entry->directory);
+		free(new_entry);
+		free(sha1_buf);
 		goto oom;
+	}
 
 	new_entry->result = digest_result;
 
@@ -850,6 +857,7 @@ int selinux_restorecon(const char *pathname_orig,
 
 	if (lstat(pathname, &sb) < 0) {
 		if (flags.ignore_noent && errno == ENOENT) {
+			free(xattr_value);
 			free(pathdnamer);
 			free(pathname);
 			return 0;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] libselinux: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-02-06 20:33     ` [PATCH v2] " Petr Lautrbach
@ 2019-02-06 22:40       ` Nicolas Iooss
  2019-02-10 18:32         ` Nicolas Iooss
  0 siblings, 1 reply; 14+ messages in thread
From: Nicolas Iooss @ 2019-02-06 22:40 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Wed, Feb 6, 2019 at 9:33 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Fixes:
>
> libselinux/src/checkAccess.c:93: leaked_storage: Variable "user_context" going out of scope leaks the storage it points to.
> libselinux/src/label_db.c:286: leaked_storage: Variable "filp" going out of scope leaks the storage it points to.
> libselinux/src/label_db.c:291: leaked_storage: Variable "filp" going out of scope leaks the storage it points to.
> libselinux/src/label_file.c:405: leaked_storage: Variable "str_buf" going out of scope leaks the storage it points to.
> libselinux/src/load_policy.c:266: leaked_storage: Variable "names" going out of scope leaks the storage it points to.
> libselinux/src/selinux_config.c:183: leaked_storage: Variable "end" going out of scope leaks the storage it points to.
> libselinux/src/selinux_config.c:184: overwrite_var: Overwriting "end" in "end = type + strlen(type) - 1" leaks the storage that "end" points to.
> libselinux/src/selinux_restorecon.c:376: leaked_storage: Variable "new_entry" going out of scope leaks the storage it points to.
> libselinux/src/selinux_restorecon.c:855: leaked_storage: Variable "xattr_value" going out of scope leaks the storage it points to.
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

> ---
>  libselinux/src/checkAccess.c        |  4 +++-
>  libselinux/src/label_db.c           |  3 +++
>  libselinux/src/label_file.c         |  4 +++-
>  libselinux/src/load_policy.c        |  4 +++-
>  libselinux/src/selinux_config.c     | 17 +++++++++--------
>  libselinux/src/selinux_restorecon.c | 12 ++++++++++--
>  6 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
> index 8de57477..16bfcfb6 100644
> --- a/libselinux/src/checkAccess.c
> +++ b/libselinux/src/checkAccess.c
> @@ -89,8 +89,10 @@ int selinux_check_passwd_access(access_vector_t requested)
>                 int retval;
>
>                 passwd_class = string_to_security_class("passwd");
> -               if (passwd_class == 0)
> +               if (passwd_class == 0) {
> +                       freecon(user_context);
>                         return 0;
> +               }
>
>                 retval = security_compute_av_raw(user_context,
>                                                      user_context,
> diff --git a/libselinux/src/label_db.c b/libselinux/src/label_db.c
> index c46d0a1d..fba96c92 100644
> --- a/libselinux/src/label_db.c
> +++ b/libselinux/src/label_db.c
> @@ -283,10 +283,12 @@ db_init(const struct selinux_opt *opts, unsigned nopts,
>         }
>         if (fstat(fileno(filp), &sb) < 0) {
>                 free(catalog);
> +               fclose(filp);
>                 return NULL;
>         }
>         if (!S_ISREG(sb.st_mode)) {
>                 free(catalog);
> +               fclose(filp);
>                 errno = EINVAL;
>                 return NULL;
>         }
> @@ -340,6 +342,7 @@ out_error:
>                 free(spec->lr.ctx_trans);
>         }
>         free(catalog);
> +       fclose(filp);
>
>         return NULL;
>  }
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index dbf51a93..b81fd552 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -317,8 +317,10 @@ end_arch_check:
>                         goto out;
>                 }
>                 rc = next_entry(str_buf, mmap_area, entry_len);
> -               if (rc < 0)
> +               if (rc < 0) {
> +                       free(str_buf);
>                         goto out;
> +               }
>
>                 if (str_buf[entry_len - 1] != '\0') {
>                         free(str_buf);
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index e9f1264a..20052beb 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -262,8 +262,10 @@ checkbool:
>                         rc = security_get_boolean_names(&names, &len);
>                         if (!rc) {
>                                 values = malloc(sizeof(int) * len);
> -                               if (!values)
> +                               if (!values) {
> +                                       free(names);
>                                         goto unmap;
> +                               }
>                                 for (i = 0; i < len; i++)
>                                         values[i] =
>                                                 security_get_boolean_active(names[i]);
> diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
> index 292728f3..b06cb63b 100644
> --- a/libselinux/src/selinux_config.c
> +++ b/libselinux/src/selinux_config.c
> @@ -177,8 +177,7 @@ static void init_selinux_config(void)
>
>                         if (!strncasecmp(buf_p, SELINUXTYPETAG,
>                                          sizeof(SELINUXTYPETAG) - 1)) {
> -                               selinux_policytype = type =
> -                                   strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
> +                               type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
>                                 if (!type)
>                                         return;
>                                 end = type + strlen(type) - 1;
> @@ -187,6 +186,11 @@ static void init_selinux_config(void)
>                                         *end = 0;
>                                         end--;
>                                 }
> +                               if (setpolicytype(type) != 0) {
> +                                       free(type);
> +                                       return;
> +                               }
> +                               free(type);
>                                 continue;
>                         } else if (!strncmp(buf_p, SETLOCALDEFS,
>                                             sizeof(SETLOCALDEFS) - 1)) {
> @@ -212,13 +216,10 @@ static void init_selinux_config(void)
>                 fclose(fp);
>         }
>
> -       if (!type) {
> -               selinux_policytype = type = strdup(SELINUXDEFAULT);
> -               if (!type)
> -                       return;
> -       }
> +       if (!selinux_policytype && setpolicytype(SELINUXDEFAULT) != 0)
> +               return;
>
> -       if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, type) == -1)
> +       if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, selinux_policytype) == -1)
>                 return;
>
>         for (i = 0; i < NEL; i++)
> diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> index 0f0fd6ca..924d9538 100644
> --- a/libselinux/src/selinux_restorecon.c
> +++ b/libselinux/src/selinux_restorecon.c
> @@ -351,12 +351,19 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch,
>         new_entry->next = NULL;
>
>         new_entry->directory = strdup(directory);
> -       if (!new_entry->directory)
> +       if (!new_entry->directory) {
> +               free(new_entry);
> +               free(sha1_buf);
>                 goto oom;
> +       }
>
>         new_entry->digest = strdup(sha1_buf);
> -       if (!new_entry->digest)
> +       if (!new_entry->digest) {
> +               free(new_entry->directory);
> +               free(new_entry);
> +               free(sha1_buf);
>                 goto oom;
> +       }
>
>         new_entry->result = digest_result;
>
> @@ -850,6 +857,7 @@ int selinux_restorecon(const char *pathname_orig,
>
>         if (lstat(pathname, &sb) < 0) {
>                 if (flags.ignore_noent && errno == ENOENT) {
> +                       free(xattr_value);
>                         free(pathdnamer);
>                         free(pathname);
>                         return 0;
> --
> 2.20.1
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] libselinux: Fix RESOURCE_LEAK defects reported by coverity scan
  2019-02-06 22:40       ` Nicolas Iooss
@ 2019-02-10 18:32         ` Nicolas Iooss
  0 siblings, 0 replies; 14+ messages in thread
From: Nicolas Iooss @ 2019-02-10 18:32 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Wed, Feb 6, 2019 at 11:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, Feb 6, 2019 at 9:33 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> >
> > Fixes:
> >
> > libselinux/src/checkAccess.c:93: leaked_storage: Variable "user_context" going out of scope leaks the storage it points to.
> > libselinux/src/label_db.c:286: leaked_storage: Variable "filp" going out of scope leaks the storage it points to.
> > libselinux/src/label_db.c:291: leaked_storage: Variable "filp" going out of scope leaks the storage it points to.
> > libselinux/src/label_file.c:405: leaked_storage: Variable "str_buf" going out of scope leaks the storage it points to.
> > libselinux/src/load_policy.c:266: leaked_storage: Variable "names" going out of scope leaks the storage it points to.
> > libselinux/src/selinux_config.c:183: leaked_storage: Variable "end" going out of scope leaks the storage it points to.
> > libselinux/src/selinux_config.c:184: overwrite_var: Overwriting "end" in "end = type + strlen(type) - 1" leaks the storage that "end" points to.
> > libselinux/src/selinux_restorecon.c:376: leaked_storage: Variable "new_entry" going out of scope leaks the storage it points to.
> > libselinux/src/selinux_restorecon.c:855: leaked_storage: Variable "xattr_value" going out of scope leaks the storage it points to.
> >
> > Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Merged.

Nicolas

> > ---
> >  libselinux/src/checkAccess.c        |  4 +++-
> >  libselinux/src/label_db.c           |  3 +++
> >  libselinux/src/label_file.c         |  4 +++-
> >  libselinux/src/load_policy.c        |  4 +++-
> >  libselinux/src/selinux_config.c     | 17 +++++++++--------
> >  libselinux/src/selinux_restorecon.c | 12 ++++++++++--
> >  6 files changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
> > index 8de57477..16bfcfb6 100644
> > --- a/libselinux/src/checkAccess.c
> > +++ b/libselinux/src/checkAccess.c
> > @@ -89,8 +89,10 @@ int selinux_check_passwd_access(access_vector_t requested)
> >                 int retval;
> >
> >                 passwd_class = string_to_security_class("passwd");
> > -               if (passwd_class == 0)
> > +               if (passwd_class == 0) {
> > +                       freecon(user_context);
> >                         return 0;
> > +               }
> >
> >                 retval = security_compute_av_raw(user_context,
> >                                                      user_context,
> > diff --git a/libselinux/src/label_db.c b/libselinux/src/label_db.c
> > index c46d0a1d..fba96c92 100644
> > --- a/libselinux/src/label_db.c
> > +++ b/libselinux/src/label_db.c
> > @@ -283,10 +283,12 @@ db_init(const struct selinux_opt *opts, unsigned nopts,
> >         }
> >         if (fstat(fileno(filp), &sb) < 0) {
> >                 free(catalog);
> > +               fclose(filp);
> >                 return NULL;
> >         }
> >         if (!S_ISREG(sb.st_mode)) {
> >                 free(catalog);
> > +               fclose(filp);
> >                 errno = EINVAL;
> >                 return NULL;
> >         }
> > @@ -340,6 +342,7 @@ out_error:
> >                 free(spec->lr.ctx_trans);
> >         }
> >         free(catalog);
> > +       fclose(filp);
> >
> >         return NULL;
> >  }
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index dbf51a93..b81fd552 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -317,8 +317,10 @@ end_arch_check:
> >                         goto out;
> >                 }
> >                 rc = next_entry(str_buf, mmap_area, entry_len);
> > -               if (rc < 0)
> > +               if (rc < 0) {
> > +                       free(str_buf);
> >                         goto out;
> > +               }
> >
> >                 if (str_buf[entry_len - 1] != '\0') {
> >                         free(str_buf);
> > diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> > index e9f1264a..20052beb 100644
> > --- a/libselinux/src/load_policy.c
> > +++ b/libselinux/src/load_policy.c
> > @@ -262,8 +262,10 @@ checkbool:
> >                         rc = security_get_boolean_names(&names, &len);
> >                         if (!rc) {
> >                                 values = malloc(sizeof(int) * len);
> > -                               if (!values)
> > +                               if (!values) {
> > +                                       free(names);
> >                                         goto unmap;
> > +                               }
> >                                 for (i = 0; i < len; i++)
> >                                         values[i] =
> >                                                 security_get_boolean_active(names[i]);
> > diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
> > index 292728f3..b06cb63b 100644
> > --- a/libselinux/src/selinux_config.c
> > +++ b/libselinux/src/selinux_config.c
> > @@ -177,8 +177,7 @@ static void init_selinux_config(void)
> >
> >                         if (!strncasecmp(buf_p, SELINUXTYPETAG,
> >                                          sizeof(SELINUXTYPETAG) - 1)) {
> > -                               selinux_policytype = type =
> > -                                   strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
> > +                               type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1);
> >                                 if (!type)
> >                                         return;
> >                                 end = type + strlen(type) - 1;
> > @@ -187,6 +186,11 @@ static void init_selinux_config(void)
> >                                         *end = 0;
> >                                         end--;
> >                                 }
> > +                               if (setpolicytype(type) != 0) {
> > +                                       free(type);
> > +                                       return;
> > +                               }
> > +                               free(type);
> >                                 continue;
> >                         } else if (!strncmp(buf_p, SETLOCALDEFS,
> >                                             sizeof(SETLOCALDEFS) - 1)) {
> > @@ -212,13 +216,10 @@ static void init_selinux_config(void)
> >                 fclose(fp);
> >         }
> >
> > -       if (!type) {
> > -               selinux_policytype = type = strdup(SELINUXDEFAULT);
> > -               if (!type)
> > -                       return;
> > -       }
> > +       if (!selinux_policytype && setpolicytype(SELINUXDEFAULT) != 0)
> > +               return;
> >
> > -       if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, type) == -1)
> > +       if (asprintf(&selinux_policyroot, "%s%s", SELINUXDIR, selinux_policytype) == -1)
> >                 return;
> >
> >         for (i = 0; i < NEL; i++)
> > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> > index 0f0fd6ca..924d9538 100644
> > --- a/libselinux/src/selinux_restorecon.c
> > +++ b/libselinux/src/selinux_restorecon.c
> > @@ -351,12 +351,19 @@ static int add_xattr_entry(const char *directory, bool delete_nonmatch,
> >         new_entry->next = NULL;
> >
> >         new_entry->directory = strdup(directory);
> > -       if (!new_entry->directory)
> > +       if (!new_entry->directory) {
> > +               free(new_entry);
> > +               free(sha1_buf);
> >                 goto oom;
> > +       }
> >
> >         new_entry->digest = strdup(sha1_buf);
> > -       if (!new_entry->digest)
> > +       if (!new_entry->digest) {
> > +               free(new_entry->directory);
> > +               free(new_entry);
> > +               free(sha1_buf);
> >                 goto oom;
> > +       }
> >
> >         new_entry->result = digest_result;
> >
> > @@ -850,6 +857,7 @@ int selinux_restorecon(const char *pathname_orig,
> >
> >         if (lstat(pathname, &sb) < 0) {
> >                 if (flags.ignore_noent && errno == ENOENT) {
> > +                       free(xattr_value);
> >                         free(pathdnamer);
> >                         free(pathname);
> >                         return 0;
> > --
> > 2.20.1
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-31 13:22 [PATCH 1/3] libsepol: Fix RESOURCE_LEAK defects reported by coverity scan Petr Lautrbach
2019-01-31 13:22 ` [PATCH 2/3] libselinux: " Petr Lautrbach
2019-01-31 21:47   ` Nicolas Iooss
2019-02-06 20:33     ` [PATCH v2] " Petr Lautrbach
2019-02-06 22:40       ` Nicolas Iooss
2019-02-10 18:32         ` Nicolas Iooss
2019-01-31 13:22 ` [PATCH 3/3] libsemanage: Fix USE_AFTER_FREE " Petr Lautrbach
2019-01-31 21:57   ` Nicolas Iooss
2019-02-01 12:45     ` Petr Lautrbach
2019-01-31 21:17 ` [PATCH 1/3] libsepol: Fix RESOURCE_LEAK " Nicolas Iooss
2019-02-01 18:19   ` [Non-DoD Source] " jwcart2
2019-01-31 21:33 ` Nicolas Iooss
2019-02-01 19:32   ` [Non-DoD Source] " jwcart2
2019-02-01 20:07 ` [Non-DoD Source] " jwcart2

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