[PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Daniel P. Berrangé]

The -enable-fips option was added a long time ago to prevent the use of
single DES when VNC when FIPS mode is enabled. It should never have been
added, because apps are supposed to unconditionally honour FIPS mode
based on the '/proc/sys/crypto/fips_enabled' file contents.

In addition there is more to achieving FIPS compliance than merely
blocking use of certain algorithms. Those algorithms which are used
need to perform self-tests at runtime.

QEMU's built-in cryptography provider has no support for self-tests,
and neither does the nettle library.

If QEMU is required to be used in a FIPS enabled host, then it must be
built with the libgcrypt library enabled, which will unconditionally
enforce FIPS compliance in any algorithm usage.

Thus there is no need to keep either the -enable-fips option in QEMU, or
QEMU's internal FIPS checking methods.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/system/deprecated.rst | 11 +++++++++++
 os-posix.c                 |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 905628f3a0..faa2bc49b1 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -158,6 +158,17 @@ devices.  It is possible to use drives the board doesn't pick up with
 -device.  This usage is now deprecated.  Use ``if=none`` instead.
 
 
+``--enable-fips`` (since 5.2)
+
+This option restricts usage of certain cryptographic algorithms when
+the host is operating in FIPS mode.
+
+If FIPS compliance is required, QEMU should be built with the ``libgcrypt``
+library enabled as a cryptography provider.
+
+Neither the ``nettle`` library, or the built-in cryptography provider are
+supported on FIPS enabled hosts.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/os-posix.c b/os-posix.c
index 1de2839554..a6846f51c1 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -153,6 +153,9 @@ int os_parse_cmd_args(int index, const char *optarg)
         break;
 #if defined(CONFIG_LINUX)
     case QEMU_OPTION_enablefips:
+        warn_report("-enable-fips is deprecated, please build QEMU with "
+                    "the `libgcrypt` library as the cryptography provider "
+                    "to enable FIPS compliance");
         fips_set_state(true);
         break;
 #endif
-- 
2.26.2

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Paolo Bonzini]

On 20/10/20 18:22, Daniel P. Berrangé wrote:
> @@ -153,6 +153,9 @@ int os_parse_cmd_args(int index, const char *optarg)
>          break;
>  #if defined(CONFIG_LINUX)
>      case QEMU_OPTION_enablefips:
> +        warn_report("-enable-fips is deprecated, please build QEMU with "
> +                    "the `libgcrypt` library as the cryptography provider "
> +                    "to enable FIPS compliance");
>          fips_set_state(true);
>          break;
>  #endif

Should you also remove fips_set_state(true) and make fips_get_state()
return the contents of /proc/sys/crypto/fips_enabled, so that VNC
password authentication is disabled?

Paolo

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Daniel P. Berrangé]

On Tue, Oct 20, 2020 at 07:22:03PM +0200, Paolo Bonzini wrote:
> On 20/10/20 18:22, Daniel P. Berrangé wrote:
> > @@ -153,6 +153,9 @@ int os_parse_cmd_args(int index, const char *optarg)
> >          break;
> >  #if defined(CONFIG_LINUX)
> >      case QEMU_OPTION_enablefips:
> > +        warn_report("-enable-fips is deprecated, please build QEMU with "
> > +                    "the `libgcrypt` library as the cryptography provider "
> > +                    "to enable FIPS compliance");
> >          fips_set_state(true);
> >          break;
> >  #endif
> 
> Should you also remove fips_set_state(true) and make fips_get_state()
> return the contents of /proc/sys/crypto/fips_enabled, so that VNC
> password authentication is disabled?

I did think about doing that, but decided that since my intention is
to delete all trace of fips_get_state / fips_set_state at the end of
the deprecation period, that it'd be saner just to leave the semantics
unchanged during the deprecation period.

Deprecation notices shouldn't really be associated with changes in
functionality at time they are introduced.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Paolo Bonzini]

On 21/10/20 10:38, Daniel P. Berrangé wrote:
> On Tue, Oct 20, 2020 at 07:22:03PM +0200, Paolo Bonzini wrote:
>> On 20/10/20 18:22, Daniel P. Berrangé wrote:
>>> @@ -153,6 +153,9 @@ int os_parse_cmd_args(int index, const char *optarg)
>>>          break;
>>>  #if defined(CONFIG_LINUX)
>>>      case QEMU_OPTION_enablefips:
>>> +        warn_report("-enable-fips is deprecated, please build QEMU with "
>>> +                    "the `libgcrypt` library as the cryptography provider "
>>> +                    "to enable FIPS compliance");
>>>          fips_set_state(true);
>>>          break;
>>>  #endif
>>
>> Should you also remove fips_set_state(true) and make fips_get_state()
>> return the contents of /proc/sys/crypto/fips_enabled, so that VNC
>> password authentication is disabled?
> 
> I did think about doing that, but decided that since my intention is
> to delete all trace of fips_get_state / fips_set_state at the end of
> the deprecation period, that it'd be saner just to leave the semantics
> unchanged during the deprecation period.

But would it be correct?  In order to have the advertised behavior of
"enable FIPS compliance just with procfs, no need to do anything in
QEMU" you need to disable VNC password authentication; so while
fips_set_state is an abomination, fips_get_state should remain.

> Deprecation notices shouldn't really be associated with changes in
> functionality at time they are introduced.

I think you can consider it a bugfix since no one sets fips_enabled
without knowing what they're doing.

Paolo

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Daniel P. Berrangé]

On Wed, Oct 21, 2020 at 11:51:10AM +0200, Paolo Bonzini wrote:
> On 21/10/20 10:38, Daniel P. Berrangé wrote:
> > On Tue, Oct 20, 2020 at 07:22:03PM +0200, Paolo Bonzini wrote:
> >> On 20/10/20 18:22, Daniel P. Berrangé wrote:
> >>> @@ -153,6 +153,9 @@ int os_parse_cmd_args(int index, const char *optarg)
> >>>          break;
> >>>  #if defined(CONFIG_LINUX)
> >>>      case QEMU_OPTION_enablefips:
> >>> +        warn_report("-enable-fips is deprecated, please build QEMU with "
> >>> +                    "the `libgcrypt` library as the cryptography provider "
> >>> +                    "to enable FIPS compliance");
> >>>          fips_set_state(true);
> >>>          break;
> >>>  #endif
> >>
> >> Should you also remove fips_set_state(true) and make fips_get_state()
> >> return the contents of /proc/sys/crypto/fips_enabled, so that VNC
> >> password authentication is disabled?
> > 
> > I did think about doing that, but decided that since my intention is
> > to delete all trace of fips_get_state / fips_set_state at the end of
> > the deprecation period, that it'd be saner just to leave the semantics
> > unchanged during the deprecation period.
> 
> But would it be correct?  In order to have the advertised behavior of
> "enable FIPS compliance just with procfs, no need to do anything in
> QEMU" you need to disable VNC password authentication; so while
> fips_set_state is an abomination, fips_get_state should remain.

There's no need for fips_get_state. Once you build QEMU with
libgcrypt, when  VNC requests a DES cipher handle, gcrypt will
return an error as that algorithm is forbidden in FIPS mode.

This is the primary reason for outsourcing all crypto to a
separate library and ignoring the impls in QEMU.

Claiming QEMU is FIPS compliant without using libgcrypt is a
bit of joke since we don't do any self-tests of ciphers, hence
this deprecation notice is warning people that libgcrypt is
going to be mandatory if you care about FIPS.


> > Deprecation notices shouldn't really be associated with changes in
> > functionality at time they are introduced.
> 
> I think you can consider it a bugfix since no one sets fips_enabled
> without knowing what they're doing.

I just would rather not change semantics of something that we are
intending to remove 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Paolo Bonzini]

On 21/10/20 12:17, Daniel P. Berrangé wrote:
>> But would it be correct?  In order to have the advertised behavior of
>> "enable FIPS compliance just with procfs, no need to do anything in
>> QEMU" you need to disable VNC password authentication; so while
>> fips_set_state is an abomination, fips_get_state should remain.
> There's no need for fips_get_state. Once you build QEMU with
> libgcrypt, when  VNC requests a DES cipher handle, gcrypt will
> return an error as that algorithm is forbidden in FIPS mode.

Oh, I thought we were still using our own code for the modified DES but
it _is_ actually using gcrypt or nettle if available.  Sorry for the noise.

> This is the primary reason for outsourcing all crypto to a
> separate library and ignoring the impls in QEMU.
> 
> Claiming QEMU is FIPS compliant without using libgcrypt is a
> bit of joke since we don't do any self-tests of ciphers, hence
> this deprecation notice is warning people that libgcrypt is
> going to be mandatory if you care about FIPS.

Yes, agreed.

Paolo

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[John Snow]

On 10/21/20 6:17 AM, Daniel P. Berrangé wrote:
> Claiming QEMU is FIPS compliant without using libgcrypt is a
> bit of joke since we don't do any self-tests of ciphers, hence
> this deprecation notice is warning people that libgcrypt is
> going to be mandatory if you care about FIPS.
> 

FWIW this is my main problem with this flag: we read the value in procfs 
and then use this to change precisely one behavior for one of our 
components. It doesn't really ... do what the name might imply it does.

Leaving that business to the crypto libraries is indeed the correct 
thing to do.

So:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement

[Thomas Huth]

On 20/10/2020 18.22, Daniel P. Berrangé wrote:
> The -enable-fips option was added a long time ago to prevent the use of
> single DES when VNC when FIPS mode is enabled. It should never have been
> added, because apps are supposed to unconditionally honour FIPS mode
> based on the '/proc/sys/crypto/fips_enabled' file contents.
> 
> In addition there is more to achieving FIPS compliance than merely
> blocking use of certain algorithms. Those algorithms which are used
> need to perform self-tests at runtime.
> 
> QEMU's built-in cryptography provider has no support for self-tests,
> and neither does the nettle library.
> 
> If QEMU is required to be used in a FIPS enabled host, then it must be
> built with the libgcrypt library enabled, which will unconditionally
> enforce FIPS compliance in any algorithm usage.
> 
> Thus there is no need to keep either the -enable-fips option in QEMU, or
> QEMU's internal FIPS checking methods.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/system/deprecated.rst | 11 +++++++++++
>  os-posix.c                 |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 905628f3a0..faa2bc49b1 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -158,6 +158,17 @@ devices.  It is possible to use drives the board doesn't pick up with
>  -device.  This usage is now deprecated.  Use ``if=none`` instead.
>  
>  

Nit: The two empty lines should be below the new entry (i.e. before the
"QMP" title below), not before it.

> +``--enable-fips`` (since 5.2)
> +
> +This option restricts usage of certain cryptographic algorithms when
> +the host is operating in FIPS mode.
> +
> +If FIPS compliance is required, QEMU should be built with the ``libgcrypt``
> +library enabled as a cryptography provider.
> +
> +Neither the ``nettle`` library, or the built-in cryptography provider are
> +supported on FIPS enabled hosts.
> +
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
> diff --git a/os-posix.c b/os-posix.c
> index 1de2839554..a6846f51c1 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -153,6 +153,9 @@ int os_parse_cmd_args(int index, const char *optarg)
>          break;
>  #if defined(CONFIG_LINUX)
>      case QEMU_OPTION_enablefips:
> +        warn_report("-enable-fips is deprecated, please build QEMU with "
> +                    "the `libgcrypt` library as the cryptography provider "
> +                    "to enable FIPS compliance");
>          fips_set_state(true);
>          break;
>  #endif

With the nit fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>