Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

From: P J P <pjp@fedoraproject.org>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"Thomas Huth" <thuth@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Eric Blake" <eblake@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
Date: Tue, 14 Sep 2021 13:30:27 +0000 (UTC)	[thread overview]
Message-ID: <798304472.4432617.1631626227208@mail.yahoo.com> (raw)
In-Reply-To: <YTnhy/Wo5M1qVs1k@redhat.com>

Hello Philippe, all

>On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé <berrange@redhat.com> wrote:
>On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
>> This series is experimental! The goal is to better limit the
>> boundary of what code is considerated security critical, and
>> what is less critical (but still important!).
>>
>> This approach was quickly discussed few months ago with Markus
>> then Daniel. Instead of classifying the code on a file path
>> basis (see [1]), we insert (runtime) hints into the code
>> (which survive code movement). Offending unsafe code can
>> taint the global security policy. By default this policy
>> is 'none': the current behavior. It can be changed on the
>> command line to 'warn' to display warnings, and to 'strict'
>> to prohibit QEMU running with a tainted policy.
>

* Thanks so much for restarting this thread. I've been at it intermittently last few
  months, thinking about how could we annotate the source/module objects.

   -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html

* Last time we discussed about having both compile and run time options for users
  to be able to select the qualified objects/backends/devices as desired.

* To confirm: How/Where is the security policy defined? Is it device/module specific OR QEMU project wide?

> IOW, the reporting via QAPI introspetion is much more important
> for libvirt and mgmt apps, than any custom cli arg / printfs
> at the QEMU level.
>

* True, while it makes sense to have a solution that is conversant with
  the management/libvirt layers, It'll be great if we could address qemu/cli
  other use cases too.

>it feels like we need
>  'secure': 'bool'

* Though we started the (above [*]) discussion with '--security' option in mind,
  I wonder if 'secure' annotation is much specific. And if we could widen its scope.
--- x ---

Source annotations: I've been thinking over following approaches
===================

1) Segregate the QEMU sources under

      ../staging/      <= devel/experimental, not for production usage
      ../src/          <= good for production usage, hence security relevant
      ../deprecated/   <= Bad for production usage, not security relevant 

   - This is similar to Linux staging drivers' tree.
   - Staging drivers are not considered for production usage and hence CVE allocation.
   - At build time by default we only build sources under ../src/ tree.
   - But we can still have options to build /staging/ and /deprecated/ trees.  
   - It's readily understandable to end users.

2) pkgconfig(1) way:
   - If we could define per device/backend a configuration (.pc) file which is then used
     at build/run time to decide which sources are suitable for the build or usage.

   - I'm trying to experiment with this.

3) We annotate QEMU devices/backends/modules with macros which define its status.
   It can then be used at build/run times to decide if it's suitable for usage.
   For ex:

    $ cat annotsrc.h

    #include <inttypes.h>

    enum SRCSTATUS {
        DEVEL = 0,
        STAGING,
        PRODUCTION,
        DEPRECATED
    };

    uint8_t get_src_status(void);
    ===

    $ cat libx.c

    #include <annotsrc.h>

    #define SRC_STATUS DEPRECATED

    uint8_t
    get_src_status(void)
    {
        return SRC_STATUS;
    }
    ===

    $ cat testlibx.c

    #include <stdio.h>
    #include <annotsrc.h>

    int
    main (int argc, char *argv[])
    {
        printf("LibX status: %d\n", get_src_status());
        return 0;
    }
--- x ---

* Approach 3) above is similar to the _security_policy_taint() API,
  but works at the source/object file level, rather than specific 'struct type' field.

* Does adding a field to struct type (ex. DeviceClass) scale to all objects/modules/backends etc?
  Does it have any limitations to include/cover other sources/objects?

* I'd really appreciate any feedback/inputs/suggestions you may have.

Thank you.
---
  -P J P
http://feedmug.com