Re: [Qemu-devel] [PATCH v3 1/2] docs: add Secure Coding Practices to developer docs

[Li Qiang]

Stefan Hajnoczi <address@hidden> 于2019年5月9日周四 下午8:20写道：

> At KVM Forum 2018 I gave a presentation on security in QEMU:
> https://www.youtube.com/watch?v=YAdRf_hwxU8 (video)
> https://vmsplice.net/~stefan/stefanha-kvm-forum-2018.pdf (slides)
>
> This patch adds a guide to secure coding practices.  This document
> covers things that developers should know about security in QEMU.  It is
> just a starting point that we can expand on later.  I hope it will be
> useful as a resource for new contributors and will save code reviewers
> from explaining the same concepts many times.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> Acked-by: Stefano Garzarella <address@hidden>
> Reviewed-by: Alex Bennée <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>

Reviewed-by: Li Qiang <address@hidden>




> ---
>  docs/devel/index.rst                   |   1 +
>  docs/devel/secure-coding-practices.rst | 106 +++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
>  create mode 100644 docs/devel/secure-coding-practices.rst
>
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index ebbab636ce..2a4ddf40ad 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -20,3 +20,4 @@ Contents:
>     stable-process
>     testing
>     decodetree
> +   secure-coding-practices
> diff --git a/docs/devel/secure-coding-practices.rst
> b/docs/devel/secure-coding-practices.rst
> new file mode 100644
> index 0000000000..cbfc8af67e
> --- /dev/null
> +++ b/docs/devel/secure-coding-practices.rst
> @@ -0,0 +1,106 @@
> +=======================
> +Secure Coding Practices
> +=======================
> +This document covers topics that both developers and security researchers
> must
> +be aware of so that they can develop safe code and audit existing code
> +properly.
> +
> +Reporting Security Bugs
> +-----------------------
> +For details on how to report security bugs or ask questions about
> potential
> +security bugs, see the `Security Process wiki page
> +<https://wiki.qemu.org/SecurityProcess>`_.
> +
> +General Secure C Coding Practices
> +---------------------------------
> +Most CVEs (security bugs) reported against QEMU are not specific to
> +virtualization or emulation.  They are simply C programming bugs.
> Therefore
> +it's critical to be aware of common classes of security bugs.
> +
> +There is a wide selection of resources available covering secure C
> coding.  For
> +example, the `CERT C Coding Standard
> +<https://wiki.sei.cmu.edu/confluence/display/c/SEI+CERT+C+Coding+Standard
> >`_
> +covers the most important classes of security bugs.
> +
> +Instead of describing them in detail here, only the names of the most
> important
> +classes of security bugs are mentioned:
> +
> +* Buffer overflows
> +* Use-after-free and double-free
> +* Integer overflows
> +* Format string vulnerabilities
> +
> +Some of these classes of bugs can be detected by analyzers.  Static
> analysis is
> +performed regularly by Coverity and the most obvious of these bugs are
> even
> +reported by compilers.  Dynamic analysis is possible with valgrind, tsan,
> and
> +asan.
> +
> +Input Validation
> +----------------
> +Inputs from the guest or external sources (e.g. network, files) cannot be
> +trusted and may be invalid.  Inputs must be checked before using them in
> a way
> +that could crash the program, expose host memory to the guest, or
> otherwise be
> +exploitable by an attacker.
> +
> +The most sensitive attack surface is device emulation.  All hardware
> register
> +accesses and data read from guest memory must be validated.  A typical
> example
> +is a device that contains multiple units that are selectable by the guest
> via
> +an index register::
> +
> +  typedef struct {
> +      ProcessingUnit unit[2];
> +      ...
> +  } MyDeviceState;
> +
> +  static void mydev_writel(void *opaque, uint32_t addr, uint32_t val)
> +  {
> +      MyDeviceState *mydev = opaque;
> +      ProcessingUnit *unit;
> +
> +      switch (addr) {
> +      case MYDEV_SELECT_UNIT:
> +          unit = &mydev->unit[val];   <-- this input wasn't validated!
> +          ...
> +      }
> +  }
> +
> +If ``val`` is not in range [0, 1] then an out-of-bounds memory access
> will take
> +place when ``unit`` is dereferenced.  The code must check that ``val`` is
> 0 or
> +1 and handle the case where it is invalid.
> +
> +Unexpected Device Accesses
> +--------------------------
> +The guest may access device registers in unusual orders or at unexpected
> +moments.  Device emulation code must not assume that the guest follows the
> +typical "theory of operation" presented in driver writer manuals.  The
> guest
> +may make nonsense accesses to device registers such as starting operations
> +before the device has been fully initialized.
> +
> +A related issue is that device emulation code must be prepared for
> unexpected
> +device register accesses while asynchronous operations are in progress.  A
> +well-behaved guest might wait for a completion interrupt before accessing
> +certain device registers.  Device emulation code must handle the case
> where the
> +guest overwrites registers or submits further requests before an ongoing
> +request completes.  Unexpected accesses must not cause memory corruption
> or
> +leaks in QEMU.
> +
> +Invalid device register accesses can be reported with
> +``qemu_log_mask(LOG_GUEST_ERROR, ...)``.  The ``-d guest_errors``
> command-line
> +option enables these log messages.
> +
> +Live Migration
> +--------------
> +Device state can be saved to disk image files and shared with other users.
> +Live migration code must validate inputs when loading device state so an
> +attacker cannot gain control by crafting invalid device states.  Device
> state
> +is therefore considered untrusted even though it is typically generated
> by QEMU
> +itself.
> +
> +Guest Memory Access Races
> +-------------------------
> +Guests with multiple vCPUs may modify guest RAM while device emulation
> code is
> +running.  Device emulation code must copy in descriptors and other guest
> RAM
> +structures and only process the local copy.  This prevents
> +time-of-check-to-time-of-use (TOCTOU) race conditions that could cause
> QEMU to
> +crash when a vCPU thread modifies guest RAM while device emulation is
> +processing it.
> --
> 2.21.0
>
>
>