[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()
>>> On 08.12.16 at 18:28, <ian.jackson@xxxxxxxxxxxxx> wrote: > Part of the motivation for a01b6d4 was the anomaly which is the > difference between the implementations of elf_phdr_size and > elf_shdr_size. > > This was introduced in 39bf7b9d0ae5 "libelf: check loops for running > away". That was part of the XSA-55 patchset. > > Looking at the commit message I was concerned that the phdr and shdr > counts might be excessive, and that libelf might get stuck in a loop > with an unreasonably high iteration count. Looking at that commit message, what I'm missing first of all is an explanation of why long loops are a problem in the first place. Do we need to be concerned of single threaded tool stacks? I think such a problem would better be addressed at higher layers, switching to multi threaded designs. > I appear to have attempted to mitigate this by: > > (a) in the case of shdrs, by using elf_access_ok inside each > loop, on the principle that an out of control loop count > would walk off the end of the image and stop; > > (b) in the case of phdrs, making an explicit limit of > (image size) / sizeof(phdr). NB that sizeof(phdr) is not the > actual size of phdrs; for a valid ELF it is a minimum. This > is fine because we're computing a backstop, which does not > have to be entirely accurate. > > I now see that (a) is ineffective because the image can specify its > own stride for the iteration (the header size). If it specifies a > stride of zero, the maximum count is unbounded. Well, a stride of zero is invalid (as is any smaller than the base ELF spec's entry structure size). > However, both count values are actually 16-bit. So there is already a > limit. The extra code in elf_phdr_size is not necessary. I'll make v2 of the patch then simply drop it. > Another part of the motivation for a01b6d4 is to produce better > messages for certain malformed ELFs. There has been no assertion that > any such ELFs (ie ELFs which would fail these new checks) actually > exist or have caused trouble for anyone. > > I think that it is a bad idea to add unnecessary checking to libelf. > > libelf is a format parser on a security boundary. Despite attempts to > provide a safe dialect to write in, every piece of code in libelf > risks security problems. Any crash (e.g. as a result of a signal) of course is a problem also for a multi threaded tool stack. But beyond that I'm not sure I understand why you say "every piece". That's perhaps partly because ... > Also, this has demonstrated that the design principles underlying the > safety of libelf are not properly understood. IIRC they were > explained in the XSA-55 00/ patch, but that's not sufficient. ... this explanation has not been recorded anywhere afaics, not even in xsa.git. > So I would prefer to: > > Revert all of a01b6d4. That was already done yesterday. > Delete the header count check in elf_phdr_count and replace it with a > compile-time assertion, in elf_phdr_count and in elf_shdr_count, that > sizeof the count field is <= 2. (This may need a new macro > ELF_HANDLE_FIELD_SIZEOF implemented in terms of > ELF__HANDLE_FIELD_TYPE.) I'll wait with adding such an assertion until we've clarified the point near the top of this reply. > Add a comment near the top of libelf.h explaining the rules for > programming inside libelf, to include: > > * Arithmetic on signed values is forbidden > > * Use of actual pointers into the ELF is forbidden > > * Division by non-constant values is forbidden > > * All loops must ensure that they have a reasonably low > iteration count > > * Code (including ELF format sanity checks) which is necessary > neither for safety nor for correct processing of correct files > should not be added to libelf. It is not libelf's role to > be an ELF validator. For all these points, I'd like it to also be clarified why those are being established. I'd appreciate if you could submit a respective patch based on the 00/ patch you refer to above, which I understand you still have available in your mailbox. For this last point - "ELF validator" is certainly the goal. But I think it is reasonable for a library to at least check the input it makes use of. If I had meant to fully validate the ELF image, I would e.g. have had to reject images with zero e_phnum but non-zero other e_ph* fields. Yet we don't care about those other fields when there are no program headers (which, as pointed out on IRC, is useless anyway, as then there's nothing to load; only e_shnum can sensibly be zero). Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |