All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] selinux: Fix strncpy in libselinux and libsepol
@ 2019-05-31 15:16 Richard Haines
  2019-05-31 19:35 ` Stephen Smalley
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Haines @ 2019-05-31 15:16 UTC (permalink / raw)
  To: selinux; +Cc: Richard Haines

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH] selinux: Fix strncpy in libselinux and libsepol
  2019-05-31 15:16 [PATCH] selinux: Fix strncpy in libselinux and libsepol Richard Haines
@ 2019-05-31 19:35 ` Stephen Smalley
  2019-06-02 13:35   ` Richard Haines
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Smalley @ 2019-05-31 19:35 UTC (permalink / raw)
  To: Richard Haines, selinux, William Roberts

On 5/31/19 11:16 AM, Richard Haines 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);
>        |  ^~~~~~~~~~~~~~~~~~~~~~~~

It would be nice if we could just remove all of this code, modulo 
ABI/API concerns (maybe we could reduce the public interfaces to 
no-ops?).  It is all legacy code I think, predating kernel 2.6.22 
(kernel automatically preserves boolean values across policy reload) and 
the use of libsemanage (managed policy, persistent boolean changes 
directly applied to the kernel policy file).  Probably not used by 
anything later than RHEL 4, which should be dead and gone by now I hope.

> 
> 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);

This might fix the warning but is it correct? len is the size of the 
buffer allocated by getline, which could be larger or smaller than 
sizeof name and also could be larger than the number of bytes read. 
process_boolean() seems to want the size of the name buffer as a bound 
for strncpy() in the strtrim() call. strtrim() also seems to be using it 
wrongly as a bound for the source aka name1, presuming they are the same 
size.

>   		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);

Ditto.

>   			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];

Similarly seems to be making faulty assumptions about using the same 
buffer sizes throughout.

>   	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) {

I think this was just a hack to make it build on macOS for Android.  But 
there is no reason for this code to be used there.  I wouldn't change 
the other buffer sizes to match.

>   #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) {
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] selinux: Fix strncpy in libselinux and libsepol
  2019-05-31 19:35 ` Stephen Smalley
@ 2019-06-02 13:35   ` Richard Haines
  2019-06-03 20:43     ` Stephen Smalley
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Haines @ 2019-06-02 13:35 UTC (permalink / raw)
  To: Stephen Smalley, selinux, William Roberts

On Fri, 2019-05-31 at 15:35 -0400, Stephen Smalley wrote:
> On 5/31/19 11:16 AM, Richard Haines 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);
> >        |  ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> It would be nice if we could just remove all of this code, modulo 
> ABI/API concerns (maybe we could reduce the public interfaces to 
> no-ops?).  It is all legacy code I think, predating kernel 2.6.22 
> (kernel automatically preserves boolean values across policy reload)
> and 
> the use of libsemanage (managed policy, persistent boolean changes 
> directly applied to the kernel policy file).  Probably not used by 
> anything later than RHEL 4, which should be dead and gone by now I
> hope.

Any comments on this list:

libsepol/src/genusers.c
	delete file + cleanup

libsepol/src/genbools.c
	delete file + cleanup

libselinux/src/load_policy.c
Remove areas that use:
	genbools_array
	genusers
	genbools
	setlocaldefs
	preservebools

libselinux/src/booleans.c
no-op:
	security_load_booleans()

modify as no need for "int permanent":
	security_set_boolean_list()

libselinux/src/selinux_config.c
Remove:
	SETLOCALDEFS

Clean up any leftovers (man pages etc.)


> 
> > 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);
> 
> This might fix the warning but is it correct? len is the size of the 
> buffer allocated by getline, which could be larger or smaller than 
> sizeof name and also could be larger than the number of bytes read. 
> process_boolean() seems to want the size of the name buffer as a
> bound 
> for strncpy() in the strtrim() call. strtrim() also seems to be using
> it 
> wrongly as a bound for the source aka name1, presuming they are the
> same 
> size.
> 
Sent a V2 patch that I hope fixes these.

> >   		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);
> 
> Ditto.
> 
> >   			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];
> 
> Similarly seems to be making faulty assumptions about using the same 
> buffer sizes throughout.
> 
> >   	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) {
> 
> I think this was just a hack to make it build on macOS for
> Android.  But 
> there is no reason for this code to be used there.  I wouldn't
> change 
> the other buffer sizes to match.
> 
> >   #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) {
> > 


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] selinux: Fix strncpy in libselinux and libsepol
  2019-06-02 13:35   ` Richard Haines
@ 2019-06-03 20:43     ` Stephen Smalley
  0 siblings, 0 replies; 7+ messages in thread
From: Stephen Smalley @ 2019-06-03 20:43 UTC (permalink / raw)
  To: Richard Haines, selinux, William Roberts

On 6/2/19 9:35 AM, Richard Haines wrote:
> On Fri, 2019-05-31 at 15:35 -0400, Stephen Smalley wrote:
>> On 5/31/19 11:16 AM, Richard Haines 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);
>>>         |  ^~~~~~~~~~~~~~~~~~~~~~~~
>>
>> It would be nice if we could just remove all of this code, modulo
>> ABI/API concerns (maybe we could reduce the public interfaces to
>> no-ops?).  It is all legacy code I think, predating kernel 2.6.22
>> (kernel automatically preserves boolean values across policy reload)
>> and
>> the use of libsemanage (managed policy, persistent boolean changes
>> directly applied to the kernel policy file).  Probably not used by
>> anything later than RHEL 4, which should be dead and gone by now I
>> hope.
> 
> Any comments on this list:
> 
> libsepol/src/genusers.c
> 	delete file + cleanup
> 
> libsepol/src/genbools.c
> 	delete file + cleanup
> 
> libselinux/src/load_policy.c
> Remove areas that use:
> 	genbools_array
> 	genusers
> 	genbools
> 	setlocaldefs
> 	preservebools
> 
> libselinux/src/booleans.c
> no-op:
> 	security_load_booleans()
> 
> modify as no need for "int permanent":
> 	security_set_boolean_list()

Yes, we'd keep the argument to preserve API/ABI but drop the code 
handling permanent == 1.  The callers were already updated to use 
libsemanage instead for saving persistent booleans.

> 
> libselinux/src/selinux_config.c
> Remove:
> 	SETLOCALDEFS
> 
> Clean up any leftovers (man pages etc.)

FWIW, see 
https://lore.kernel.org/selinux/1177594803.24282.322.camel@moss-spartans.epoch.ncsc.mil/

It never landed upstream since policyrep never merged to master.
Picked up again here:
https://lore.kernel.org/selinux/1201203958.21288.120.camel@moss-spartans.epoch.ncsc.mil/

But RHEL4 was still a concern, so we fell back to just this patch:
https://lore.kernel.org/selinux/1202311592.27371.150.camel@moss-spartans.epoch.ncsc.mil/

I'm hoping we can remove it all now finally.

> 
> 
>>
>>> 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);
>>
>> This might fix the warning but is it correct? len is the size of the
>> buffer allocated by getline, which could be larger or smaller than
>> sizeof name and also could be larger than the number of bytes read.
>> process_boolean() seems to want the size of the name buffer as a
>> bound
>> for strncpy() in the strtrim() call. strtrim() also seems to be using
>> it
>> wrongly as a bound for the source aka name1, presuming they are the
>> same
>> size.
>>
> Sent a V2 patch that I hope fixes these.
> 
>>>    		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);
>>
>> Ditto.
>>
>>>    			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];
>>
>> Similarly seems to be making faulty assumptions about using the same
>> buffer sizes throughout.
>>
>>>    	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) {
>>
>> I think this was just a hack to make it build on macOS for
>> Android.  But
>> there is no reason for this code to be used there.  I wouldn't
>> change
>> the other buffer sizes to match.
>>
>>>    #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) {
>>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] selinux: Fix strncpy in libselinux and libsepol
  2019-05-31 15:35 ` William Roberts
@ 2019-05-31 16:16   ` Richard Haines
  0 siblings, 0 replies; 7+ messages in thread
From: Richard Haines @ 2019-05-31 16:16 UTC (permalink / raw)
  To: William Roberts; +Cc: selinux

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] selinux: Fix strncpy in libselinux and libsepol
  2019-05-31 13:14 Richard Haines
@ 2019-05-31 15:35 ` William Roberts
  2019-05-31 16:16   ` Richard Haines
  0 siblings, 1 reply; 7+ messages in thread
From: William Roberts @ 2019-05-31 15:35 UTC (permalink / raw)
  To: Richard Haines; +Cc: selinux

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH] selinux: Fix strncpy in libselinux and libsepol
@ 2019-05-31 13:14 Richard Haines
  2019-05-31 15:35 ` William Roberts
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Haines @ 2019-05-31 13:14 UTC (permalink / raw)
  To: selinux; +Cc: Richard Haines

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-04 18:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 15:16 [PATCH] selinux: Fix strncpy in libselinux and libsepol Richard Haines
2019-05-31 19:35 ` Stephen Smalley
2019-06-02 13:35   ` Richard Haines
2019-06-03 20:43     ` Stephen Smalley
  -- strict thread matches above, loose matches on Subject: below --
2019-05-31 13:14 Richard Haines
2019-05-31 15:35 ` William Roberts
2019-05-31 16:16   ` Richard Haines

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.