diff mbox

[v4] vnc: disable VNC password authentication (security type 2) when in FIPS mode

Message ID 20120608213809.24584.63909.stgit@sifl
State New
Headers show

Commit Message

Paul Moore June 8, 2012, 9:38 p.m. UTC
FIPS 140-2 requires disabling certain ciphers, including DES, which is used
by VNC to obscure passwords when they are sent over the network.  The
solution for FIPS users is to disable the use of VNC password auth when the
host system is operating in FIPS mode.

This patch causes QEMU to emit a message to stderr when the host system is
running in FIPS mode and a VNC password was specified on the commend line.
If the system is not running in FIPS mode, or is running in FIPS mode but
VNC password authentication was not requested, QEMU operates normally.

Signed-off-by: Paul Moore <pmoore@redhat.com>

--
Changelog
* v4
- Removed the use of syslog
* v3
- Use fgetc() instead of fgets() in fips_enabled
- Only emit a syslog message if the caller tries to use VNC password auth
- Suggest alternative auth methods in the stderr notice
* v2
- Protected syslog with _WIN32
- Protected the guts of fips_enabled() with __linux__
- Converted fips_enabled() and the fips flag from int to bool
*v1
- Initial draft
---
 qemu-doc.texi |    8 +++++---
 ui/vnc.c      |   27 +++++++++++++++++++++++++++
 ui/vnc.h      |    1 +
 3 files changed, 33 insertions(+), 3 deletions(-)

Comments

Daniel P. Berrangé June 11, 2012, 9:37 a.m. UTC | #1
On Fri, Jun 08, 2012 at 05:38:12PM -0400, Paul Moore wrote:
> FIPS 140-2 requires disabling certain ciphers, including DES, which is used
> by VNC to obscure passwords when they are sent over the network.  The
> solution for FIPS users is to disable the use of VNC password auth when the
> host system is operating in FIPS mode.
> 
> This patch causes QEMU to emit a message to stderr when the host system is
> running in FIPS mode and a VNC password was specified on the commend line.
> If the system is not running in FIPS mode, or is running in FIPS mode but
> VNC password authentication was not requested, QEMU operates normally.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
Paul Moore July 31, 2012, 5:08 p.m. UTC | #2
On Friday, June 08, 2012 05:38:12 PM Paul Moore wrote:
> FIPS 140-2 requires disabling certain ciphers, including DES, which is used
> by VNC to obscure passwords when they are sent over the network.  The
> solution for FIPS users is to disable the use of VNC password auth when the
> host system is operating in FIPS mode.
> 
> This patch causes QEMU to emit a message to stderr when the host system is
> running in FIPS mode and a VNC password was specified on the commend line.
> If the system is not running in FIPS mode, or is running in FIPS mode but
> VNC password authentication was not requested, QEMU operates normally.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Hi Anthony,

Any word on this patch?  Other than Daniel Berrange's reviewed-by tag, the 
discussion of the v4 patch has been quiet and I think we addressed all the 
other remaining issues in the discussion attached to the v2 patch posting.

-Paul

> --
> Changelog
> * v4
> - Removed the use of syslog
> * v3
> - Use fgetc() instead of fgets() in fips_enabled
> - Only emit a syslog message if the caller tries to use VNC password auth
> - Suggest alternative auth methods in the stderr notice
> * v2
> - Protected syslog with _WIN32
> - Protected the guts of fips_enabled() with __linux__
> - Converted fips_enabled() and the fips flag from int to bool
> *v1
> - Initial draft
> ---
>  qemu-doc.texi |    8 +++++---
>  ui/vnc.c      |   27 +++++++++++++++++++++++++++
>  ui/vnc.h      |    1 +
>  3 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 0af0ff4..fe8d3df 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -1124,9 +1124,11 @@ the protocol limits passwords to 8 characters it
> should not be considered to provide high security. The password can be
> fairly easily brute-forced by a client making repeat connections. For this
> reason, a VNC server using password authentication should be restricted to
> only listen on the loopback interface -or UNIX domain sockets. Password
> authentication is requested with the @code{password} -option, and then once
> QEMU is running the password is set with the monitor. Until -the monitor is
> used to set the password all clients will be rejected. +or UNIX domain
> sockets. Password authentication is not supported when operating +in FIPS
> 140-2 compliance mode as it requires the use of the DES cipher. Password
> +authentication is requested with the @code{password} option, and then once
> QEMU +is running the password is set with the monitor. Until the monitor is
> used to +set the password all clients will be rejected.
> 
>  @example
>  qemu-system-i386 [...OPTIONS...] -vnc :1,password -monitor stdio
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 54bc5ad..4bd816d 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -48,6 +48,21 @@ static DisplayChangeListener *dcl;
>  static int vnc_cursor_define(VncState *vs);
>  static void vnc_release_modifiers(VncState *vs);
> 
> +static bool fips_enabled(void)
> +{
> +    bool enabled = false;
> +
> +#ifdef __linux__
> +    FILE *fds = fopen("/proc/sys/crypto/fips_enabled", "r");
> +    if (fds != NULL) {
> +        enabled = (fgetc(fds) == '1');
> +        fclose(fds);
> +    }
> +#endif /* __linux__ */
> +
> +    return enabled;
> +}
> +
>  static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
>  {
>  #ifdef _VNC_DEBUG
> @@ -2748,6 +2763,9 @@ void vnc_display_init(DisplayState *ds)
>      dcl->idle = 1;
>      vnc_display = vs;
> 
> +    vs->fips = fips_enabled();
> +    VNC_DEBUG("FIPS mode %s\n", (vs->fips ? "enabled" : "disabled"));
> +
>      vs->lsock = -1;
> 
>      vs->ds = ds;
> @@ -2896,6 +2914,15 @@ int vnc_display_open(DisplayState *ds, const char
> *display) while ((options = strchr(options, ','))) {
>          options++;
>          if (strncmp(options, "password", 8) == 0) {
> +            if (vs->fips) {
> +                fprintf(stderr,
> +                        "VNC password auth disabled due to FIPS mode, "
> +                        "consider using the VeNCrypt or SASL authentication
> " +                        "methods as an alternative\n");
> +                g_free(vs->display);
> +                vs->display = NULL;
> +                return -1;
> +            }
>              password = 1; /* Require password auth */
>          } else if (strncmp(options, "reverse", 7) == 0) {
>              reverse = 1;
> diff --git a/ui/vnc.h b/ui/vnc.h
> index a851ebd..d41631b 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -160,6 +160,7 @@ struct VncDisplay
>      char *display;
>      char *password;
>      time_t expires;
> +    bool fips;
>      int auth;
>      bool lossy;
>      bool non_adaptive;
Anthony Liguori July 31, 2012, 7:52 p.m. UTC | #3
Paul Moore <pmoore@redhat.com> writes:

> On Friday, June 08, 2012 05:38:12 PM Paul Moore wrote:
>> FIPS 140-2 requires disabling certain ciphers, including DES, which is used
>> by VNC to obscure passwords when they are sent over the network.  The
>> solution for FIPS users is to disable the use of VNC password auth when the
>> host system is operating in FIPS mode.
>> 
>> This patch causes QEMU to emit a message to stderr when the host system is
>> running in FIPS mode and a VNC password was specified on the commend line.
>> If the system is not running in FIPS mode, or is running in FIPS mode but
>> VNC password authentication was not requested, QEMU operates normally.
>> 
>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> Hi Anthony,
>
> Any word on this patch?  Other than Daniel Berrange's reviewed-by tag, the 
> discussion of the v4 patch has been quiet and I think we addressed all the 
> other remaining issues in the discussion attached to the v2 patch
> posting.

I asked for the specific language in FIPS mandating this.  I don't see
any other VNC server implementing a check like this.  I would rather do
this in a more user friendly fashion like make it a config file option
that a user can set while in fips mode.

Regards,

Anthony Liguori

>
> -Paul
>
>> --
>> Changelog
>> * v4
>> - Removed the use of syslog
>> * v3
>> - Use fgetc() instead of fgets() in fips_enabled
>> - Only emit a syslog message if the caller tries to use VNC password auth
>> - Suggest alternative auth methods in the stderr notice
>> * v2
>> - Protected syslog with _WIN32
>> - Protected the guts of fips_enabled() with __linux__
>> - Converted fips_enabled() and the fips flag from int to bool
>> *v1
>> - Initial draft
>> ---
>>  qemu-doc.texi |    8 +++++---
>>  ui/vnc.c      |   27 +++++++++++++++++++++++++++
>>  ui/vnc.h      |    1 +
>>  3 files changed, 33 insertions(+), 3 deletions(-)
>> 
>> diff --git a/qemu-doc.texi b/qemu-doc.texi
>> index 0af0ff4..fe8d3df 100644
>> --- a/qemu-doc.texi
>> +++ b/qemu-doc.texi
>> @@ -1124,9 +1124,11 @@ the protocol limits passwords to 8 characters it
>> should not be considered to provide high security. The password can be
>> fairly easily brute-forced by a client making repeat connections. For this
>> reason, a VNC server using password authentication should be restricted to
>> only listen on the loopback interface -or UNIX domain sockets. Password
>> authentication is requested with the @code{password} -option, and then once
>> QEMU is running the password is set with the monitor. Until -the monitor is
>> used to set the password all clients will be rejected. +or UNIX domain
>> sockets. Password authentication is not supported when operating +in FIPS
>> 140-2 compliance mode as it requires the use of the DES cipher. Password
>> +authentication is requested with the @code{password} option, and then once
>> QEMU +is running the password is set with the monitor. Until the monitor is
>> used to +set the password all clients will be rejected.
>> 
>>  @example
>>  qemu-system-i386 [...OPTIONS...] -vnc :1,password -monitor stdio
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 54bc5ad..4bd816d 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -48,6 +48,21 @@ static DisplayChangeListener *dcl;
>>  static int vnc_cursor_define(VncState *vs);
>>  static void vnc_release_modifiers(VncState *vs);
>> 
>> +static bool fips_enabled(void)
>> +{
>> +    bool enabled = false;
>> +
>> +#ifdef __linux__
>> +    FILE *fds = fopen("/proc/sys/crypto/fips_enabled", "r");
>> +    if (fds != NULL) {
>> +        enabled = (fgetc(fds) == '1');
>> +        fclose(fds);
>> +    }
>> +#endif /* __linux__ */
>> +
>> +    return enabled;
>> +}
>> +
>>  static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
>>  {
>>  #ifdef _VNC_DEBUG
>> @@ -2748,6 +2763,9 @@ void vnc_display_init(DisplayState *ds)
>>      dcl->idle = 1;
>>      vnc_display = vs;
>> 
>> +    vs->fips = fips_enabled();
>> +    VNC_DEBUG("FIPS mode %s\n", (vs->fips ? "enabled" : "disabled"));
>> +
>>      vs->lsock = -1;
>> 
>>      vs->ds = ds;
>> @@ -2896,6 +2914,15 @@ int vnc_display_open(DisplayState *ds, const char
>> *display) while ((options = strchr(options, ','))) {
>>          options++;
>>          if (strncmp(options, "password", 8) == 0) {
>> +            if (vs->fips) {
>> +                fprintf(stderr,
>> +                        "VNC password auth disabled due to FIPS mode, "
>> +                        "consider using the VeNCrypt or SASL authentication
>> " +                        "methods as an alternative\n");
>> +                g_free(vs->display);
>> +                vs->display = NULL;
>> +                return -1;
>> +            }
>>              password = 1; /* Require password auth */
>>          } else if (strncmp(options, "reverse", 7) == 0) {
>>              reverse = 1;
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index a851ebd..d41631b 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -160,6 +160,7 @@ struct VncDisplay
>>      char *display;
>>      char *password;
>>      time_t expires;
>> +    bool fips;
>>      int auth;
>>      bool lossy;
>>      bool non_adaptive;
> -- 
> paul moore
> security and virtualization @ redhat
Daniel P. Berrangé July 31, 2012, 8:12 p.m. UTC | #4
On Tue, Jul 31, 2012 at 02:52:07PM -0500, Anthony Liguori wrote:
> Paul Moore <pmoore@redhat.com> writes:
> 
> > On Friday, June 08, 2012 05:38:12 PM Paul Moore wrote:
> >> FIPS 140-2 requires disabling certain ciphers, including DES, which is used
> >> by VNC to obscure passwords when they are sent over the network.  The
> >> solution for FIPS users is to disable the use of VNC password auth when the
> >> host system is operating in FIPS mode.
> >> 
> >> This patch causes QEMU to emit a message to stderr when the host system is
> >> running in FIPS mode and a VNC password was specified on the commend line.
> >> If the system is not running in FIPS mode, or is running in FIPS mode but
> >> VNC password authentication was not requested, QEMU operates normally.
> >> 
> >> Signed-off-by: Paul Moore <pmoore@redhat.com>
> >
> > Hi Anthony,
> >
> > Any word on this patch?  Other than Daniel Berrange's reviewed-by tag, the 
> > discussion of the v4 patch has been quiet and I think we addressed all the 
> > other remaining issues in the discussion attached to the v2 patch
> > posting.
> 
> I asked for the specific language in FIPS mandating this.  I don't see
> any other VNC server implementing a check like this.  I would rather do
> this in a more user friendly fashion like make it a config file option
> that a user can set while in fips mode.

The FIPS standard doesn't refer to particular applications like VNC.
As Paul says earlier, FIP 140-2 requires that DES (and certain other
ciphers) not be used in any applications which are running in a FIPS
compliant environment. Since VNC auth uses DES, this auth scheme
cannot be permitted in a FIPS environment.

The reason no other VNC server does this is almost certainly because
none of their developers have ever tried to have their code work in
a FIPS environment, so I don't think that's a relevant comparison.

I'm not really sure what addding more configuration options gains
us here. The choice of auth mode is already configurable. This patch
is about ensuring that the user is not allowed to configure it, if
FIPS mode is in effect (as indicated by the kernels syfs tunable).
So in fact adding config params doesn't really address this.

The proposed patch is already very straightforward, is using the
official interface exposed by the upstream kernel to userspace &
has negligable maintenence burden IMHO.

Regards,
Daniel
Paul Moore July 31, 2012, 8:28 p.m. UTC | #5
On Tuesday, July 31, 2012 09:12:57 PM Daniel P. Berrange wrote:
> On Tue, Jul 31, 2012 at 02:52:07PM -0500, Anthony Liguori wrote:
> > Paul Moore <pmoore@redhat.com> writes:
> > > On Friday, June 08, 2012 05:38:12 PM Paul Moore wrote:
> > >> FIPS 140-2 requires disabling certain ciphers, including DES, which is
> > >> used
> > >> by VNC to obscure passwords when they are sent over the network.  The
> > >> solution for FIPS users is to disable the use of VNC password auth when
> > >> the
> > >> host system is operating in FIPS mode.
> > >> 
> > >> This patch causes QEMU to emit a message to stderr when the host system
> > >> is
> > >> running in FIPS mode and a VNC password was specified on the commend
> > >> line.
> > >> If the system is not running in FIPS mode, or is running in FIPS mode
> > >> but
> > >> VNC password authentication was not requested, QEMU operates normally.
> > >> 
> > >> Signed-off-by: Paul Moore <pmoore@redhat.com>
> > > 
> > > Hi Anthony,
> > > 
> > > Any word on this patch?  Other than Daniel Berrange's reviewed-by tag,
> > > the
> > > discussion of the v4 patch has been quiet and I think we addressed all
> > > the
> > > other remaining issues in the discussion attached to the v2 patch
> > > posting.
> > 
> > I asked for the specific language in FIPS mandating this.  I don't see
> > any other VNC server implementing a check like this.  I would rather do
> > this in a more user friendly fashion like make it a config file option
> > that a user can set while in fips mode.
> 
> The FIPS standard doesn't refer to particular applications like VNC.
> As Paul says earlier, FIP 140-2 requires that DES (and certain other
> ciphers) not be used in any applications which are running in a FIPS
> compliant environment. Since VNC auth uses DES, this auth scheme
> cannot be permitted in a FIPS environment.
> 
> The reason no other VNC server does this is almost certainly because
> none of their developers have ever tried to have their code work in
> a FIPS environment, so I don't think that's a relevant comparison.
> 
> I'm not really sure what addding more configuration options gains
> us here. The choice of auth mode is already configurable. This patch
> is about ensuring that the user is not allowed to configure it, if
> FIPS mode is in effect (as indicated by the kernels syfs tunable).
> So in fact adding config params doesn't really address this.
> 
> The proposed patch is already very straightforward, is using the
> official interface exposed by the upstream kernel to userspace &
> has negligable maintenence burden IMHO.

I've got nothing to add other than to say that I agree 100% with Daniel's 
comments above.
Anthony Liguori July 31, 2012, 8:52 p.m. UTC | #6
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Jul 31, 2012 at 02:52:07PM -0500, Anthony Liguori wrote:
>> Paul Moore <pmoore@redhat.com> writes:
>> 
>> > On Friday, June 08, 2012 05:38:12 PM Paul Moore wrote:
>> >> FIPS 140-2 requires disabling certain ciphers, including DES, which is used
>> >> by VNC to obscure passwords when they are sent over the network.  The
>> >> solution for FIPS users is to disable the use of VNC password auth when the
>> >> host system is operating in FIPS mode.
>> >> 
>> >> This patch causes QEMU to emit a message to stderr when the host system is
>> >> running in FIPS mode and a VNC password was specified on the commend line.
>> >> If the system is not running in FIPS mode, or is running in FIPS mode but
>> >> VNC password authentication was not requested, QEMU operates normally.
>> >> 
>> >> Signed-off-by: Paul Moore <pmoore@redhat.com>
>> >
>> > Hi Anthony,
>> >
>> > Any word on this patch?  Other than Daniel Berrange's reviewed-by tag, the 
>> > discussion of the v4 patch has been quiet and I think we addressed all the 
>> > other remaining issues in the discussion attached to the v2 patch
>> > posting.
>> 
>> I asked for the specific language in FIPS mandating this.  I don't see
>> any other VNC server implementing a check like this.  I would rather do
>> this in a more user friendly fashion like make it a config file option
>> that a user can set while in fips mode.
>
> The FIPS standard doesn't refer to particular applications like VNC.
> As Paul says earlier, FIP 140-2 requires that DES (and certain other
> ciphers) not be used in any applications which are running in a FIPS
> compliant environment. Since VNC auth uses DES, this auth scheme
> cannot be permitted in a FIPS environment.

OpenSSL requires an explicit function call to enable fips
mode--FIPS_mode_set().  It's not something that happens unconditionally
behind the scenes.  From talking to some folks here, it seems like
an -enable-fips option would meet the requirements of FIPS.

> The reason no other VNC server does this is almost certainly because
> none of their developers have ever tried to have their code work in
> a FIPS environment, so I don't think that's a relevant comparison.
>
> I'm not really sure what addding more configuration options gains
> us here. The choice of auth mode is already configurable. This patch
> is about ensuring that the user is not allowed to configure it, if
> FIPS mode is in effect (as indicated by the kernels syfs tunable).
> So in fact adding config params doesn't really address this.

Disabling options unconditionally based on a magic kernel parameter is
fundamentally wrong.  If a user wants QEMU to participate in FIPS,
it should explicitly ask QEMU to.

Since OpenSSL also does this, there seems to be ample precedence for it.

> The proposed patch is already very straightforward, is using the
> official interface exposed by the upstream kernel to userspace &
> has negligable maintenence burden IMHO.

It's not QEMU's role to enforce security policy.  Unconditionally
disabling features goes against a very basic architectural assumption in
QEMU.

From what I'm told, there's nothing in FIPS that prevents us from
masking this behavior behind a command line option.  And I think that's
the right thing to do.

Regards,

Anthony Liguori

>
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
diff mbox

Patch

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 0af0ff4..fe8d3df 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1124,9 +1124,11 @@  the protocol limits passwords to 8 characters it should not be considered
 to provide high security. The password can be fairly easily brute-forced by
 a client making repeat connections. For this reason, a VNC server using password
 authentication should be restricted to only listen on the loopback interface
-or UNIX domain sockets. Password authentication is requested with the @code{password}
-option, and then once QEMU is running the password is set with the monitor. Until
-the monitor is used to set the password all clients will be rejected.
+or UNIX domain sockets. Password authentication is not supported when operating
+in FIPS 140-2 compliance mode as it requires the use of the DES cipher. Password
+authentication is requested with the @code{password} option, and then once QEMU
+is running the password is set with the monitor. Until the monitor is used to
+set the password all clients will be rejected.
 
 @example
 qemu-system-i386 [...OPTIONS...] -vnc :1,password -monitor stdio
diff --git a/ui/vnc.c b/ui/vnc.c
index 54bc5ad..4bd816d 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -48,6 +48,21 @@  static DisplayChangeListener *dcl;
 static int vnc_cursor_define(VncState *vs);
 static void vnc_release_modifiers(VncState *vs);
 
+static bool fips_enabled(void)
+{
+    bool enabled = false;
+
+#ifdef __linux__
+    FILE *fds = fopen("/proc/sys/crypto/fips_enabled", "r");
+    if (fds != NULL) {
+        enabled = (fgetc(fds) == '1');
+        fclose(fds);
+    }
+#endif /* __linux__ */
+
+    return enabled;
+}
+
 static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
 {
 #ifdef _VNC_DEBUG
@@ -2748,6 +2763,9 @@  void vnc_display_init(DisplayState *ds)
     dcl->idle = 1;
     vnc_display = vs;
 
+    vs->fips = fips_enabled();
+    VNC_DEBUG("FIPS mode %s\n", (vs->fips ? "enabled" : "disabled"));
+
     vs->lsock = -1;
 
     vs->ds = ds;
@@ -2896,6 +2914,15 @@  int vnc_display_open(DisplayState *ds, const char *display)
     while ((options = strchr(options, ','))) {
         options++;
         if (strncmp(options, "password", 8) == 0) {
+            if (vs->fips) {
+                fprintf(stderr,
+                        "VNC password auth disabled due to FIPS mode, "
+                        "consider using the VeNCrypt or SASL authentication "
+                        "methods as an alternative\n");
+                g_free(vs->display);
+                vs->display = NULL;
+                return -1;
+            }
             password = 1; /* Require password auth */
         } else if (strncmp(options, "reverse", 7) == 0) {
             reverse = 1;
diff --git a/ui/vnc.h b/ui/vnc.h
index a851ebd..d41631b 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -160,6 +160,7 @@  struct VncDisplay
     char *display;
     char *password;
     time_t expires;
+    bool fips;
     int auth;
     bool lossy;
     bool non_adaptive;