I would like to get this correct so will do further work as per your comments, then send another patch. > On Thursday, 7 May 2015, 18:25, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > > On 05/07/2015 10:40 AM, Richard Haines wrote: >> Check buffer address limits when processing *.bin files >> to catch any over-runs. On failure process text file instead. >> >> To test, the bin files were corrupted by adding and removing >> various bits of data. Various file sizes were also checked and >> all were caught by the patch. >> >> Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx> > > Thanks, applied, but see below. > >> --- >> libselinux/src/label_file.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c >> index b3e5671..c722f29 100644 >> --- a/libselinux/src/label_file.c >> +++ b/libselinux/src/label_file.c >> @@ -325,6 +325,8 @@ static int load_mmap(struct selabel_handle *rec, const > char *path, struct stat * >> addr += sizeof(uint32_t); >> if (memcmp((char *)addr, pcre_version(), len)) >> return -1; /* pcre version content mismatch */ >> + if (addr + *plen >= (char *)mmap_area->addr + > mmap_area->len) >> + return -1; /* Buffer over-run */ > > Technically this is too late; it already read *plen bytes starting at > addr via the memcmp above. Also, what happens if *plen is large and > (addr + *plen) overflows? Compare with the checking performed on every > fetch from the policy image via next_entry() in libsepol in the > PF_USE_MEMORY case, which is likewise loading from an in-memory policy > whether mmap'd or otherwise. > >> addr += *plen; >> } >> >> @@ -390,11 +392,15 @@ static int load_mmap(struct selabel_handle *rec, > const char *path, struct stat * >> if (!spec->lr.ctx_raw) >> goto err; >> >> + if (addr + *plen >= (char *)mmap_area->addr + > mmap_area->len) >> + return -1; > > Similarly, too late. Worse, the preceding code did a strdup() on the > addr without ever validating that it was NUL terminated. Also seem to > be missing corresponding checking for the earlier stem reading code. > >> addr += *plen; >> >> 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; > > Not too late since we haven't yet dereferenced it, but no validation > that regex_str is NUL-terminated. > > >> addr += *plen; >> >> spec->mode = *(mode_t *)addr; >> @@ -415,12 +421,16 @@ static int load_mmap(struct selabel_handle *rec, > const char *path, struct stat * >> plen = (uint32_t *)addr; >> addr += sizeof(uint32_t); >> spec->regex = (pcre *)addr; >> + if (addr + *plen >= (char *)mmap_area->addr + > mmap_area->len) >> + return -1; >> addr += *plen; >> >> plen = (uint32_t *)addr; >> addr += sizeof(uint32_t); >> spec->lsd.study_data = (void *)addr; >> spec->lsd.flags |= PCRE_EXTRA_STUDY_DATA; >> + if (addr + *plen >= (char *)mmap_area->addr + > mmap_area->len) >> + return -1; >> addr += *plen; >> >> data->nspec++; >> > _______________________________________________ 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.
