Message ID | 1386777271-12667-1-git-send-email-kraxel@redhat.com |
---|---|
State | New |
Headers | show |
Il 11/12/2013 16:54, Gerd Hoffmann ha scritto: > Current code silently changes the authentication settings > in case you try to set a password without password authentication > turned on. This is bad. Return an error instead. > > If we want allow changing auth settings at runtime this should > be done explicitly using a separate monitor command, not as > side effect of set_passwd. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Isn't this backwards-incompatible? Paolo > --- > ui/vnc.c | 34 ++++++---------------------------- > 1 file changed, 6 insertions(+), 28 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index 5601cc3..79efb80 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -2971,26 +2971,6 @@ static void vnc_display_close(DisplayState *ds) > #endif > } > > -static int vnc_display_disable_login(DisplayState *ds) > -{ > - VncDisplay *vs = vnc_display; > - > - if (!vs) { > - return -1; > - } > - > - if (vs->password) { > - g_free(vs->password); > - } > - > - vs->password = NULL; > - if (vs->auth == VNC_AUTH_NONE) { > - vs->auth = VNC_AUTH_VNC; > - } > - > - return 0; > -} > - > int vnc_display_password(DisplayState *ds, const char *password) > { > VncDisplay *vs = vnc_display; > @@ -2998,20 +2978,18 @@ int vnc_display_password(DisplayState *ds, const char *password) > if (!vs) { > return -EINVAL; > } > - > - if (!password) { > - /* This is not the intention of this interface but err on the side > - of being safe */ > - return vnc_display_disable_login(ds); > + if (vs->auth == VNC_AUTH_NONE) { > + error_printf_unless_qmp("If you want use passwords please enable " > + "password auth using '-vnc ${dpy},password'."); > + return -EINVAL; > } > > if (vs->password) { > g_free(vs->password); > vs->password = NULL; > } > - vs->password = g_strdup(password); > - if (vs->auth == VNC_AUTH_NONE) { > - vs->auth = VNC_AUTH_VNC; > + if (password) { > + vs->password = g_strdup(password); > } > > return 0; >
On Mi, 2013-12-11 at 17:06 +0100, Paolo Bonzini wrote: > Il 11/12/2013 16:54, Gerd Hoffmann ha scritto: > > Current code silently changes the authentication settings > > in case you try to set a password without password authentication > > turned on. This is bad. Return an error instead. > > > > If we want allow changing auth settings at runtime this should > > be done explicitly using a separate monitor command, not as > > side effect of set_passwd. > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Isn't this backwards-incompatible? Yes. I think it is the correct thing nevertheless. Users which want a passwort protected guests should configure vnc correctly to avoid a unprotected window between qemu start and setting the password. Also note that enabling passwd auth via "set_passwd" side-effect bypasses fips restrictions. So this is a clear security improvement IMHO. cheers, Gerd
Il 11/12/2013 17:29, Gerd Hoffmann ha scritto: > On Mi, 2013-12-11 at 17:06 +0100, Paolo Bonzini wrote: >> Il 11/12/2013 16:54, Gerd Hoffmann ha scritto: >>> Current code silently changes the authentication settings >>> in case you try to set a password without password authentication >>> turned on. This is bad. Return an error instead. >>> >>> If we want allow changing auth settings at runtime this should >>> be done explicitly using a separate monitor command, not as >>> side effect of set_passwd. >>> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> >> Isn't this backwards-incompatible? > > Yes. I think it is the correct thing nevertheless. Fine by me, let's just make sure we document it well. Can you start the 2.0 changelog wiki page? > Users which want a passwort protected guests should configure vnc > correctly to avoid a unprotected window between qemu start and setting > the password. > > Also note that enabling passwd auth via "set_passwd" side-effect > bypasses fips restrictions. That'd be a clear bug, even one that could be fixed in stable versions. Paolo > So this is a clear security improvement IMHO. > > cheers, > Gerd > > >
Hi, > >> > >> Isn't this backwards-incompatible? > > > > Yes. I think it is the correct thing nevertheless. > > Fine by me, let's just make sure we document it well. Can you start the > 2.0 changelog wiki page? -ENOACCOUNT Anthony? Can you add me? uid kraxel please. cheers, Gerd
diff --git a/ui/vnc.c b/ui/vnc.c index 5601cc3..79efb80 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -2971,26 +2971,6 @@ static void vnc_display_close(DisplayState *ds) #endif } -static int vnc_display_disable_login(DisplayState *ds) -{ - VncDisplay *vs = vnc_display; - - if (!vs) { - return -1; - } - - if (vs->password) { - g_free(vs->password); - } - - vs->password = NULL; - if (vs->auth == VNC_AUTH_NONE) { - vs->auth = VNC_AUTH_VNC; - } - - return 0; -} - int vnc_display_password(DisplayState *ds, const char *password) { VncDisplay *vs = vnc_display; @@ -2998,20 +2978,18 @@ int vnc_display_password(DisplayState *ds, const char *password) if (!vs) { return -EINVAL; } - - if (!password) { - /* This is not the intention of this interface but err on the side - of being safe */ - return vnc_display_disable_login(ds); + if (vs->auth == VNC_AUTH_NONE) { + error_printf_unless_qmp("If you want use passwords please enable " + "password auth using '-vnc ${dpy},password'."); + return -EINVAL; } if (vs->password) { g_free(vs->password); vs->password = NULL; } - vs->password = g_strdup(password); - if (vs->auth == VNC_AUTH_NONE) { - vs->auth = VNC_AUTH_VNC; + if (password) { + vs->password = g_strdup(password); } return 0;
Current code silently changes the authentication settings in case you try to set a password without password authentication turned on. This is bad. Return an error instead. If we want allow changing auth settings at runtime this should be done explicitly using a separate monitor command, not as side effect of set_passwd. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/vnc.c | 34 ++++++---------------------------- 1 file changed, 6 insertions(+), 28 deletions(-)