diff mbox series

[RFC,4/4] libselinux: check for truncations

Message ID 20220510182039.28771-4-cgzones@googlemail.com (mailing list archive)
State Superseded
Headers show
Series [RFC,1/4] libselinux: simplify policy path logic to avoid uninitialized read | expand

Commit Message

Christian Göttsche May 10, 2022, 6:20 p.m. UTC 
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(-)

Comments

James Carter May 12, 2022, 3:34 p.m. UTC | #1 
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
>
diff mbox series

Patch

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;