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