On 05/19/2015 06:26 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] with V2 fixing those in [2]. > V3 corrects int32_t/uint32_t for *_len entries. > > 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 > [2] http://marc.info/?l=selinux&m=143161763905159&w=2 > > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx> > --- > libselinux/src/label_file.c | 204 ++++++++++++++++++++++++++++---------------- > libselinux/src/label_file.h | 20 ++++- > 2 files changed, 149 insertions(+), 75 deletions(-) > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c > index c722f29..64839e0 100644 > --- a/libselinux/src/label_file.c > +++ b/libselinux/src/label_file.c > @@ -304,57 +300,80 @@ 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; > - 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(&version, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS) > return -1; > - addr += sizeof(uint32_t); > > - if (*section_len >= SELINUX_COMPILED_FCONTEXT_PCRE_VERS) { > + if (version >= 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 */ > - 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 */ > - addr += *plen; > + > + rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || entry_len <= 0) > + return -1; Can't be < 0 since it is uint32_t and don't need to separately check for == 0 because you check the length against the expected version string length below. > + > + /* Check version lengths */ > + if (len != entry_len) > + return -1; > + > + /* Check if pcre version mismatch */ > + str_buf = malloc(entry_len); Need to malloc entry_len + 1 bytes in order to NUL-terminate it below without buffer overflow. Also, need to check that entry_len < UINT32_MAX before adding one to it. > + if (!str_buf) > + return -1; > + > + rc = next_entry(str_buf, mmap_area, entry_len); > + if (rc < 0) { > + free(str_buf); > + return -1; > + } > + > + str_buf[entry_len] = 0; Should probably be consistent, e.g. use = '\0' or = 0 throughout. I think the former is preferred stylistically and maybe for portability, although I don't know of any case where it would differ. > + if ((strcmp(str_buf, pcre_version()) != 0)) { > + free(str_buf); > + return -1; > + } > + free(str_buf); > } > > /* allocate the stems_data array */ > - section_len = (uint32_t *)addr; > - addr += sizeof(uint32_t); > + rc = next_entry(&stem_map_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || stem_map_len <= 0) > + return -1; Can't be < 0 as it is uint32_t. > > /* > * map indexed by the stem # in the mmap file and contains the stem > * number in the data stem_arr > */ > - stem_map_len = *section_len; > stem_map = calloc(stem_map_len, sizeof(*stem_map)); > if (!stem_map) > return -1; > > - for (i = 0; i < *section_len; i++) { > + for (i = 0; i < stem_map_len; i++) { > char *buf; > uint32_t stem_len; > int newid; > > /* the length does not inlude the nul */ > - plen = (uint32_t *)addr; > - addr += sizeof(uint32_t); > + rc = next_entry(&stem_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || stem_len <= 0) { > + rc = -1; > + goto err; > + } Likewise, can't be < 0 due to type. > + > + buf = (char *)mmap_area->addr; > + /* Check if buf is going to over-run before null check. */ > + rc = next_entry(NULL, mmap_area, (stem_len + 1)); But you should test that stem_len < UINT32_MAX before adding one to it so it doesn't wrap around. > + if (rc < 0) > + goto err; > > - stem_len = *plen; > - buf = (char *)addr; > - addr += (stem_len + 1); // +1 is the nul > + if (buf[stem_len] != '\0') { > + rc = -1; > + goto err; > + } > > /* store the mapping between old and new */ > newid = find_stem(data, buf, stem_len); > @@ -370,12 +389,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(®ex_array_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || regex_array_len <= 0) { > + rc = -1; > + goto err; > + } Can't be < 0. > > - for (i = 0; i < *section_len; i++) { > + for (i = 0; i < regex_array_len; i++) { > struct spec *spec; > - int32_t stem_id; > + int32_t stem_id, meta_chars; > > rc = grow_specs(data); > if (rc < 0) > @@ -385,53 +407,89 @@ 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 */ > + rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || entry_len <= 0) { > + rc = -1; > goto err; > + } Only need to check for entry_len != 0, < not possible. > > - if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len) > - return -1; > - addr += *plen; > + str_buf = malloc(entry_len); > + if (!str_buf) { > + rc = -1; > + goto err; > + } > + rc = next_entry(str_buf, mmap_area, entry_len); > + 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 (str_buf[entry_len - 1] != '\0') { > + free(str_buf); > + rc = -1; > + goto err; > + } This is why you need the != 0 test above. > + spec->lr.ctx_raw = str_buf; > + > + /* Process regex string */ > + rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || entry_len <= 0) { > + rc = -1; > + goto err; > + } Don't need the < 0 part of the test, just != 0. > > - spec->mode = *(mode_t *)addr; > - addr += sizeof(mode_t); > + spec->regex_str = (char *)mmap_area->addr; > + rc = next_entry(NULL, mmap_area, entry_len); > + if (rc < 0) > + goto err; > + > + if (spec->regex_str[entry_len - 1] != '\0') { > + rc = -1; > + goto err; > + } > + > + /* Process mode */ > + rc = next_entry(&spec->mode, mmap_area, sizeof(mode_t)); > + if (rc < 0) > + goto err; > > /* map the stem id from the mmap file to the data->stem_arr */ > - stem_id = *(int32_t *)addr; > - if (stem_id == -1 || stem_id >= stem_map_len) > + rc = next_entry(&stem_id, mmap_area, sizeof(int32_t)); > + if (rc < 0) > + goto err; > + > + if (stem_id < 0 || stem_id >= stem_map_len) > spec->stem_id = -1; > - else > + else > spec->stem_id = stem_map[stem_id]; > - addr += sizeof(int32_t); > > /* retrieve the hasMetaChars bit */ > - spec->hasMetaChars = *(uint32_t *)addr; > - addr += sizeof(uint32_t); > + rc = next_entry(&meta_chars, mmap_area, sizeof(uint32_t)); > + if (rc < 0) > + goto err; > > - 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; > + spec->hasMetaChars = meta_chars; > > - plen = (uint32_t *)addr; > - addr += sizeof(uint32_t); > - spec->lsd.study_data = (void *)addr; > + /* Process regex and study_data entries */ > + rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || entry_len <= 0) { > + rc = -1; > + goto err; > + } Don't need the < 0 test, just != 0. > + spec->regex = (pcre *)mmap_area->addr; > + rc = next_entry(NULL, mmap_area, entry_len); > + if (rc < 0) > + goto err; Seems unsafe (we don't do anything to ensure that spec->regex is pointing to a valid pcre, or that its entry_len is some minimal size required for the underlying structure) but offhand I don't know if there is any function exported by libpcre that would allow us to validate it. You don't have to address it; just noting it. Was wondering if we could use pcre_fullinfo() to get the expected sizes and compare as we do when writing in sefcontext_compile(), but that presumes that re and sd are in fact already valid structures so I don't think it helps here. > + > + rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); > + if (rc < 0 || entry_len <= 0) { > + rc = -1; > + goto err; > + } > + spec->lsd.study_data = (void *)mmap_area->addr; > spec->lsd.flags |= PCRE_EXTRA_STUDY_DATA; > - if (addr + *plen >= (char *)mmap_area->addr + mmap_area->len) > - return -1; > - addr += *plen; > + rc = next_entry(NULL, mmap_area, entry_len); > + if (rc < 0) > + goto err; Likewise for spec->lsd.study_data. > > 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.
