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

[Paul Moore <pmoore@redhat.com>]

On Wednesday, May 02, 2012 10:18:50 AM Daniel P. Berrange wrote:
> On Tue, May 01, 2012 at 05:20:40PM -0400, Paul Moore wrote:
> > 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>
> > 
> >  #define VNC_REFRESH_INTERVAL_BASE 30
> >  #define VNC_REFRESH_INTERVAL_INC  50
> > 
> > @@ -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)
> 
> s/int/bool/  and use true/false as values

Fixed.
 
> > +{
> > +    int enabled = 0;
> > +    char value;
> > +    FILE *fds;
> > +
> > +    fds = fopen("/proc/sys/crypto/fips_enabled", "r");
> > +    if (fds == NULL) {
> > +        return 0;
> > +    }
> > +    if (fread(&value, sizeof(value), 1, fds) == 1 && value == '1') {
> > +        enabled = 1;
> > +    }
> > +    fclose(fds);
> > +
> > +    return enabled;
> > +}
> 
> As already pointed out,wWe should probably make this depend on
> __linux__, and 'return false' fo other platforms.

Yep, that makes sense.  Fixed.

> >  static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
> >  {
> >  #ifdef _VNC_DEBUG
> > 
> > @@ -2748,6 +2767,12 @@ 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"));
> > +    if (vs->fips) {
> > +        syslog(LOG_NOTICE, "Disabling VNC password auth due to FIPS
> > mode\n"); +    }
> 
> I think this syslog message is better placed in the next chunk of the
> patch where you actually test the vs->fips value.

My reasoning for placing it here is so that there would be some positive 
indication that VNC password auth is disabled even if the user isn't 
attempting to use the it.  Although I can understand the reasoning for moving 
it down as well so that things are quieter in the non-passwd-auth case.

With this in mind, do you still think it should be moved down?

Regardless, thanks again for your time and comments.
 
-- 
paul moore
security and virtualization @ redhat