Re: [Xen-devel] [PATCH 8/8] libelf: safety: Document safety principles in header file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.16 at 16:44, <ian.jackson@xxxxxxxxxxxxx> wrote:
> +/*
> + * DESIGN PRINCIPLES FOR THE SAFETY OF LIBELF
> + *
> + * libelf is a complex piece of code on a security boundary: when
> + * built as part of the tools, it parses guest kernels and loads them
> + * into guest memory.  Bugs in libelf can become privilege escalation
> + * or denial of service bugs in the toolstack.
> + *
> + * We try to reduce the risk of such bugs by writing the actual format
> + * parsing in a mostly-safe subset of C.  To avoid nonlocal exits or
> + * the need for explicit error-checking code, we make all references
> + * into the input image, or into guest memory, via an inherently safe
> + * wrapper system.
> + *
> + * This means that it is safe to simply honour the instructions from
> + * the image, even if they are nonsense.  If the image implies wild
> + * pointer accesses, these will be harmlessly defused; a note will be
> + * made that things are broken; and processing can safely continue
> + * despite some of the operations having not been done.  Eventually
> + * the error will be reported.
> + *
> + *
> + * To preserve these safety properties, there are some rules that
> + * programmers editing libelf need to follow:
> + *
> + *  - Any loop needs to be accompanied by calls to elf_iter_ok (or
> + *    elf_iter_ok_counted).
> + *
> + *        Rationale: the image must not be able to cause libelf to do
> + *        unbounded work (ie, get stuck in a loop).

As expressed before, I'm not convinced library code should be
concerned about caller restrictions.

> + *  - The input image and output area must be accessed only via the
> + *    safe pointer access system.  Real pointers into the input or
> + *    output may not be even *calculated*.
> + *
> + *        Rationale: calculating wild pointers is undefined behaviour;
> + *        if the compiler sees that you might be calculating wild
> + *        pointers, it may remove important checks!
> + *
> + *  - Stack local buffer variables containing information derived from
> + *    the image (including structs, or byte buffers) must be
> + *    completely zeroed, using elf_memset_unchecked (and an
> + *    accompanying elf_iter_ok_counted) on entry to the function (or
> + *    somewhere very obviously near there).
> + *
> + *        Rationale: This avoids uninitialised stack data being used
> + *        as input to any of the loader.

What distinguishes a "buffer variable" from a plain variable? I.e.
where is the boundary here as to which ones need zeroing? Also,
I don't think zeroing is needed when a variable gets fully written
over (like in a structure assignment). Do you perhaps want to
limit the above to "directly derived from the image"?

> + *  - All integer variables should be unsigned.
> + *
> + *        Rationale: this avoids signed integer overflow (which has
> + *        undefined behaviour in C, and if spotted by the compiler can
> + *        cause it to generate bad code).

There are certainly cases where one explicitly needs negative
values, and hence signed integer types. Therefore I don't think
we can outright exclude this. Perhaps limit by "... variables with
non-negative value range ..."?

> + *  - Arithmetic operations other than + - * should be avoided; in
> + *    particular, division (/ or %) by non-constant values should be
> + *    avoided.  If it cannot be avoided, the divisor must be checked
> + *    for zero.
> + *
> + *        Rationale: We must avoid division-by-zero (or other overflow
> + *        traps).
> + *
> + *  - If it is desirable to breach these rules, there should be a
> + *    comment explaining why this is OK.
> + *
> + * Even so, this is a fairly high-risk environment, so:
> + *
> + *  - Do not add code which is not necessary for libelf to function
> + *    with correct input, or to avoid being vulnerable to incorrect
> + *    input.  Do not add additional functionally-unnecessary checks
> + *    for diagnosing problems with the image, or validating sanity of
> + *    the input ELF.
> + *
> + *        Rationale: Redundant checks have 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.

While I can understand your reasoning, I continue to think that
libelf in a ELF loader, and hence should check certain things. The
best example I currently know of are e_[ps]hentsize, which the
code uses without checking the lower valid bound.

Overall I'm certainly not meaning to veto any of this, but I think it
would be better for some other REST maintainer (agreeing with
your way of thinking) to ack this.

Jan

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