Jan Beulich writes ("Re: [PATCH] libelf: Fix div0 issues in
elf_{shdr,phdr}_count()"):
> On 08.12.16 at 18:28, <ian.jack...@eu.citrix.com> wrote:
> > 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.
libxl is (essentially) singlethreaded. It is very hard to write
correct multithreaded programs and I would be adamantly opposed to
anything that made libxl more thready inside. Also, even with
threading, a long-running domain setup would make it difficult to kill
the domain, unless we have asynchronous termination of such a thread,
which would be a nightmare to implement correctly.
> > 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).
That something is invalid does not mean it cannot be specified.
> > 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 ...
All code risks bugs.
The programming environment in which libelf code is written is by far
from guaranteed safe. Safety relies on programmers who write the code
following the rules. The most common kinds of mistake are rendered
harmless, but other kinds of mistake are possible.
> > 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.
I will do this.
> 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).
(I think you omitted "not", so you meant "is certainly not the goal.)
IMO libelf should do enough tests to function correctly with correct
input, and avoid being vulnerable to incorrect input. Code to perform
other tests introduces risk (of bugs, including security bugs such as
the one introduced in a01b6d4).
Such code has almost zero benefit because 1. we do not expect
ELF-generating tools to generate invalid ELFs so these checks' failure
paths will very likely never be executed anywhere, and 2. anyone
debugging such a hypothetical bad ELF will have a variety of tools
available which will do a much better job of analysing it.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
