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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Ian Jackson writes ("Re: [PATCH] libelf: Fix div0 issues in 
elf_{shdr,phdr}_count()"):
> Admittedly the rule about iteration count is not so easy to remember,
> and I had failed to anticipate that someone would introduce division.
> But both of those kind of bugs are denial of service, rather than
> privilege escalation.
> 
> Having stared at the commit message of a01b6d46 and at the
> pre-a01b6d46 code, I still don't understand what motivated the
> changes.

Jan and I had a conversation on irc.


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.

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.

However, both count values are actually 16-bit.  So there is already a
limit.  The extra code in elf_phdr_size is not necessary.

IMO this near-miss demonstrates that a more robust approach is
required to bounding loops in libelf.

A possibility would be to introduce
  bool elf_iter_cont(struct elf_binary *elf, size_t copysz);
and using it in every for(;;) in libelf.  It would keep a counter (set
to zero in libelf) and abandon the processing if the total of all the
copysz values passed was "too large".  (copysz would be bounded below
by 1, so that it is safe to pass a size from the ELF.)

Then

-     for ( i = 0; i < elf_shdr_count(elf); i++ )
+     for ( i = 0; elf_iter_cont(elf, e_shentsize)
+          && i < elf_shdr_count(elf); i++ )

An alternative would be to decorate every loop with a comment
explaining why the loop does reasonably-bounded work.  I'm not sure
whether this, or my more sophisticated suggestion, will find favour.

In any case, the max calculation in elf_phdr_size is unnecessary for
security, and has no purpose related to functionality, and can be
deleted.


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.

If libelf "accepts" some malformed image, and constructs a mess in the
guest memory space, then that is unfortunate.  But it is not a big
problem.  The guest will probably crash or go wrong, but that is not a
problem for the host.

If someone is faced with fallout from a malformed ELF, they would
hopefully try use an object file inspector like objdump on the loaded
image.  That would reveal the problem.  (And of course ELFs are mostly
generated by a fairly small ecosystem of toolchain programs.)


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.


So I would prefer to:


Revert all of a01b6d4.


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


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.


Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel