Re: [Xen-devel] [PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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