[PATCH 1/3] libsemanage: Fix USE_AFTER_FREE (CWE-672) in semanage_direct_get_module_info()

[PATCH 1/3] libsemanage: Fix USE_AFTER_FREE (CWE-672) in semanage_direct_get_module_info()

[Petr Lautrbach]

>From fclose(3):
Upon successful completion, 0 is returned.  Otherwise, EOF is returned
and errno is set to indicate the error. In either case, any further
access (including another call to fclose()) to the stream results in
undefined behavior.

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

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index d83941b05ca1..d5716ce579e9 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -2293,6 +2293,7 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
 	tmp = NULL;
 
 	if (fclose(fp) != 0) {
+		fp = NULL;
 		ERR(sh,
 		    "Unable to close %s module lang ext file.",
 		    (*modinfo)->name);
-- 
2.35.1

[PATCH 2/3] semodule_package: Close leaking fd

[Petr Lautrbach]

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 semodule-utils/semodule_package/semodule_package.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c
index 3515234e36de..bc8584b51079 100644
--- a/semodule-utils/semodule_package/semodule_package.c
+++ b/semodule-utils/semodule_package/semodule_package.c
@@ -73,6 +73,7 @@ static int file_to_data(const char *path, char **data, size_t * len)
 		goto err;
 	}
 	if (!sb.st_size) {
+		close(fd);
 		*len = 0;
 		return 0;
 	}
-- 
2.35.1

[PATCH 3/3] mcstrans: Fir RESOURCE_LEAK and USE_AFTER_FREE coverity scan defects

[Petr Lautrbach]

---
 mcstrans/src/mcstrans.c  | 25 ++++++++++++++++++++++++-
 mcstrans/src/mcstransd.c |  4 +++-
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/mcstrans/src/mcstrans.c b/mcstrans/src/mcstrans.c
index d42760fdbfc2..af3f507ef718 100644
--- a/mcstrans/src/mcstrans.c
+++ b/mcstrans/src/mcstrans.c
@@ -632,16 +632,23 @@ add_cache(domain_t *domain, char *raw, char *trans) {
 
 	map->raw = strdup(raw);
 	if (!map->raw) {
+		free(map);
 		goto err;
 	}
 	map->trans = strdup(trans);
 	if (!map->trans) {
+		free(map->raw);
+		free(map);
 		goto err;
 	}
 
 	log_debug(" add_cache (%s,%s)\n", raw, trans);
-	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0)
+	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0) {
+		free(map->trans);
+		free(map->raw);
+		free(map);
 		goto err;
+	}
 
 	if (add_to_hashtable(domain->trans_to_raw, map->trans, map) < 0)
 		goto err;
@@ -1568,6 +1575,7 @@ trans_context(const char *incon, char **rcon) {
 			trans = compute_trans_from_raw(range, domain);
 			if (trans)
 				if (add_cache(domain, range, trans) < 0) {
+					free(trans);
 					free(range);
 					return -1;
 				}
@@ -1579,6 +1587,7 @@ trans_context(const char *incon, char **rcon) {
 				ltrans = compute_trans_from_raw(lrange, domain);
 				if (ltrans) {
 					if (add_cache(domain, lrange, ltrans) < 0) {
+						free(ltrans);
 						free(range);
 						return -1;
 					}
@@ -1597,6 +1606,7 @@ trans_context(const char *incon, char **rcon) {
 				utrans = compute_trans_from_raw(urange, domain);
 				if (utrans) {
 					if (add_cache(domain, urange, utrans) < 0) {
+						free(utrans);
 						free(ltrans);
 						free(range);
 						return -1;
@@ -1636,6 +1646,10 @@ trans_context(const char *incon, char **rcon) {
 		}
 		if (dashp)
 			*dashp = '-';
+		if (trans) {
+			free(trans);
+			trans = NULL;
+		}
 	}
 
 	if (trans) {
@@ -1696,7 +1710,9 @@ untrans_context(const char *incon, char **rcon) {
 					canonical = compute_trans_from_raw(raw, domain);
 					if (canonical && strcmp(canonical, range))
 						if (add_cache(domain, raw, canonical) < 0) {
+							free(canonical);
 							free(range);
+							free(raw);
 							return -1;
 						}
 				}
@@ -1704,6 +1720,7 @@ untrans_context(const char *incon, char **rcon) {
 					free(canonical);
 				if (add_cache(domain, raw, range) < 0) {
 					free(range);
+					free(raw);
 					return -1;
 				}
 			} else {
@@ -1721,6 +1738,7 @@ untrans_context(const char *incon, char **rcon) {
 						canonical = compute_trans_from_raw(lraw, domain);
 						if (canonical)
 							if (add_cache(domain, lraw, canonical) < 0) {
+								free(canonical);
 								free(lraw);
 								free(range);
 								return -1;
@@ -1752,6 +1770,7 @@ untrans_context(const char *incon, char **rcon) {
 						canonical = compute_trans_from_raw(uraw, domain);
 						if (canonical)
 							if (add_cache(domain, uraw, canonical) < 0) {
+								free(canonical);
 								free(uraw);
 								free(lraw);
 								free(range);
@@ -1802,6 +1821,10 @@ untrans_context(const char *incon, char **rcon) {
 		}
 		if (dashp)
 			*dashp = '-';
+		if (raw) {
+			free(raw);
+			raw = NULL;
+		}
 	}
 
 	if (raw) {
diff --git a/mcstrans/src/mcstransd.c b/mcstrans/src/mcstransd.c
index 536c0f32f23a..42262e580386 100644
--- a/mcstrans/src/mcstransd.c
+++ b/mcstrans/src/mcstransd.c
@@ -328,6 +328,7 @@ process_events(struct pollfd **ufds, int *nfds)
 					/* Setup pollfd for deletion later. */
 					(*ufds)[ii].fd = -1;
 					close(connfd);
+					connfd = -1;
 					/* So we don't get bothered later */
 					revents = revents & ~(POLLHUP);
 				}
@@ -341,10 +342,11 @@ process_events(struct pollfd **ufds, int *nfds)
 			/* Set the pollfd up for deletion later. */
 			(*ufds)[ii].fd = -1;
 			close(connfd);
+			connfd = -1;
 
 			revents = revents & ~(POLLHUP);
 		}
-		if (revents) {
+		if (revents && connfd != -1) {
 			syslog(LOG_ERR, "Unknown/error events (%x) encountered"
 					" for fd (%d)\n", revents, connfd);
 
-- 
2.35.1

Re: [PATCH 1/3] libsemanage: Fix USE_AFTER_FREE (CWE-672) in semanage_direct_get_module_info()

[James Carter]

On Fri, Apr 1, 2022 at 11:54 AM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> >From fclose(3):
> Upon successful completion, 0 is returned.  Otherwise, EOF is returned
> and errno is set to indicate the error. In either case, any further
> access (including another call to fclose()) to the stream results in
> undefined behavior.
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>

For all three patches:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsemanage/src/direct_api.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index d83941b05ca1..d5716ce579e9 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -2293,6 +2293,7 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>         tmp = NULL;
>
>         if (fclose(fp) != 0) {
> +               fp = NULL;
>                 ERR(sh,
>                     "Unable to close %s module lang ext file.",
>                     (*modinfo)->name);
> --
> 2.35.1
>

Re: [PATCH 1/3] libsemanage: Fix USE_AFTER_FREE (CWE-672) in semanage_direct_get_module_info()

[Petr Lautrbach]

James Carter <jwcart2@gmail.com> writes:

> On Fri, Apr 1, 2022 at 11:54 AM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> >From fclose(3):
>> Upon successful completion, 0 is returned.  Otherwise, EOF is returned
>> and errno is set to indicate the error. In either case, any further
>> access (including another call to fclose()) to the stream results in
>> undefined behavior.
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>
> For all three patches:
> Acked-by: James Carter <jwcart2@gmail.com>

Merged, thanks!


>
>> ---
>>  libsemanage/src/direct_api.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>> index d83941b05ca1..d5716ce579e9 100644
>> --- a/libsemanage/src/direct_api.c
>> +++ b/libsemanage/src/direct_api.c
>> @@ -2293,6 +2293,7 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>         tmp = NULL;
>>
>>         if (fclose(fp) != 0) {
>> +               fp = NULL;
>>                 ERR(sh,
>>                     "Unable to close %s module lang ext file.",
>>                     (*modinfo)->name);
>> --
>> 2.35.1
>>