On 05/14/2015 10:36 AM, Richard Haines wrote: > The reading of bin files has been changed to follow that of loading > policy to catch over-runs. Entries that should be NULL terminated are > also checked. If any error, then process the text file. This should > fix all problems highlighted in [1]. > > Tested with bin files built using sefcontext_compile PCRE_VERS 1 and 2. > > The following is a rough guide to the difference in processing a bin > file against a text file: > 6K entries - x5 > 4K entries - x4 > 1K entries - x3 > 500 entries - x2 > > [1] http://marc.info/?l=selinux&m=143101983922281&w=2 > > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx> > --- > libselinux/src/label_file.c | 167 +++++++++++++++++++++++++++----------------- > libselinux/src/label_file.h | 20 +++++- > 2 files changed, 121 insertions(+), 66 deletions(-) > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c > index c722f29..d6103d8 100644 > --- a/libselinux/src/label_file.c > +++ b/libselinux/src/label_file.c > @@ -8,7 +8,6 @@ > * developed by Secure Computing Corporation. > */ > > -#include <assert.h> > #include <fcntl.h> > #include <stdarg.h> > #include <string.h> > @@ -248,9 +247,9 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat * > struct mmap_area *mmap_area; > > uint32_t i; > - uint32_t *magic; > - uint32_t *section_len; > - uint32_t *plen; > + uint32_t magic[2]; > + uint32_t section_len[2]; > + uint32_t plen[2]; Why isn't this just: uint32_t magic, section_len, plen; and use &magic, §ion_len, &plen in calls to next_entry() and magic, section_len, and plen in place of *magic, *section_len, *plen. > @@ -304,35 +303,36 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat * > data->mmap_areas = mmap_area; > > /* check if this looks like an fcontext file */ > - magic = (uint32_t *)addr; > - if (*magic != SELINUX_MAGIC_COMPILED_FCONTEXT) > + rc = next_entry(magic, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || *magic != SELINUX_MAGIC_COMPILED_FCONTEXT) > return -1; Then this becomes: rc = next_entry(&magic, mmap_area, sizeof(uint32_t)); if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT) return -1; Or, alternatively, you could do it as in libsepol and declare: uint32_t buf[2]; rc = next_entry(buf, mmap_area, sizeof buf); and read several values at one time, e.g. the magic as buf[0], the version as buf[1], and either the pcre version string length or the stem map length as buf[2]. BTW, there is no endianness conversion here, unlike libsepol for binary policies? So you have to ensure that you generate file_contexts.bin on the same endian architecture as you use it? That's a bit worrisome although probably not an issue at the moment. Can't change that though without more substantial alteration and a version change, so don't worry about it for now. > - addr += sizeof(uint32_t); > > /* check if this version is higher than we understand */ > - section_len = (uint32_t *)addr; > - if (*section_len > SELINUX_COMPILED_FCONTEXT_MAX_VERS) > + rc = next_entry(section_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || *section_len > SELINUX_COMPILED_FCONTEXT_MAX_VERS) > return -1; Kind of strange that we reuse a variable called section_len for the binary file format version, the pcre string version length, and the stem array length. > - addr += sizeof(uint32_t); > > if (*section_len >= SELINUX_COMPILED_FCONTEXT_PCRE_VERS) { > len = strlen(pcre_version()); > - plen = (uint32_t *)addr; > - if (*plen > mmap_area->len) > - return -1; /* runs off the end of the map */ > - if (len != *plen) > - return -1; /* pcre version length mismatch */ You want to retain the length comparison before doing the memcmp. > - addr += sizeof(uint32_t); > - if (memcmp((char *)addr, pcre_version(), len)) > + > + rc = next_entry(plen, mmap_area, sizeof(uint32_t)); > + if (rc < 0) > + return -1; > + > + /* Check for over-run then safe to do memcmp */ > + rc = next_entry(NULL, mmap_area, *plen); > + if (rc < 0) > + return -1; > + > + if (memcmp((char *)mmap_area->addr - *plen, > + pcre_version(), len)) This is a strange idiom. I'd prefer to either allocate a string and copy it in, as we do in libsepol for e.g. the policydb_str, or just perform the length check inline first and and don't call next_entry() until after comparing. > @@ -349,12 +349,16 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat * > int newid; > > /* the length does not inlude the nul */ > - plen = (uint32_t *)addr; > - addr += sizeof(uint32_t); > + rc = next_entry(plen, mmap_area, sizeof(uint32_t)); > + if (rc < 0) > + goto err; > > stem_len = *plen; > - buf = (char *)addr; > - addr += (stem_len + 1); // +1 is the nul > + buf = (char *)mmap_area->addr; > + /* +1 for null */ > + rc = next_entry(NULL, mmap_area, (stem_len + 1)); > + if (rc < 0) > + goto err; Likewise, and where do you check that buf[stem_len] == '\0'? Otherwise find_stem() can run off the end. Also, can stem_len + 1 overflow to 0? This btw was another mistake in the file format; should have been at least consistent throughout as to whether or not strings were NUL-terminated and no real reason to include them in the file since we have to verify it is NUL-terminated at load anyway - might as well just add it at load time too. > > /* store the mapping between old and new */ > newid = find_stem(data, buf, stem_len); > @@ -370,12 +374,15 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat * > } > > /* allocate the regex array */ > - section_len = (uint32_t *)addr; > - addr += sizeof(*section_len); > + rc = next_entry(section_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0) > + goto err; > > for (i = 0; i < *section_len; i++) { > struct spec *spec; > - int32_t stem_id; > + int32_t stem_id[2]; > + int32_t meta_chars[2]; Don't need arrays here either. > + char *str_buf; > > rc = grow_specs(data); > if (rc < 0) > @@ -385,53 +392,85 @@ static int load_mmap(struct selabel_handle *rec, const char *path, struct stat * > spec->from_mmap = 1; > spec->regcomp = 1; > > - plen = (uint32_t *)addr; > - addr += sizeof(uint32_t); > - rc = -1; > - spec->lr.ctx_raw = strdup((char *)addr); > - if (!spec->lr.ctx_raw) > + /* Process context by malloc space plen size, check for NULL, > + * then add ptr to spec->lr.ctx_raw. */ > + rc = next_entry(plen, mmap_area, sizeof(uint32_t)); > + if (rc < 0) > goto err; > > - if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len) > - return -1; > - addr += *plen; > + str_buf = malloc(*plen); > + if (!str_buf) { > + rc = -1; > + goto err; > + } > + rc = next_entry(str_buf, mmap_area, *plen); > + if (rc < 0) > + goto err; > > - plen = (uint32_t *)addr; > - addr += sizeof(uint32_t); > - spec->regex_str = (char *)addr; > - if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len) > - return -1; > - addr += *plen; > + if (strlen(str_buf) != *plen - 1) { No, check that str_buf[*plen-1] == '\0'; strlen(str_buf) will run off the end, potentially unbounded, if not terminated. > + free(str_buf); > + rc = -1; > + goto err; > + } > + spec->lr.ctx_raw = str_buf; > + > + /* Process regex string by adding addr to spec->regex_str, > + * skip over entry to catch over-runs, then check if > + * NULL terminated. */ > + rc = next_entry(plen, mmap_area, sizeof(uint32_t)); > + if (rc < 0) > + goto err; > > - spec->mode = *(mode_t *)addr; > - addr += sizeof(mode_t); > + spec->regex_str = (char *)mmap_area->addr; > + rc = next_entry(NULL, mmap_area, *plen); > + if (rc < 0) > + goto err; > + > + if (strlen(spec->regex_str) != *plen - 1) { > + rc = -1; > + goto err; > + } Same as above. _______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
