>>> On 09.12.16 at 16:44, <ian.jack...@eu.citrix.com> 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@lists.xen.org https://lists.xen.org/xen-devel
