Re: [Qemu-devel] [PATCH] vnc: disable VNC password authentication (security type 2) when in FIPS mode

[Paul Moore]

On Wednesday, May 02, 2012 12:54:21 AM Andreas Färber wrote:
> Am 01.05.2012 23:20, schrieb Paul Moore:
> > 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 emits a syslog entry indicating that VNC
> > password
> 
> "to emit"
> 
> > auth is disabled when it detects the host is running in FIPS mode, and
> > unless a VNC password was specified on the command line it continues
> > normally.  However, if a VNC password was given on the command line, qemu
> > fails with an error message to stderr explaining that that VNC password
> 
> "explaining that VNC"

Thanks, typos fixed.

> > auth is not allowed in FIPS mode.
> > 
> > Signed-off-by: Paul Moore <address@hidden>
> 
> Interesting feature. :)

It would appear that depends on who you ask :)

> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index deb9ecd..620791e 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -32,6 +32,7 @@
> > 
> >  #include "acl.h"
> >  #include "qemu-objects.h"
> >  #include "qmp-commands.h"
> > 
> > +#include <syslog.h>
> 
> syslog.h is POSIX, but it'll need a guard for mingw32.

Is "#ifndef _WIN32" the right guard to use?  Both here and where we make the 
actual syslog() call?

> > @@ -48,6 +49,24 @@ static DisplayChangeListener *dcl;
> > 
> >  static int vnc_cursor_define(VncState *vs);
> >  static void vnc_release_modifiers(VncState *vs);
> > 
> > +static int fips_enabled(void)
> > +{
> > +    int enabled = 0;
> > +    char value;
> > +    FILE *fds;
> > +
> > +    fds = fopen("/proc/sys/crypto/fips_enabled", "r");
> 
> How standardized is this? Should we limit this to __linux__ or something?

It is in the mainline Linux Kernel so fairly standard as far as Linux is 
concerned.  However, it is Linux only to the best of my knowledge so I've gone 
ahead and protected it with __linux__ as you and others have mentioned.

> > +    if (fds == NULL) {
> > +        return 0;
> > +    }
> > +    if (fread(&value, sizeof(value), 1, fds) == 1 && value == '1') {
> > +        enabled = 1;
> > +    }
> > +    fclose(fds);
> > +
> > +    return enabled;
> > +}
> 
> bool would seem nicer as return type and field type below.

Yep, I agree.

I'll post a v2 later today; thanks for taking the time to review the patch and 
send your comments.

-- 
paul moore
security and virtualization @ redhat