From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 10/22] libelf: check nul-terminated strings properly Date: Tue, 11 Jun 2013 00:17:49 +0100 Message-ID: <51B65E9D.3030303@citrix.com> References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com> <1370629642-6990-11-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1370629642-6990-11-git-send-email-ian.jackson@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org On 07/06/13 19:27, Ian Jackson wrote: > It is not safe to simply take pointers into the ELF and use them as C > pointers. They might not be properly nul-terminated (and the pointers > might be wild). > > So we are going to introduce a new function elf_strval for safely > getting strings. This will check that the addresses are in range and > that there is a proper nul-terminated string. Of course it might > discover that there isn't. In that case, it will be made to fail. > This means that elf_note_name might fail, too. > > For the benefit of call sites which are just going to pass the value > to a printf-like function, we provide elf_strfmt which returns > "(invalid)" on failure rather than NULL. > > In this patch we introduce dummy definitions of these functions. We > introduce calls to elf_strval and elf_strfmt everywhere, and update > all the call sites with appropriate error checking. > > There is not yet any semantic change, since before this patch all the > places where we introduce elf_strval dereferenced the value anyway, so > it mustn't have been NULL. > > In future patches, when elf_strval is made able return NULL, when it > does so it will mark the elf "broken" so that an appropriate > diagnostic can be printed. > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Ian Jackson > Acked-by: Ian Campbell > Reviewed-by: Konrad Rzeszutek Wilk > > v2: Fix coding style, in one "if" statement. > --- > tools/xcutils/readnotes.c | 10 +++++++--- > xen/common/libelf/libelf-dominfo.c | 13 ++++++++++--- > xen/common/libelf/libelf-tools.c | 10 +++++++--- > xen/include/xen/libelf.h | 7 +++++-- > 4 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c > index 7ff2530..ca86ba5 100644 > --- a/tools/xcutils/readnotes.c > +++ b/tools/xcutils/readnotes.c > @@ -63,7 +63,7 @@ struct setup_header { > static void print_string_note(const char *prefix, struct elf_binary *elf, > ELF_HANDLE_DECL(elf_note) note) > { > - printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note)); > + printf("%s: %s\n", prefix, elf_strfmt(elf, elf_note_desc(elf, note))); > } > > static void print_numeric_note(const char *prefix, struct elf_binary *elf, > @@ -103,10 +103,13 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, > { > ELF_HANDLE_DECL(elf_note) note; > int notes_found = 0; > + const char *this_note_name; > > for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) ) > { > - if (0 != strcmp(elf_note_name(elf, note), "Xen")) > + this_note_name = elf_note_name(elf, note); > + if (NULL == this_note_name || > + 0 != strcmp(this_note_name, "Xen")) > continue; Consistency? At various places in this patch, you differ between a single if statement and two if statements to do this new extra checking for NULL. The most concise would be if ( !ptr || !strcmp(ptr, "Xen") ) but it could certainly be argued that that is perhaps too concise. ~Andrew > > notes_found++; > @@ -294,7 +297,8 @@ int main(int argc, char **argv) > > shdr = elf_shdr_by_name(&elf, "__xen_guest"); > if (ELF_HANDLE_VALID(shdr)) > - printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, shdr)); > + printf("__xen_guest: %s\n", > + elf_strfmt(&elf, elf_section_start(&elf, shdr))); > > return 0; > } > diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c > index 566f6f9..ba0dc83 100644 > --- a/xen/common/libelf/libelf-dominfo.c > +++ b/xen/common/libelf/libelf-dominfo.c > @@ -137,7 +137,10 @@ int elf_xen_parse_note(struct elf_binary *elf, > > if ( note_desc[type].str ) > { > - str = elf_note_desc(elf, note); > + str = elf_strval(elf, elf_note_desc(elf, note)); > + if (str == NULL) > + /* elf_strval will mark elf broken if it fails so no need to log */ > + return 0; > elf_msg(elf, "%s: %s = \"%s\"\n", __FUNCTION__, > note_desc[type].name, str); > parms->elf_notes[type].type = XEN_ENT_STR; > @@ -220,6 +223,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf, > { > int xen_elfnotes = 0; > ELF_HANDLE_DECL(elf_note) note; > + const char *note_name; > > parms->elf_note_start = start; > parms->elf_note_end = end; > @@ -227,7 +231,10 @@ static int elf_xen_parse_notes(struct elf_binary *elf, > ELF_HANDLE_PTRVAL(note) < parms->elf_note_end; > note = elf_note_next(elf, note) ) > { > - if ( strcmp(elf_note_name(elf, note), "Xen") ) > + note_name = elf_note_name(elf, note); > + if ( note_name == NULL ) > + continue; > + if ( strcmp(note_name, "Xen") ) > continue; > if ( elf_xen_parse_note(elf, parms, note) ) > return -1; > @@ -541,7 +548,7 @@ int elf_xen_parse(struct elf_binary *elf, > parms->elf_note_start = ELF_INVALID_PTRVAL; > parms->elf_note_end = ELF_INVALID_PTRVAL; > elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__, > - parms->guest_info); > + elf_strfmt(elf, parms->guest_info)); > elf_xen_parse_guest_info(elf, parms); > break; > } > diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c > index bf68bcd..fa7dedd 100644 > --- a/xen/common/libelf/libelf-tools.c > +++ b/xen/common/libelf/libelf-tools.c > @@ -119,7 +119,7 @@ const char *elf_section_name(struct elf_binary *elf, > if ( ELF_PTRVAL_INVALID(elf->sec_strtab) ) > return "unknown"; > > - return elf->sec_strtab + elf_uval(elf, shdr, sh_name); > + return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name)); > } > > ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) > @@ -151,6 +151,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym > ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab); > ELF_HANDLE_DECL(elf_sym) sym; > uint64_t info, name; > + const char *sym_name; > > for ( ; ptr < end; ptr += elf_size(elf, sym) ) > { > @@ -159,7 +160,10 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym > name = elf_uval(elf, sym, st_name); > if ( ELF32_ST_BIND(info) != STB_GLOBAL ) > continue; > - if ( strcmp(elf->sym_strtab + name, symbol) ) > + sym_name = elf_strval(elf, elf->sym_strtab + name); > + if ( sym_name == NULL ) /* out of range, oops */ > + return ELF_INVALID_HANDLE(elf_sym); > + if ( strcmp(sym_name, symbol) ) > continue; > return sym; > } > @@ -177,7 +181,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index) > > const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) > { > - return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note); > + return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note)); > } > > ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) > diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h > index dc9b5ae..0c2c11e 100644 > --- a/xen/include/xen/libelf.h > +++ b/xen/include/xen/libelf.h > @@ -252,6 +252,9 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr, > uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr); > > > +#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the future */ > +#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) instead */ > + > #define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz)) > #define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz)) > /* > @@ -279,7 +282,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n > ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index); > ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index); > > -const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); > +const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */ > ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); > ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); > > @@ -289,7 +292,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(el > ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol); > ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index); > > -const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); > +const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */ > ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); > uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); > uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),