All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read
@ 2022-05-10 18:20 Christian Göttsche
  2022-05-10 18:20 ` [RFC PATCH 2/4] libselinux: add header guard for internal header Christian Göttsche
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Christian Göttsche @ 2022-05-10 18:20 UTC (permalink / raw)
  To: selinux

In case the function __policy_init() gets called with a NULL pointer,
the stack variable path remains uninitialized (except at its last
index).  If parsing the binary policy fails in sepol_policydb_read() the
error branch would access those uninitialized memory.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/audit2why.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
index ca38e13c..44a9a341 100644
--- a/libselinux/src/audit2why.c
+++ b/libselinux/src/audit2why.c
@@ -192,25 +192,16 @@ static PyObject *finish(PyObject *self __attribute__((unused)), PyObject *args)
 static int __policy_init(const char *init_path)
 {
 	FILE *fp;
-	char path[PATH_MAX];
+	const char *curpolicy;
 	char errormsg[PATH_MAX+1024+20];
 	struct sepol_policy_file *pf = NULL;
 	int rc;
 	unsigned int cnt;
 
-	path[PATH_MAX-1] = '\0';
 	if (init_path) {
-		strncpy(path, init_path, PATH_MAX-1);
-		fp = fopen(path, "re");
-		if (!fp) {
-			snprintf(errormsg, sizeof(errormsg), 
-				 "unable to open %s:  %m\n",
-				 path);
-			PyErr_SetString( PyExc_ValueError, errormsg);
-			return 1;
-		}
+		curpolicy = init_path;
 	} else {
-		const char *curpolicy = selinux_current_policy_path();
+		curpolicy = selinux_current_policy_path();
 		if (!curpolicy) {
 			/* SELinux disabled, must use -p option. */
 			snprintf(errormsg, sizeof(errormsg),
@@ -218,14 +209,15 @@ static int __policy_init(const char *init_path)
 			PyErr_SetString( PyExc_ValueError, errormsg);
 			return 1;
 		}
-		fp = fopen(curpolicy, "re");
-		if (!fp) {
-			snprintf(errormsg, sizeof(errormsg), 
-				 "unable to open %s:  %m\n",
-				 curpolicy);
-			PyErr_SetString( PyExc_ValueError, errormsg);
-			return 1;
-		}
+	}
+
+	fp = fopen(curpolicy, "re");
+	if (!fp) {
+		snprintf(errormsg, sizeof(errormsg),
+			 "unable to open %s:  %m\n",
+			 curpolicy);
+		PyErr_SetString( PyExc_ValueError, errormsg);
+		return 1;
 	}
 
 	avc = calloc(sizeof(struct avc_t), 1);
@@ -249,7 +241,7 @@ static int __policy_init(const char *init_path)
 	sepol_policy_file_set_fp(pf, fp);	
 	if (sepol_policydb_read(avc->policydb, pf)) {
 		snprintf(errormsg, sizeof(errormsg), 
-			 "invalid binary policy %s\n", path);
+			 "invalid binary policy %s\n", curpolicy);
 		PyErr_SetString( PyExc_ValueError, errormsg);
 		fclose(fp);
 		return 1;
-- 
2.36.1


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

* [RFC PATCH 2/4] libselinux: add header guard for internal header
  2022-05-10 18:20 [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read Christian Göttsche
@ 2022-05-10 18:20 ` Christian Göttsche
  2022-05-10 18:20 ` [RFC PATCH 3/4] libselinux: introduce strlcpy Christian Göttsche
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Christian Göttsche @ 2022-05-10 18:20 UTC (permalink / raw)
  To: selinux

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/selinux_internal.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
index 297dcf26..9f4c9073 100644
--- a/libselinux/src/selinux_internal.h
+++ b/libselinux/src/selinux_internal.h
@@ -1,3 +1,6 @@
+#ifndef SELINUX_INTERNAL_H_
+#define SELINUX_INTERNAL_H_
+
 #include <selinux/selinux.h>
 #include <pthread.h>
 
@@ -90,3 +93,5 @@ extern int selinux_page_size ;
 #define SELINUXCONFIG SELINUXDIR "config"
 
 extern int has_selinux_config ;
+
+#endif /* SELINUX_INTERNAL_H_ */
-- 
2.36.1


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

* [RFC PATCH 3/4] libselinux: introduce strlcpy
  2022-05-10 18:20 [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read Christian Göttsche
  2022-05-10 18:20 ` [RFC PATCH 2/4] libselinux: add header guard for internal header Christian Göttsche
@ 2022-05-10 18:20 ` Christian Göttsche
  2022-05-10 18:20 ` [RFC PATCH 4/4] libselinux: check for truncations Christian Göttsche
  2022-05-18 15:22 ` [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read James Carter
  3 siblings, 0 replies; 10+ messages in thread
From: Christian Göttsche @ 2022-05-10 18:20 UTC (permalink / raw)
  To: selinux

To copy string safely, by always NULL-terminating them, and provide an
easy way to check for truncation introduce the nonstandard function
strlcpy(3).  Use the system implementation if available.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/Makefile           |  6 ++++++
 libselinux/src/selinux_internal.c | 18 ++++++++++++++++++
 libselinux/src/selinux_internal.h |  4 ++++
 3 files changed, 28 insertions(+)
 create mode 100644 libselinux/src/selinux_internal.c

diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
index 04bf4f24..88aa32f8 100644
--- a/libselinux/src/Makefile
+++ b/libselinux/src/Makefile
@@ -103,6 +103,12 @@ FTS_LDLIBS ?=
 
 override CFLAGS += -I../include -D_GNU_SOURCE $(DISABLE_FLAGS) $(PCRE_CFLAGS)
 
+# check for strlcpy(3) availability
+H := \#
+ifeq (yes,$(shell printf '${H}include <string.h>\nint main(void){char*d,*s;strlcpy(d, s, 0);return 0;}' | $(CC) -x c -o /dev/null - >/dev/null 2>&1 && echo yes))
+override CFLAGS += -DHAVE_STRLCPY
+endif
+
 SWIG_CFLAGS += -Wno-error -Wno-unused-variable -Wno-unused-but-set-variable -Wno-unused-parameter \
 		-Wno-shadow -Wno-uninitialized -Wno-missing-prototypes -Wno-missing-declarations \
 		-Wno-deprecated-declarations
diff --git a/libselinux/src/selinux_internal.c b/libselinux/src/selinux_internal.c
new file mode 100644
index 00000000..c2be7c0a
--- /dev/null
+++ b/libselinux/src/selinux_internal.c
@@ -0,0 +1,18 @@
+#include "selinux_internal.h"
+
+#include <string.h>
+
+
+#ifndef HAVE_STRLCPY
+size_t strlcpy(char *dest, const char *src, size_t size)
+{
+	size_t ret = strlen(src);
+
+	if (size) {
+		size_t len = (ret >= size) ? size - 1 : ret;
+		memcpy(dest, src, len);
+		dest[len] = '\0';
+	}
+	return ret;
+}
+#endif /* HAVE_STRLCPY */
diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
index 9f4c9073..06f2c038 100644
--- a/libselinux/src/selinux_internal.h
+++ b/libselinux/src/selinux_internal.h
@@ -94,4 +94,8 @@ extern int selinux_page_size ;
 
 extern int has_selinux_config ;
 
+#ifndef HAVE_STRLCPY
+size_t strlcpy(char *dest, const char *src, size_t size);
+#endif
+
 #endif /* SELINUX_INTERNAL_H_ */
-- 
2.36.1


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

* [RFC PATCH 4/4] libselinux: check for truncations
  2022-05-10 18:20 [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read Christian Göttsche
  2022-05-10 18:20 ` [RFC PATCH 2/4] libselinux: add header guard for internal header Christian Göttsche
  2022-05-10 18:20 ` [RFC PATCH 3/4] libselinux: introduce strlcpy Christian Göttsche
@ 2022-05-10 18:20 ` Christian Göttsche
  2022-05-12 15:34   ` James Carter
                     ` (2 more replies)
  2022-05-18 15:22 ` [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read James Carter
  3 siblings, 3 replies; 10+ messages in thread
From: Christian Göttsche @ 2022-05-10 18:20 UTC (permalink / raw)
  To: selinux

Check for truncations when building or copying strings involving user
input.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/canonicalize_context.c |  6 +++++-
 libselinux/src/compute_av.c           |  7 ++++++-
 libselinux/src/compute_create.c       |  6 ++++++
 libselinux/src/compute_member.c       |  7 ++++++-
 libselinux/src/compute_relabel.c      |  7 ++++++-
 libselinux/src/compute_user.c         |  7 ++++++-
 libselinux/src/selinux_restorecon.c   | 11 ++++++++++-
 libselinux/src/setrans_client.c       |  8 +++++++-
 8 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/libselinux/src/canonicalize_context.c b/libselinux/src/canonicalize_context.c
index faab7305..8a22a4cd 100644
--- a/libselinux/src/canonicalize_context.c
+++ b/libselinux/src/canonicalize_context.c
@@ -33,7 +33,11 @@ int security_canonicalize_context_raw(const char * con,
 		ret = -1;
 		goto out;
 	}
-	strncpy(buf, con, size);
+	if (strlcpy(buf, con, size) >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out;
+	}
 
 	ret = write(fd, buf, strlen(buf) + 1);
 	if (ret < 0)
diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
index 9d17339d..e513be6a 100644
--- a/libselinux/src/compute_av.c
+++ b/libselinux/src/compute_av.c
@@ -40,8 +40,13 @@ int security_compute_av_flags_raw(const char * scon,
 	}
 
 	kclass = unmap_class(tclass);
-	snprintf(buf, len, "%s %s %hu %x", scon, tcon,
+
+	ret = snprintf(buf, len, "%s %s %hu %x", scon, tcon,
 		 kclass, unmap_perm(tclass, requested));
+	if (ret < 0 || ret >= len) {
+		errno = EOVERFLOW;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_create.c b/libselinux/src/compute_create.c
index 1d75714d..4cba2d2f 100644
--- a/libselinux/src/compute_create.c
+++ b/libselinux/src/compute_create.c
@@ -75,8 +75,14 @@ int security_compute_create_name_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
+
 	len = snprintf(buf, size, "%s %s %hu",
 		       scon, tcon, unmap_class(tclass));
+	if (len < 0 || len >= size) {
+		errno = EOVERFLOW;
+		goto out2;
+	}
+
 	if (objname &&
 	    object_name_encode(objname, buf + len, size - len) < 0) {
 		errno = ENAMETOOLONG;
diff --git a/libselinux/src/compute_member.c b/libselinux/src/compute_member.c
index 16234b79..82d76080 100644
--- a/libselinux/src/compute_member.c
+++ b/libselinux/src/compute_member.c
@@ -36,7 +36,12 @@ int security_compute_member_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || ret >= size) {
+		errno = EOVERFLOW;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_relabel.c b/libselinux/src/compute_relabel.c
index dd20d652..96259bac 100644
--- a/libselinux/src/compute_relabel.c
+++ b/libselinux/src/compute_relabel.c
@@ -36,7 +36,12 @@ int security_compute_relabel_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || ret >= size) {
+		errno = EOVERFLOW;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_user.c b/libselinux/src/compute_user.c
index ae5e7b4a..23a551e4 100644
--- a/libselinux/src/compute_user.c
+++ b/libselinux/src/compute_user.c
@@ -38,7 +38,12 @@ int security_compute_user_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s", scon, user);
+
+	ret = snprintf(buf, size, "%s %s", scon, user);
+	if (ret < 0 || ret >= size) {
+		errno = EOVERFLOW;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index e6192912..7436dab5 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -940,7 +940,16 @@ loop_body:
 			}
 			/* fall through */
 		default:
-			strcpy(ent_path, ftsent->fts_path);
+			if (strlcpy(ent_path, ftsent->fts_path, sizeof(ent_path)) >= sizeof(ent_path)) {
+				selinux_log(SELINUX_ERROR,
+					    "Path name too long on %s.\n",
+					    ftsent->fts_path);
+				errno = ENAMETOOLONG;
+				state->error = -1;
+				state->abort = true;
+				goto finish;
+			}
+
 			ent_st = *ftsent->fts_statp;
 			if (state->parallel)
 				pthread_mutex_unlock(&state->mutex);
diff --git a/libselinux/src/setrans_client.c b/libselinux/src/setrans_client.c
index faa12681..920f9032 100644
--- a/libselinux/src/setrans_client.c
+++ b/libselinux/src/setrans_client.c
@@ -66,7 +66,13 @@ static int setransd_open(void)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_UNIX;
-	strncpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path));
+
+	if (strlcpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path)) >= sizeof(addr.sun_path)) {
+		close(fd);
+		errno = EOVERFLOW;
+		return -1;
+	}
+
 	if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
 		close(fd);
 		return -1;
-- 
2.36.1


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

* Re: [RFC PATCH 4/4] libselinux: check for truncations
  2022-05-10 18:20 ` [RFC PATCH 4/4] libselinux: check for truncations Christian Göttsche
@ 2022-05-12 15:34   ` James Carter
  2022-05-17 15:07   ` [RFC PATCH v2 " Christian Göttsche
  2022-06-07 17:14   ` [PATCH v4 " Christian Göttsche
  2 siblings, 0 replies; 10+ messages in thread
From: James Carter @ 2022-05-12 15:34 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Tue, May 10, 2022 at 5:36 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Check for truncations when building or copying strings involving user
> input.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libselinux/src/canonicalize_context.c |  6 +++++-
>  libselinux/src/compute_av.c           |  7 ++++++-
>  libselinux/src/compute_create.c       |  6 ++++++
>  libselinux/src/compute_member.c       |  7 ++++++-
>  libselinux/src/compute_relabel.c      |  7 ++++++-
>  libselinux/src/compute_user.c         |  7 ++++++-
>  libselinux/src/selinux_restorecon.c   | 11 ++++++++++-
>  libselinux/src/setrans_client.c       |  8 +++++++-
>  8 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/libselinux/src/canonicalize_context.c b/libselinux/src/canonicalize_context.c
> index faab7305..8a22a4cd 100644
> --- a/libselinux/src/canonicalize_context.c
> +++ b/libselinux/src/canonicalize_context.c
> @@ -33,7 +33,11 @@ int security_canonicalize_context_raw(const char * con,
>                 ret = -1;
>                 goto out;
>         }
> -       strncpy(buf, con, size);
> +       if (strlcpy(buf, con, size) >= size) {
> +               errno = EOVERFLOW;
> +               ret = -1;
> +               goto out;
> +       }
>
>         ret = write(fd, buf, strlen(buf) + 1);
>         if (ret < 0)
> diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
> index 9d17339d..e513be6a 100644
> --- a/libselinux/src/compute_av.c
> +++ b/libselinux/src/compute_av.c
> @@ -40,8 +40,13 @@ int security_compute_av_flags_raw(const char * scon,
>         }
>
>         kclass = unmap_class(tclass);
> -       snprintf(buf, len, "%s %s %hu %x", scon, tcon,
> +
> +       ret = snprintf(buf, len, "%s %s %hu %x", scon, tcon,
>                  kclass, unmap_perm(tclass, requested));
> +       if (ret < 0 || ret >= len) {

error: comparison of integer expressions of different signedness:
‘int’ and ‘size_t’

> +               errno = EOVERFLOW;
> +               goto out2;
> +       }
>
>         ret = write(fd, buf, strlen(buf));
>         if (ret < 0)
> diff --git a/libselinux/src/compute_create.c b/libselinux/src/compute_create.c
> index 1d75714d..4cba2d2f 100644
> --- a/libselinux/src/compute_create.c
> +++ b/libselinux/src/compute_create.c
> @@ -75,8 +75,14 @@ int security_compute_create_name_raw(const char * scon,
>                 ret = -1;
>                 goto out;
>         }
> +
>         len = snprintf(buf, size, "%s %s %hu",
>                        scon, tcon, unmap_class(tclass));
> +       if (len < 0 || len >= size) {

error: comparison of integer expressions of different signedness:
‘int’ and ‘size_t’

> +               errno = EOVERFLOW;

You need ret = -1 here.

> +               goto out2;
> +       }
> +
>         if (objname &&
>             object_name_encode(objname, buf + len, size - len) < 0) {
>                 errno = ENAMETOOLONG;
> diff --git a/libselinux/src/compute_member.c b/libselinux/src/compute_member.c
> index 16234b79..82d76080 100644
> --- a/libselinux/src/compute_member.c
> +++ b/libselinux/src/compute_member.c
> @@ -36,7 +36,12 @@ int security_compute_member_raw(const char * scon,
>                 ret = -1;
>                 goto out;
>         }
> -       snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
> +
> +       ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
> +       if (ret < 0 || ret >= size) {

error: comparison of integer expressions of different signedness:
‘int’ and ‘size_t’

> +               errno = EOVERFLOW;
> +               goto out2;
> +       }
>
>         ret = write(fd, buf, strlen(buf));
>         if (ret < 0)
> diff --git a/libselinux/src/compute_relabel.c b/libselinux/src/compute_relabel.c
> index dd20d652..96259bac 100644
> --- a/libselinux/src/compute_relabel.c
> +++ b/libselinux/src/compute_relabel.c
> @@ -36,7 +36,12 @@ int security_compute_relabel_raw(const char * scon,
>                 ret = -1;
>                 goto out;
>         }
> -       snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
> +
> +       ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
> +       if (ret < 0 || ret >= size) {

error: comparison of integer expressions of different signedness:
‘int’ and ‘size_t’

> +               errno = EOVERFLOW;
> +               goto out2;
> +       }
>
>         ret = write(fd, buf, strlen(buf));
>         if (ret < 0)
> diff --git a/libselinux/src/compute_user.c b/libselinux/src/compute_user.c
> index ae5e7b4a..23a551e4 100644
> --- a/libselinux/src/compute_user.c
> +++ b/libselinux/src/compute_user.c
> @@ -38,7 +38,12 @@ int security_compute_user_raw(const char * scon,
>                 ret = -1;
>                 goto out;
>         }
> -       snprintf(buf, size, "%s %s", scon, user);
> +
> +       ret = snprintf(buf, size, "%s %s", scon, user);
> +       if (ret < 0 || ret >= size) {

error: comparison of integer expressions of different signedness:
‘int’ and ‘size_t’

Everything else looks fine.

Thanks,
Jim

> +               errno = EOVERFLOW;
> +               goto out2;
> +       }
>
>         ret = write(fd, buf, strlen(buf));
>         if (ret < 0)
> diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> index e6192912..7436dab5 100644
> --- a/libselinux/src/selinux_restorecon.c
> +++ b/libselinux/src/selinux_restorecon.c
> @@ -940,7 +940,16 @@ loop_body:
>                         }
>                         /* fall through */
>                 default:
> -                       strcpy(ent_path, ftsent->fts_path);
> +                       if (strlcpy(ent_path, ftsent->fts_path, sizeof(ent_path)) >= sizeof(ent_path)) {
> +                               selinux_log(SELINUX_ERROR,
> +                                           "Path name too long on %s.\n",
> +                                           ftsent->fts_path);
> +                               errno = ENAMETOOLONG;
> +                               state->error = -1;
> +                               state->abort = true;
> +                               goto finish;
> +                       }
> +
>                         ent_st = *ftsent->fts_statp;
>                         if (state->parallel)
>                                 pthread_mutex_unlock(&state->mutex);
> diff --git a/libselinux/src/setrans_client.c b/libselinux/src/setrans_client.c
> index faa12681..920f9032 100644
> --- a/libselinux/src/setrans_client.c
> +++ b/libselinux/src/setrans_client.c
> @@ -66,7 +66,13 @@ static int setransd_open(void)
>
>         memset(&addr, 0, sizeof(addr));
>         addr.sun_family = AF_UNIX;
> -       strncpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path));
> +
> +       if (strlcpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path)) >= sizeof(addr.sun_path)) {
> +               close(fd);
> +               errno = EOVERFLOW;
> +               return -1;
> +       }
> +
>         if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
>                 close(fd);
>                 return -1;
> --
> 2.36.1
>

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

* [RFC PATCH v2 4/4] libselinux: check for truncations
  2022-05-10 18:20 ` [RFC PATCH 4/4] libselinux: check for truncations Christian Göttsche
  2022-05-12 15:34   ` James Carter
@ 2022-05-17 15:07   ` Christian Göttsche
  2022-05-20 12:43     ` [PATCH v3 " Christian Göttsche
  2022-06-07 17:14   ` [PATCH v4 " Christian Göttsche
  2 siblings, 1 reply; 10+ messages in thread
From: Christian Göttsche @ 2022-05-17 15:07 UTC (permalink / raw)
  To: selinux

Check for truncations when building or copying strings involving user
input.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
   - add explicit casts to avoid int <-> size_t comparisons
   - ensure containing functions return -1
---
 libselinux/src/canonicalize_context.c |  6 +++++-
 libselinux/src/compute_av.c           |  8 +++++++-
 libselinux/src/compute_create.c       |  7 +++++++
 libselinux/src/compute_member.c       |  8 +++++++-
 libselinux/src/compute_relabel.c      |  8 +++++++-
 libselinux/src/compute_user.c         |  8 +++++++-
 libselinux/src/selinux_restorecon.c   | 10 +++++++++-
 libselinux/src/setrans_client.c       |  8 +++++++-
 8 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/libselinux/src/canonicalize_context.c b/libselinux/src/canonicalize_context.c
index faab7305..8a22a4cd 100644
--- a/libselinux/src/canonicalize_context.c
+++ b/libselinux/src/canonicalize_context.c
@@ -33,7 +33,11 @@ int security_canonicalize_context_raw(const char * con,
 		ret = -1;
 		goto out;
 	}
-	strncpy(buf, con, size);
+	if (strlcpy(buf, con, size) >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out;
+	}
 
 	ret = write(fd, buf, strlen(buf) + 1);
 	if (ret < 0)
diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
index 9d17339d..354a19e1 100644
--- a/libselinux/src/compute_av.c
+++ b/libselinux/src/compute_av.c
@@ -40,8 +40,14 @@ int security_compute_av_flags_raw(const char * scon,
 	}
 
 	kclass = unmap_class(tclass);
-	snprintf(buf, len, "%s %s %hu %x", scon, tcon,
+
+	ret = snprintf(buf, len, "%s %s %hu %x", scon, tcon,
 		 kclass, unmap_perm(tclass, requested));
+	if (ret < 0 || (size_t)ret >= len) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_create.c b/libselinux/src/compute_create.c
index 1d75714d..e9f3c96a 100644
--- a/libselinux/src/compute_create.c
+++ b/libselinux/src/compute_create.c
@@ -75,8 +75,15 @@ int security_compute_create_name_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
+
 	len = snprintf(buf, size, "%s %s %hu",
 		       scon, tcon, unmap_class(tclass));
+	if (len < 0 || (size_t)len >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
+
 	if (objname &&
 	    object_name_encode(objname, buf + len, size - len) < 0) {
 		errno = ENAMETOOLONG;
diff --git a/libselinux/src/compute_member.c b/libselinux/src/compute_member.c
index 16234b79..53d2f559 100644
--- a/libselinux/src/compute_member.c
+++ b/libselinux/src/compute_member.c
@@ -36,7 +36,13 @@ int security_compute_member_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_relabel.c b/libselinux/src/compute_relabel.c
index dd20d652..9c0a2304 100644
--- a/libselinux/src/compute_relabel.c
+++ b/libselinux/src/compute_relabel.c
@@ -36,7 +36,13 @@ int security_compute_relabel_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_user.c b/libselinux/src/compute_user.c
index ae5e7b4a..f55f945a 100644
--- a/libselinux/src/compute_user.c
+++ b/libselinux/src/compute_user.c
@@ -38,7 +38,13 @@ int security_compute_user_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s", scon, user);
+
+	ret = snprintf(buf, size, "%s %s", scon, user);
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 9dd6be81..ab79f543 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -961,7 +961,15 @@ loop_body:
 			}
 			/* fall through */
 		default:
-			strcpy(ent_path, ftsent->fts_path);
+			if (strlcpy(ent_path, ftsent->fts_path, sizeof(ent_path)) >= sizeof(ent_path)) {
+				selinux_log(SELINUX_ERROR,
+					    "Path name too long on %s.\n",
+					    ftsent->fts_path);
+				errno = ENAMETOOLONG;
+				state->error = -1;
+				state->abort = true;
+				goto finish;
+			}
 
 			if (state->parallel)
 				pthread_mutex_unlock(&state->mutex);
diff --git a/libselinux/src/setrans_client.c b/libselinux/src/setrans_client.c
index faa12681..920f9032 100644
--- a/libselinux/src/setrans_client.c
+++ b/libselinux/src/setrans_client.c
@@ -66,7 +66,13 @@ static int setransd_open(void)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_UNIX;
-	strncpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path));
+
+	if (strlcpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path)) >= sizeof(addr.sun_path)) {
+		close(fd);
+		errno = EOVERFLOW;
+		return -1;
+	}
+
 	if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
 		close(fd);
 		return -1;
-- 
2.36.1


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

* Re: [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read
  2022-05-10 18:20 [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read Christian Göttsche
                   ` (2 preceding siblings ...)
  2022-05-10 18:20 ` [RFC PATCH 4/4] libselinux: check for truncations Christian Göttsche
@ 2022-05-18 15:22 ` James Carter
  2022-06-08 13:40   ` James Carter
  3 siblings, 1 reply; 10+ messages in thread
From: James Carter @ 2022-05-18 15:22 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Tue, May 10, 2022 at 4:53 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> In case the function __policy_init() gets called with a NULL pointer,
> the stack variable path remains uninitialized (except at its last
> index).  If parsing the binary policy fails in sepol_policydb_read() the
> error branch would access those uninitialized memory.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For the series with v2 of patch 4:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/audit2why.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
> index ca38e13c..44a9a341 100644
> --- a/libselinux/src/audit2why.c
> +++ b/libselinux/src/audit2why.c
> @@ -192,25 +192,16 @@ static PyObject *finish(PyObject *self __attribute__((unused)), PyObject *args)
>  static int __policy_init(const char *init_path)
>  {
>         FILE *fp;
> -       char path[PATH_MAX];
> +       const char *curpolicy;
>         char errormsg[PATH_MAX+1024+20];
>         struct sepol_policy_file *pf = NULL;
>         int rc;
>         unsigned int cnt;
>
> -       path[PATH_MAX-1] = '\0';
>         if (init_path) {
> -               strncpy(path, init_path, PATH_MAX-1);
> -               fp = fopen(path, "re");
> -               if (!fp) {
> -                       snprintf(errormsg, sizeof(errormsg),
> -                                "unable to open %s:  %m\n",
> -                                path);
> -                       PyErr_SetString( PyExc_ValueError, errormsg);
> -                       return 1;
> -               }
> +               curpolicy = init_path;
>         } else {
> -               const char *curpolicy = selinux_current_policy_path();
> +               curpolicy = selinux_current_policy_path();
>                 if (!curpolicy) {
>                         /* SELinux disabled, must use -p option. */
>                         snprintf(errormsg, sizeof(errormsg),
> @@ -218,14 +209,15 @@ static int __policy_init(const char *init_path)
>                         PyErr_SetString( PyExc_ValueError, errormsg);
>                         return 1;
>                 }
> -               fp = fopen(curpolicy, "re");
> -               if (!fp) {
> -                       snprintf(errormsg, sizeof(errormsg),
> -                                "unable to open %s:  %m\n",
> -                                curpolicy);
> -                       PyErr_SetString( PyExc_ValueError, errormsg);
> -                       return 1;
> -               }
> +       }
> +
> +       fp = fopen(curpolicy, "re");
> +       if (!fp) {
> +               snprintf(errormsg, sizeof(errormsg),
> +                        "unable to open %s:  %m\n",
> +                        curpolicy);
> +               PyErr_SetString( PyExc_ValueError, errormsg);
> +               return 1;
>         }
>
>         avc = calloc(sizeof(struct avc_t), 1);
> @@ -249,7 +241,7 @@ static int __policy_init(const char *init_path)
>         sepol_policy_file_set_fp(pf, fp);
>         if (sepol_policydb_read(avc->policydb, pf)) {
>                 snprintf(errormsg, sizeof(errormsg),
> -                        "invalid binary policy %s\n", path);
> +                        "invalid binary policy %s\n", curpolicy);
>                 PyErr_SetString( PyExc_ValueError, errormsg);
>                 fclose(fp);
>                 return 1;
> --
> 2.36.1
>

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

* [PATCH v3 4/4] libselinux: check for truncations
  2022-05-17 15:07   ` [RFC PATCH v2 " Christian Göttsche
@ 2022-05-20 12:43     ` Christian Göttsche
  0 siblings, 0 replies; 10+ messages in thread
From: Christian Göttsche @ 2022-05-20 12:43 UTC (permalink / raw)
  To: selinux

Check for truncations when building or copying strings involving user
input.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

---
v3:
   - free buf in error branch in security_canonicalize_context_raw()
v2:
   - add explicit casts to avoid int <-> size_t comparisons
   - ensure containing functions return -1

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/canonicalize_context.c |  6 +++++-
 libselinux/src/compute_av.c           |  8 +++++++-
 libselinux/src/compute_create.c       |  7 +++++++
 libselinux/src/compute_member.c       |  8 +++++++-
 libselinux/src/compute_relabel.c      |  8 +++++++-
 libselinux/src/compute_user.c         |  8 +++++++-
 libselinux/src/selinux_restorecon.c   | 10 +++++++++-
 libselinux/src/setrans_client.c       |  8 +++++++-
 8 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/libselinux/src/canonicalize_context.c b/libselinux/src/canonicalize_context.c
index faab7305..6af8491d 100644
--- a/libselinux/src/canonicalize_context.c
+++ b/libselinux/src/canonicalize_context.c
@@ -33,7 +33,11 @@ int security_canonicalize_context_raw(const char * con,
 		ret = -1;
 		goto out;
 	}
-	strncpy(buf, con, size);
+	if (strlcpy(buf, con, size) >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf) + 1);
 	if (ret < 0)
diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
index 9d17339d..354a19e1 100644
--- a/libselinux/src/compute_av.c
+++ b/libselinux/src/compute_av.c
@@ -40,8 +40,14 @@ int security_compute_av_flags_raw(const char * scon,
 	}
 
 	kclass = unmap_class(tclass);
-	snprintf(buf, len, "%s %s %hu %x", scon, tcon,
+
+	ret = snprintf(buf, len, "%s %s %hu %x", scon, tcon,
 		 kclass, unmap_perm(tclass, requested));
+	if (ret < 0 || (size_t)ret >= len) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_create.c b/libselinux/src/compute_create.c
index 1d75714d..e9f3c96a 100644
--- a/libselinux/src/compute_create.c
+++ b/libselinux/src/compute_create.c
@@ -75,8 +75,15 @@ int security_compute_create_name_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
+
 	len = snprintf(buf, size, "%s %s %hu",
 		       scon, tcon, unmap_class(tclass));
+	if (len < 0 || (size_t)len >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
+
 	if (objname &&
 	    object_name_encode(objname, buf + len, size - len) < 0) {
 		errno = ENAMETOOLONG;
diff --git a/libselinux/src/compute_member.c b/libselinux/src/compute_member.c
index 16234b79..53d2f559 100644
--- a/libselinux/src/compute_member.c
+++ b/libselinux/src/compute_member.c
@@ -36,7 +36,13 @@ int security_compute_member_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_relabel.c b/libselinux/src/compute_relabel.c
index dd20d652..9c0a2304 100644
--- a/libselinux/src/compute_relabel.c
+++ b/libselinux/src/compute_relabel.c
@@ -36,7 +36,13 @@ int security_compute_relabel_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_user.c b/libselinux/src/compute_user.c
index ae5e7b4a..f55f945a 100644
--- a/libselinux/src/compute_user.c
+++ b/libselinux/src/compute_user.c
@@ -38,7 +38,13 @@ int security_compute_user_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s", scon, user);
+
+	ret = snprintf(buf, size, "%s %s", scon, user);
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 9dd6be81..ab79f543 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -961,7 +961,15 @@ loop_body:
 			}
 			/* fall through */
 		default:
-			strcpy(ent_path, ftsent->fts_path);
+			if (strlcpy(ent_path, ftsent->fts_path, sizeof(ent_path)) >= sizeof(ent_path)) {
+				selinux_log(SELINUX_ERROR,
+					    "Path name too long on %s.\n",
+					    ftsent->fts_path);
+				errno = ENAMETOOLONG;
+				state->error = -1;
+				state->abort = true;
+				goto finish;
+			}
 
 			if (state->parallel)
 				pthread_mutex_unlock(&state->mutex);
diff --git a/libselinux/src/setrans_client.c b/libselinux/src/setrans_client.c
index faa12681..920f9032 100644
--- a/libselinux/src/setrans_client.c
+++ b/libselinux/src/setrans_client.c
@@ -66,7 +66,13 @@ static int setransd_open(void)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_UNIX;
-	strncpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path));
+
+	if (strlcpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path)) >= sizeof(addr.sun_path)) {
+		close(fd);
+		errno = EOVERFLOW;
+		return -1;
+	}
+
 	if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
 		close(fd);
 		return -1;
-- 
2.36.1


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

* [PATCH v4 4/4] libselinux: check for truncations
  2022-05-10 18:20 ` [RFC PATCH 4/4] libselinux: check for truncations Christian Göttsche
  2022-05-12 15:34   ` James Carter
  2022-05-17 15:07   ` [RFC PATCH v2 " Christian Göttsche
@ 2022-06-07 17:14   ` Christian Göttsche
  2 siblings, 0 replies; 10+ messages in thread
From: Christian Göttsche @ 2022-06-07 17:14 UTC (permalink / raw)
  To: selinux

Check for truncations when building or copying strings involving user
input.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

---
v4:
   - rebase on top of de285252 ("Revert "libselinux: restorecon: pin
     file to avoid TOCTOU issues"")
v3:
   - free buf in error branch in security_canonicalize_context_raw()
v2:
   - add explicit casts to avoid int <-> size_t comparisons
   - ensure containing functions return -1

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/canonicalize_context.c |  6 +++++-
 libselinux/src/compute_av.c           |  8 +++++++-
 libselinux/src/compute_create.c       |  7 +++++++
 libselinux/src/compute_member.c       |  8 +++++++-
 libselinux/src/compute_relabel.c      |  8 +++++++-
 libselinux/src/compute_user.c         |  8 +++++++-
 libselinux/src/selinux_restorecon.c   | 11 ++++++++++-
 libselinux/src/setrans_client.c       |  8 +++++++-
 8 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/libselinux/src/canonicalize_context.c b/libselinux/src/canonicalize_context.c
index faab7305..6af8491d 100644
--- a/libselinux/src/canonicalize_context.c
+++ b/libselinux/src/canonicalize_context.c
@@ -33,7 +33,11 @@ int security_canonicalize_context_raw(const char * con,
 		ret = -1;
 		goto out;
 	}
-	strncpy(buf, con, size);
+	if (strlcpy(buf, con, size) >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf) + 1);
 	if (ret < 0)
diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
index 9d17339d..354a19e1 100644
--- a/libselinux/src/compute_av.c
+++ b/libselinux/src/compute_av.c
@@ -40,8 +40,14 @@ int security_compute_av_flags_raw(const char * scon,
 	}
 
 	kclass = unmap_class(tclass);
-	snprintf(buf, len, "%s %s %hu %x", scon, tcon,
+
+	ret = snprintf(buf, len, "%s %s %hu %x", scon, tcon,
 		 kclass, unmap_perm(tclass, requested));
+	if (ret < 0 || (size_t)ret >= len) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_create.c b/libselinux/src/compute_create.c
index 1d75714d..e9f3c96a 100644
--- a/libselinux/src/compute_create.c
+++ b/libselinux/src/compute_create.c
@@ -75,8 +75,15 @@ int security_compute_create_name_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
+
 	len = snprintf(buf, size, "%s %s %hu",
 		       scon, tcon, unmap_class(tclass));
+	if (len < 0 || (size_t)len >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
+
 	if (objname &&
 	    object_name_encode(objname, buf + len, size - len) < 0) {
 		errno = ENAMETOOLONG;
diff --git a/libselinux/src/compute_member.c b/libselinux/src/compute_member.c
index 16234b79..53d2f559 100644
--- a/libselinux/src/compute_member.c
+++ b/libselinux/src/compute_member.c
@@ -36,7 +36,13 @@ int security_compute_member_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_relabel.c b/libselinux/src/compute_relabel.c
index dd20d652..9c0a2304 100644
--- a/libselinux/src/compute_relabel.c
+++ b/libselinux/src/compute_relabel.c
@@ -36,7 +36,13 @@ int security_compute_relabel_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+
+	ret = snprintf(buf, size, "%s %s %hu", scon, tcon, unmap_class(tclass));
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/compute_user.c b/libselinux/src/compute_user.c
index ae5e7b4a..f55f945a 100644
--- a/libselinux/src/compute_user.c
+++ b/libselinux/src/compute_user.c
@@ -38,7 +38,13 @@ int security_compute_user_raw(const char * scon,
 		ret = -1;
 		goto out;
 	}
-	snprintf(buf, size, "%s %s", scon, user);
+
+	ret = snprintf(buf, size, "%s %s", scon, user);
+	if (ret < 0 || (size_t)ret >= size) {
+		errno = EOVERFLOW;
+		ret = -1;
+		goto out2;
+	}
 
 	ret = write(fd, buf, strlen(buf));
 	if (ret < 0)
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 9f5b326c..66e6a4a2 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -954,7 +954,16 @@ loop_body:
 			}
 			/* fall through */
 		default:
-			strcpy(ent_path, ftsent->fts_path);
+			if (strlcpy(ent_path, ftsent->fts_path, sizeof(ent_path)) >= sizeof(ent_path)) {
+				selinux_log(SELINUX_ERROR,
+					    "Path name too long on %s.\n",
+					    ftsent->fts_path);
+				errno = ENAMETOOLONG;
+				state->error = -1;
+				state->abort = true;
+				goto finish;
+			}
+
 			ent_st = *ftsent->fts_statp;
 			if (state->parallel)
 				pthread_mutex_unlock(&state->mutex);
diff --git a/libselinux/src/setrans_client.c b/libselinux/src/setrans_client.c
index faa12681..920f9032 100644
--- a/libselinux/src/setrans_client.c
+++ b/libselinux/src/setrans_client.c
@@ -66,7 +66,13 @@ static int setransd_open(void)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_UNIX;
-	strncpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path));
+
+	if (strlcpy(addr.sun_path, SETRANS_UNIX_SOCKET, sizeof(addr.sun_path)) >= sizeof(addr.sun_path)) {
+		close(fd);
+		errno = EOVERFLOW;
+		return -1;
+	}
+
 	if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
 		close(fd);
 		return -1;
-- 
2.36.1


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

* Re: [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read
  2022-05-18 15:22 ` [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read James Carter
@ 2022-06-08 13:40   ` James Carter
  0 siblings, 0 replies; 10+ messages in thread
From: James Carter @ 2022-06-08 13:40 UTC (permalink / raw)
  To: Christian Göttsche; +Cc: SElinux list

On Wed, May 18, 2022 at 11:22 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, May 10, 2022 at 4:53 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > In case the function __policy_init() gets called with a NULL pointer,
> > the stack variable path remains uninitialized (except at its last
> > index).  If parsing the binary policy fails in sepol_policydb_read() the
> > error branch would access those uninitialized memory.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For the series with v2 of patch 4:
> Acked-by: James Carter <jwcart2@gmail.com>
>
Merged with v4 of patch 4.
Thanks,
Jim

> > ---
> >  libselinux/src/audit2why.c | 34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
> > index ca38e13c..44a9a341 100644
> > --- a/libselinux/src/audit2why.c
> > +++ b/libselinux/src/audit2why.c
> > @@ -192,25 +192,16 @@ static PyObject *finish(PyObject *self __attribute__((unused)), PyObject *args)
> >  static int __policy_init(const char *init_path)
> >  {
> >         FILE *fp;
> > -       char path[PATH_MAX];
> > +       const char *curpolicy;
> >         char errormsg[PATH_MAX+1024+20];
> >         struct sepol_policy_file *pf = NULL;
> >         int rc;
> >         unsigned int cnt;
> >
> > -       path[PATH_MAX-1] = '\0';
> >         if (init_path) {
> > -               strncpy(path, init_path, PATH_MAX-1);
> > -               fp = fopen(path, "re");
> > -               if (!fp) {
> > -                       snprintf(errormsg, sizeof(errormsg),
> > -                                "unable to open %s:  %m\n",
> > -                                path);
> > -                       PyErr_SetString( PyExc_ValueError, errormsg);
> > -                       return 1;
> > -               }
> > +               curpolicy = init_path;
> >         } else {
> > -               const char *curpolicy = selinux_current_policy_path();
> > +               curpolicy = selinux_current_policy_path();
> >                 if (!curpolicy) {
> >                         /* SELinux disabled, must use -p option. */
> >                         snprintf(errormsg, sizeof(errormsg),
> > @@ -218,14 +209,15 @@ static int __policy_init(const char *init_path)
> >                         PyErr_SetString( PyExc_ValueError, errormsg);
> >                         return 1;
> >                 }
> > -               fp = fopen(curpolicy, "re");
> > -               if (!fp) {
> > -                       snprintf(errormsg, sizeof(errormsg),
> > -                                "unable to open %s:  %m\n",
> > -                                curpolicy);
> > -                       PyErr_SetString( PyExc_ValueError, errormsg);
> > -                       return 1;
> > -               }
> > +       }
> > +
> > +       fp = fopen(curpolicy, "re");
> > +       if (!fp) {
> > +               snprintf(errormsg, sizeof(errormsg),
> > +                        "unable to open %s:  %m\n",
> > +                        curpolicy);
> > +               PyErr_SetString( PyExc_ValueError, errormsg);
> > +               return 1;
> >         }
> >
> >         avc = calloc(sizeof(struct avc_t), 1);
> > @@ -249,7 +241,7 @@ static int __policy_init(const char *init_path)
> >         sepol_policy_file_set_fp(pf, fp);
> >         if (sepol_policydb_read(avc->policydb, pf)) {
> >                 snprintf(errormsg, sizeof(errormsg),
> > -                        "invalid binary policy %s\n", path);
> > +                        "invalid binary policy %s\n", curpolicy);
> >                 PyErr_SetString( PyExc_ValueError, errormsg);
> >                 fclose(fp);
> >                 return 1;
> > --
> > 2.36.1
> >

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

end of thread, other threads:[~2022-06-08 13:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-10 18:20 [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read Christian Göttsche
2022-05-10 18:20 ` [RFC PATCH 2/4] libselinux: add header guard for internal header Christian Göttsche
2022-05-10 18:20 ` [RFC PATCH 3/4] libselinux: introduce strlcpy Christian Göttsche
2022-05-10 18:20 ` [RFC PATCH 4/4] libselinux: check for truncations Christian Göttsche
2022-05-12 15:34   ` James Carter
2022-05-17 15:07   ` [RFC PATCH v2 " Christian Göttsche
2022-05-20 12:43     ` [PATCH v3 " Christian Göttsche
2022-06-07 17:14   ` [PATCH v4 " Christian Göttsche
2022-05-18 15:22 ` [RFC PATCH 1/4] libselinux: simplify policy path logic to avoid uninitialized read James Carter
2022-06-08 13:40   ` James Carter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.