diff mbox series

selinux: Fix strncpy in libselinux and libsepol

Message ID 20190531131446.9402-1-richard_c_haines@btinternet.com (mailing list archive)
State Changes Requested
Headers show
Series selinux: Fix strncpy in libselinux and libsepol | expand

Commit Message

Richard Haines May 31, 2019, 1:14 p.m. UTC 
When building with gcc9, get build errors such as:

genbools.c:24:2: error: ‘strncpy’ output may be truncated copying 8191
bytes from a string of length 8191 [-Werror=stringop-truncation]
   24 |  strncpy(dest, ptr, size);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
---
 libselinux/src/booleans.c |  4 ++--
 libsepol/src/genbools.c   | 20 +++++++++++---------
 2 files changed, 13 insertions(+), 11 deletions(-)

Comments

William Roberts May 31, 2019, 3:35 p.m. UTC | #1 
On Fri, May 31, 2019 at 8:23 AM Richard Haines
<richard_c_haines@btinternet.com> wrote:
>
> When building with gcc9, get build errors such as:
>
> genbools.c:24:2: error: ‘strncpy’ output may be truncated copying 8191
> bytes from a string of length 8191 [-Werror=stringop-truncation]
>    24 |  strncpy(dest, ptr, size);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> ---
>  libselinux/src/booleans.c |  4 ++--
>  libsepol/src/genbools.c   | 20 +++++++++++---------
>  2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c
> index ab1e0754..cdc03517 100644
> --- a/libselinux/src/booleans.c
> +++ b/libselinux/src/booleans.c
> @@ -539,7 +539,7 @@ int security_load_booleans(char *path)
>
>         __fsetlocking(boolf, FSETLOCKING_BYCALLER);
>         while (getline(&inbuf, &len, boolf) > 0) {
> -               int ret = process_boolean(inbuf, name, sizeof(name), &val);
> +               int ret = process_boolean(inbuf, name, len, &val);
>                 if (ret == -1)
>                         errors++;
>                 if (ret == 1)
> @@ -557,7 +557,7 @@ int security_load_booleans(char *path)
>                 int ret;
>                 __fsetlocking(boolf, FSETLOCKING_BYCALLER);
>                 while (getline(&inbuf, &len, boolf) > 0) {
> -                       ret = process_boolean(inbuf, name, sizeof(name), &val);
> +                       ret = process_boolean(inbuf, name, len, &val);
>                         if (ret == -1)
>                                 errors++;
>                         if (ret == 1)
> diff --git a/libsepol/src/genbools.c b/libsepol/src/genbools.c
> index d4a2df62..ad194ca6 100644
> --- a/libsepol/src/genbools.c
> +++ b/libsepol/src/genbools.c
> @@ -10,6 +10,8 @@
>  #include "private.h"
>  #include "dso.h"
>
> +#define FGET_BUFSIZ 255
> +
>  /* -- Deprecated -- */
>
>  static char *strtrim(char *dest, char *source, int size)
> @@ -32,7 +34,7 @@ static char *strtrim(char *dest, char *source, int size)
>
>  static int process_boolean(char *buffer, char *name, int namesize, int *val)
>  {
> -       char name1[BUFSIZ];
> +       char name1[FGET_BUFSIZ];
>         char *ptr = NULL;
>         char *tok;
>
> @@ -48,7 +50,7 @@ static int process_boolean(char *buffer, char *name, int namesize, int *val)
>                 ERR(NULL, "illegal boolean definition %s", buffer);
>                 return -1;
>         }
> -       strncpy(name1, tok, BUFSIZ - 1);
> +       strncpy(name1, tok, FGET_BUFSIZ - 1);
>         strtrim(name, name1, namesize - 1);
>
>         tok = strtok_r(NULL, "\0", &ptr);
> @@ -79,8 +81,8 @@ static int load_booleans(struct policydb *policydb, const char *path,
>  {
>         FILE *boolf;
>         char *buffer = NULL;
> -       char localbools[BUFSIZ];
> -       char name[BUFSIZ];
> +       char localbools[FGET_BUFSIZ];
> +       char name[FGET_BUFSIZ + 1];
>         int val;
>         int errors = 0, changes = 0;
>         struct cond_bool_datum *datum;
> @@ -90,12 +92,12 @@ static int load_booleans(struct policydb *policydb, const char *path,
>                 goto localbool;
>
>  #ifdef __APPLE__
> -        if ((buffer = (char *)malloc(255 * sizeof(char))) == NULL) {
> -          ERR(NULL, "out of memory");
> -         return -1;
> +       if ((buffer = (char *)malloc(FGET_BUFSIZ * sizeof(char))) == NULL) {
> +               ERR(NULL, "out of memory");
> +               return -1;
>         }
>
> -        while(fgets(buffer, 255, boolf) != NULL) {
> +       while (fgets(buffer, FGET_BUFSIZ, boolf) != NULL) {
>  #else
>         size_t size = 0;
>         while (getline(&buffer, &size, boolf) > 0) {
> @@ -124,7 +126,7 @@ static int load_booleans(struct policydb *policydb, const char *path,
>
>  #ifdef __APPLE__
>
> -         while(fgets(buffer, 255, boolf) != NULL) {
> +       while (fgets(buffer, FGET_BUFSIZ, boolf) != NULL) {
>  #else
>
>             while (getline(&buffer, &size, boolf) > 0) {
> --
> 2.21.0
>
This seems fine to me, ack. Still running the test suite, but I don't
see why anything would break.
Richard Haines May 31, 2019, 4:16 p.m. UTC | #2 
On Fri, 2019-05-31 at 08:35 -0700, William Roberts wrote:
> On Fri, May 31, 2019 at 8:23 AM Richard Haines
> <richard_c_haines@btinternet.com> wrote:
> > When building with gcc9, get build errors such as:
> > 
> > genbools.c:24:2: error: ‘strncpy’ output may be truncated copying
> > 8191
> > bytes from a string of length 8191 [-Werror=stringop-truncation]
> >    24 |  strncpy(dest, ptr, size);
> >       |  ^~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> > ---
> >  libselinux/src/booleans.c |  4 ++--
> >  libsepol/src/genbools.c   | 20 +++++++++++---------
> >  2 files changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c
> > index ab1e0754..cdc03517 100644
> > --- a/libselinux/src/booleans.c
> > +++ b/libselinux/src/booleans.c
> > @@ -539,7 +539,7 @@ int security_load_booleans(char *path)
> > 
> >         __fsetlocking(boolf, FSETLOCKING_BYCALLER);
> >         while (getline(&inbuf, &len, boolf) > 0) {
> > -               int ret = process_boolean(inbuf, name,
> > sizeof(name), &val);
> > +               int ret = process_boolean(inbuf, name, len, &val);
> >                 if (ret == -1)
> >                         errors++;
> >                 if (ret == 1)
> > @@ -557,7 +557,7 @@ int security_load_booleans(char *path)
> >                 int ret;
> >                 __fsetlocking(boolf, FSETLOCKING_BYCALLER);
> >                 while (getline(&inbuf, &len, boolf) > 0) {
> > -                       ret = process_boolean(inbuf, name,
> > sizeof(name), &val);
> > +                       ret = process_boolean(inbuf, name, len,
> > &val);
> >                         if (ret == -1)
> >                                 errors++;
> >                         if (ret == 1)
> > diff --git a/libsepol/src/genbools.c b/libsepol/src/genbools.c
> > index d4a2df62..ad194ca6 100644
> > --- a/libsepol/src/genbools.c
> > +++ b/libsepol/src/genbools.c
> > @@ -10,6 +10,8 @@
> >  #include "private.h"
> >  #include "dso.h"
> > 
> > +#define FGET_BUFSIZ 255
> > +
> >  /* -- Deprecated -- */
> > 
> >  static char *strtrim(char *dest, char *source, int size)
> > @@ -32,7 +34,7 @@ static char *strtrim(char *dest, char *source,
> > int size)
> > 
> >  static int process_boolean(char *buffer, char *name, int namesize,
> > int *val)
> >  {
> > -       char name1[BUFSIZ];
> > +       char name1[FGET_BUFSIZ];
> >         char *ptr = NULL;
> >         char *tok;
> > 
> > @@ -48,7 +50,7 @@ static int process_boolean(char *buffer, char
> > *name, int namesize, int *val)
> >                 ERR(NULL, "illegal boolean definition %s", buffer);
> >                 return -1;
> >         }
> > -       strncpy(name1, tok, BUFSIZ - 1);
> > +       strncpy(name1, tok, FGET_BUFSIZ - 1);
> >         strtrim(name, name1, namesize - 1);
> > 
> >         tok = strtok_r(NULL, "\0", &ptr);
> > @@ -79,8 +81,8 @@ static int load_booleans(struct policydb
> > *policydb, const char *path,
> >  {
> >         FILE *boolf;
> >         char *buffer = NULL;
> > -       char localbools[BUFSIZ];
> > -       char name[BUFSIZ];
> > +       char localbools[FGET_BUFSIZ];
> > +       char name[FGET_BUFSIZ + 1];
> >         int val;
> >         int errors = 0, changes = 0;
> >         struct cond_bool_datum *datum;
> > @@ -90,12 +92,12 @@ static int load_booleans(struct policydb
> > *policydb, const char *path,
> >                 goto localbool;
> > 
> >  #ifdef __APPLE__
> > -        if ((buffer = (char *)malloc(255 * sizeof(char))) == NULL)
> > {
> > -          ERR(NULL, "out of memory");
> > -         return -1;
> > +       if ((buffer = (char *)malloc(FGET_BUFSIZ * sizeof(char)))
> > == NULL) {
> > +               ERR(NULL, "out of memory");
> > +               return -1;
> >         }
> > 
> > -        while(fgets(buffer, 255, boolf) != NULL) {
> > +       while (fgets(buffer, FGET_BUFSIZ, boolf) != NULL) {
> >  #else
> >         size_t size = 0;
> >         while (getline(&buffer, &size, boolf) > 0) {
> > @@ -124,7 +126,7 @@ static int load_booleans(struct policydb
> > *policydb, const char *path,
> > 
> >  #ifdef __APPLE__
> > 
> > -         while(fgets(buffer, 255, boolf) != NULL) {
> > +       while (fgets(buffer, FGET_BUFSIZ, boolf) != NULL) {
> >  #else
> > 
> >             while (getline(&buffer, &size, boolf) > 0) {
> > --
> > 2.21.0
> > 
> This seems fine to me, ack. Still running the test suite, but I don't
> see why anything would break.

I tested the libsepol code by generating /etc/selinux/targeted/booleans
(e.g. auditadm_exec_content=false) and running semodule -B

I tested the libselinux code by running security_load_booleans(3) test.

This problem happened after I upgraded to Fedora 30.
diff mbox series

Patch

diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c
index ab1e0754..cdc03517 100644
--- a/libselinux/src/booleans.c
+++ b/libselinux/src/booleans.c
@@ -539,7 +539,7 @@  int security_load_booleans(char *path)
 
 	__fsetlocking(boolf, FSETLOCKING_BYCALLER);
 	while (getline(&inbuf, &len, boolf) > 0) {
-		int ret = process_boolean(inbuf, name, sizeof(name), &val);
+		int ret = process_boolean(inbuf, name, len, &val);
 		if (ret == -1)
 			errors++;
 		if (ret == 1)
@@ -557,7 +557,7 @@  int security_load_booleans(char *path)
 		int ret;
 		__fsetlocking(boolf, FSETLOCKING_BYCALLER);
 		while (getline(&inbuf, &len, boolf) > 0) {
-			ret = process_boolean(inbuf, name, sizeof(name), &val);
+			ret = process_boolean(inbuf, name, len, &val);
 			if (ret == -1)
 				errors++;
 			if (ret == 1)
diff --git a/libsepol/src/genbools.c b/libsepol/src/genbools.c
index d4a2df62..ad194ca6 100644
--- a/libsepol/src/genbools.c
+++ b/libsepol/src/genbools.c
@@ -10,6 +10,8 @@ 
 #include "private.h"
 #include "dso.h"
 
+#define FGET_BUFSIZ 255
+
 /* -- Deprecated -- */
 
 static char *strtrim(char *dest, char *source, int size)
@@ -32,7 +34,7 @@  static char *strtrim(char *dest, char *source, int size)
 
 static int process_boolean(char *buffer, char *name, int namesize, int *val)
 {
-	char name1[BUFSIZ];
+	char name1[FGET_BUFSIZ];
 	char *ptr = NULL;
 	char *tok;
 
@@ -48,7 +50,7 @@  static int process_boolean(char *buffer, char *name, int namesize, int *val)
 		ERR(NULL, "illegal boolean definition %s", buffer);
 		return -1;
 	}
-	strncpy(name1, tok, BUFSIZ - 1);
+	strncpy(name1, tok, FGET_BUFSIZ - 1);
 	strtrim(name, name1, namesize - 1);
 
 	tok = strtok_r(NULL, "\0", &ptr);
@@ -79,8 +81,8 @@  static int load_booleans(struct policydb *policydb, const char *path,
 {
 	FILE *boolf;
 	char *buffer = NULL;
-	char localbools[BUFSIZ];
-	char name[BUFSIZ];
+	char localbools[FGET_BUFSIZ];
+	char name[FGET_BUFSIZ + 1];
 	int val;
 	int errors = 0, changes = 0;
 	struct cond_bool_datum *datum;
@@ -90,12 +92,12 @@  static int load_booleans(struct policydb *policydb, const char *path,
 		goto localbool;
 
 #ifdef __APPLE__
-        if ((buffer = (char *)malloc(255 * sizeof(char))) == NULL) {
-          ERR(NULL, "out of memory");
-	  return -1;
+	if ((buffer = (char *)malloc(FGET_BUFSIZ * sizeof(char))) == NULL) {
+		ERR(NULL, "out of memory");
+		return -1;
 	}
 
-        while(fgets(buffer, 255, boolf) != NULL) {
+	while (fgets(buffer, FGET_BUFSIZ, boolf) != NULL) {
 #else
 	size_t size = 0;
 	while (getline(&buffer, &size, boolf) > 0) {
@@ -124,7 +126,7 @@  static int load_booleans(struct policydb *policydb, const char *path,
 
 #ifdef __APPLE__
 
-	  while(fgets(buffer, 255, boolf) != NULL) {
+	while (fgets(buffer, FGET_BUFSIZ, boolf) != NULL) {
 #else
 
 	    while (getline(&buffer, &size, boolf) > 0) {