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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
---
 xen/include/xen/libelf.h | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 6436bd7..8b75242 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -60,6 +60,96 @@ typedef void elf_log_callback(struct elf_binary*, void 
*caller_data,
 
 /* ------------------------------------------------------------------------ */
 
+/*
+ * 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).
+ *
+ *  - 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.
+ *
+ *  - 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).
+ *
+ *  - 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.
+ *
+ *  - However, it is OK to have checks code which provide duplicate
+ *    defence against certain hostile images, if it is not otherwise
+ *    obvious how libelf would be defended against such images.
+ *
+ *        Rationale: Redundant checks where the situation would
+ *        otherwise not be quite clear mean that the safety of the
+ *        code is easy to see throughout; so that any unsafe code
+ *        would be more obvious.
+ */
+
 /* Macros for accessing the input image and output area. */
 
 /*
@@ -475,6 +565,8 @@ static inline void *elf_memset_unchecked(void *s, int c, 
size_t n)
    * pointers.  These are just like the real functions.
    * We provide these so that in libelf-private.h we can #define
    * memcpy, memset and memmove to undefined MISTAKE things.
+   *
+   * Remember to call elf_iter_ok_counted nearby.
    */
 
 static inline int elf_strcmp_safe(struct elf_binary *elf,
-- 
2.1.4


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