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