vnc: Fix password expiration through 'change vnc ""'

commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
change vnc password command that changed the behavior of setting the VNC
password to an empty string from disabling login to disabling authentication.

This commit refactors the code to eliminate this overloaded semantics in
vnc_display_password and instead introduces the vnc_display_disable_login.   The
monitor implementation then determines the behavior of an empty or missing
string.

Recently, a set_password command was added that allows both the Spice and VNC
password to be set.  This command has not shown up in a release yet so the
behavior is not yet defined.

This patch proposes that an empty password be treated as an empty password with
no special handling.  For specifically disabling login, I believe a new command
should be introduced instead of overloading semantics.

I'm not sure how Spice handles this but I would recommend that we have Spice
and VNC have consistent semantics here for the 0.14.0 release.

Reported-by: Neil Wilson <neil@aldur.co.uk>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

On 01/31/2011 02:43 PM, Anthony Liguori wrote:
> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
> change vnc password command that changed the behavior of setting the VNC
> password to an empty string from disabling login to disabling authentication.
>
> This commit refactors the code to eliminate this overloaded semantics in
> vnc_display_password and instead introduces the vnc_display_disable_login.   The
> monitor implementation then determines the behavior of an empty or missing
> string.
>
> Recently, a set_password command was added that allows both the Spice and VNC
> password to be set.  This command has not shown up in a release yet so the
> behavior is not yet defined.
>
> This patch proposes that an empty password be treated as an empty password with
> no special handling.  For specifically disabling login, I believe a new command
> should be introduced instead of overloading semantics.
>
> I'm not sure how Spice handles this but I would recommend that we have Spice
> and VNC have consistent semantics here for the 0.14.0 release.
>
> Reported-by: Neil Wilson<neil@aldur.co.uk>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> diff --git a/console.h b/console.h
> index 3157330..f4e4741 100644
> --- a/console.h
> +++ b/console.h
> @@ -369,6 +369,7 @@ void vnc_display_init(DisplayState *ds);
>   void vnc_display_close(DisplayState *ds);
>   int vnc_display_open(DisplayState *ds, const char *display);
>   int vnc_display_password(DisplayState *ds, const char *password);
> +int vnc_display_disable_login(DisplayState *ds);
>   int vnc_display_pw_expire(DisplayState *ds, time_t expires);
>   void do_info_vnc_print(Monitor *mon, const QObject *data);
>   void do_info_vnc(Monitor *mon, QObject **ret_data);
> diff --git a/monitor.c b/monitor.c
> index c5f54f4..24ed971 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1018,6 +1018,13 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>
>   static int change_vnc_password(const char *password)
>   {
> +    if (!password || !password[0]) {
> +        if (vnc_display_disable_login(NULL)) {
> +            qerror_report(QERR_SET_PASSWD_FAILED);
> +            return -1;
> +        }
>    

Missing return 0;

However, since there is a check in the password handlign in VNC for 
empty password too, this still ended up working as expected.  I've 
removed that additional check locally and then added the return 0; such 
that I could confirm the functionality again.

Regards,

Anthony Liguori

> +    }
> +
>       if (vnc_display_password(NULL, password)<  0) {
>           qerror_report(QERR_SET_PASSWD_FAILED);
>           return -1;
> @@ -1117,6 +1124,8 @@ static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>               qerror_report(QERR_INVALID_PARAMETER, "connected");
>               return -1;
>           }
> +        /* Note that setting an empty password will not disable login through
> +         * this interface. */
>           rc = vnc_display_password(NULL, password);
>           if (rc != 0) {
>               qerror_report(QERR_SET_PASSWD_FAILED);
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 495d6d6..73e7ffa 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2484,6 +2484,24 @@ void vnc_display_close(DisplayState *ds)
>   #endif
>   }
>
> +int vnc_display_disable_login(DisplayState *ds)
> +{
> +    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> +
> +    if (!vs) {
> +        return -1;
> +    }
> +
> +    if (vs->password) {
> +        qemu_free(vs->password);
> +    }
> +
> +    vs->password = NULL;
> +    vs->auth = VNC_AUTH_VNC;
> +
> +    return 0;
> +}
> +
>   int vnc_display_password(DisplayState *ds, const char *password)
>   {
>       VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> @@ -2492,19 +2510,18 @@ int vnc_display_password(DisplayState *ds, const char *password)
>           return -1;
>       }
>
> +    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->password) {
>           qemu_free(vs->password);
>           vs->password = NULL;
>       }
> -    if (password&&  password[0]) {
> -        if (!(vs->password = qemu_strdup(password)))
> -            return -1;
> -        if (vs->auth == VNC_AUTH_NONE) {
> -            vs->auth = VNC_AUTH_VNC;
> -        }
> -    } else {
> -        vs->auth = VNC_AUTH_NONE;
> -    }
> +    vs->password = qemu_strdup(password);
> +    vs->auth = VNC_AUTH_VNC;
>
>       return 0;
>   }
>

On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote:
> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
> change vnc password command that changed the behavior of setting the VNC
> password to an empty string from disabling login to disabling authentication.
> 
> This commit refactors the code to eliminate this overloaded semantics in
> vnc_display_password and instead introduces the vnc_display_disable_login.   The
> monitor implementation then determines the behavior of an empty or missing
> string.

Personally I think this is a little overkill & just reverting the
original patch was fine, but from a functional POV your patch
produces the same results, so I won't argue.

> Recently, a set_password command was added that allows both the Spice and VNC
> password to be set.  This command has not shown up in a release yet so the
> behavior is not yet defined.
> 
> This patch proposes that an empty password be treated as an empty password with
> no special handling.  For specifically disabling login, I believe a new command
> should be introduced instead of overloading semantics.

Agreed, if some mgmt app does need to change this kind of thin
on the fly, they'll likely want more than just a toggle between
AUTH_NONE/AUTH_VNC too. eg There's AUTH_SASL, which is the only
VNC auth scheme with any real security, and the psuedo auth
schemes for providing the TLS encryption/certificate support.

> I'm not sure how Spice handles this but I would recommend that we have Spice
> and VNC have consistent semantics here for the 0.14.0 release.

Sounds like a very good idea.

> Reported-by: Neil Wilson <neil@aldur.co.uk>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/console.h b/console.h
> index 3157330..f4e4741 100644
> --- a/console.h
> +++ b/console.h
> @@ -369,6 +369,7 @@ void vnc_display_init(DisplayState *ds);
>  void vnc_display_close(DisplayState *ds);
>  int vnc_display_open(DisplayState *ds, const char *display);
>  int vnc_display_password(DisplayState *ds, const char *password);
> +int vnc_display_disable_login(DisplayState *ds);
>  int vnc_display_pw_expire(DisplayState *ds, time_t expires);
>  void do_info_vnc_print(Monitor *mon, const QObject *data);
>  void do_info_vnc(Monitor *mon, QObject **ret_data);
> diff --git a/monitor.c b/monitor.c
> index c5f54f4..24ed971 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1018,6 +1018,13 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  
>  static int change_vnc_password(const char *password)
>  {
> +    if (!password || !password[0]) {
> +        if (vnc_display_disable_login(NULL)) {
> +            qerror_report(QERR_SET_PASSWD_FAILED);
> +            return -1;
> +        }
> +    }
> +
>      if (vnc_display_password(NULL, password) < 0) {
>          qerror_report(QERR_SET_PASSWD_FAILED);
>          return -1;
> @@ -1117,6 +1124,8 @@ static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>              qerror_report(QERR_INVALID_PARAMETER, "connected");
>              return -1;
>          }
> +        /* Note that setting an empty password will not disable login through
> +         * this interface. */
>          rc = vnc_display_password(NULL, password);
>          if (rc != 0) {
>              qerror_report(QERR_SET_PASSWD_FAILED);
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 495d6d6..73e7ffa 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2484,6 +2484,24 @@ void vnc_display_close(DisplayState *ds)
>  #endif
>  }
>  
> +int vnc_display_disable_login(DisplayState *ds)
> +{
> +    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> +
> +    if (!vs) {
> +        return -1;
> +    }
> +
> +    if (vs->password) {
> +        qemu_free(vs->password);
> +    }
> +
> +    vs->password = NULL;
> +    vs->auth = VNC_AUTH_VNC;
> +
> +    return 0;
> +}
> +
>  int vnc_display_password(DisplayState *ds, const char *password)
>  {
>      VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> @@ -2492,19 +2510,18 @@ int vnc_display_password(DisplayState *ds, const char *password)
>          return -1;
>      }
>  
> +    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->password) {
>          qemu_free(vs->password);
>          vs->password = NULL;
>      }
> -    if (password && password[0]) {
> -        if (!(vs->password = qemu_strdup(password)))
> -            return -1;
> -        if (vs->auth == VNC_AUTH_NONE) {
> -            vs->auth = VNC_AUTH_VNC;
> -        }
> -    } else {
> -        vs->auth = VNC_AUTH_NONE;
> -    }
> +    vs->password = qemu_strdup(password);
> +    vs->auth = VNC_AUTH_VNC;
>  
>      return 0;
>  }

Looks good, assuming the addition of the missing 'return 0' you already
mentioned

Daniel

On 02/03/2011 10:29 AM, Daniel P. Berrange wrote:
> On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote:
>    
>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
>> change vnc password command that changed the behavior of setting the VNC
>> password to an empty string from disabling login to disabling authentication.
>>
>> This commit refactors the code to eliminate this overloaded semantics in
>> vnc_display_password and instead introduces the vnc_display_disable_login.   The
>> monitor implementation then determines the behavior of an empty or missing
>> string.
>>      
> Personally I think this is a little overkill&  just reverting the
> original patch was fine, but from a functional POV your patch
> produces the same results, so I won't argue.
>    

For 0.15, I'd like to introduce a new set of commands such that we don't 
multiplex the change command anymore.   This refactoring lays the ground 
work for that.

For instance, if you created a block device with the name 'vnc', you'd 
get very unexpected results!  Multiplexing based on special values on 
top of existing commands is pretty evil.

>> Recently, a set_password command was added that allows both the Spice and VNC
>> password to be set.  This command has not shown up in a release yet so the
>> behavior is not yet defined.
>>
>> This patch proposes that an empty password be treated as an empty password with
>> no special handling.  For specifically disabling login, I believe a new command
>> should be introduced instead of overloading semantics.
>>      
> Agreed, if some mgmt app does need to change this kind of thin
> on the fly, they'll likely want more than just a toggle between
> AUTH_NONE/AUTH_VNC too. eg There's AUTH_SASL, which is the only
> VNC auth scheme with any real security, and the psuedo auth
> schemes for providing the TLS encryption/certificate support.
>
>    
>> I'm not sure how Spice handles this but I would recommend that we have Spice
>> and VNC have consistent semantics here for the 0.14.0 release.
>>      
> Sounds like a very good idea.
>
>    
>> Reported-by: Neil Wilson<neil@aldur.co.uk>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/console.h b/console.h
>> index 3157330..f4e4741 100644
>> --- a/console.h
>> +++ b/console.h
>> @@ -369,6 +369,7 @@ void vnc_display_init(DisplayState *ds);
>>   void vnc_display_close(DisplayState *ds);
>>   int vnc_display_open(DisplayState *ds, const char *display);
>>   int vnc_display_password(DisplayState *ds, const char *password);
>> +int vnc_display_disable_login(DisplayState *ds);
>>   int vnc_display_pw_expire(DisplayState *ds, time_t expires);
>>   void do_info_vnc_print(Monitor *mon, const QObject *data);
>>   void do_info_vnc(Monitor *mon, QObject **ret_data);
>> diff --git a/monitor.c b/monitor.c
>> index c5f54f4..24ed971 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1018,6 +1018,13 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>
>>   static int change_vnc_password(const char *password)
>>   {
>> +    if (!password || !password[0]) {
>> +        if (vnc_display_disable_login(NULL)) {
>> +            qerror_report(QERR_SET_PASSWD_FAILED);
>> +            return -1;
>> +        }
>> +    }
>> +
>>       if (vnc_display_password(NULL, password)<  0) {
>>           qerror_report(QERR_SET_PASSWD_FAILED);
>>           return -1;
>> @@ -1117,6 +1124,8 @@ static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>               qerror_report(QERR_INVALID_PARAMETER, "connected");
>>               return -1;
>>           }
>> +        /* Note that setting an empty password will not disable login through
>> +         * this interface. */
>>           rc = vnc_display_password(NULL, password);
>>           if (rc != 0) {
>>               qerror_report(QERR_SET_PASSWD_FAILED);
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 495d6d6..73e7ffa 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2484,6 +2484,24 @@ void vnc_display_close(DisplayState *ds)
>>   #endif
>>   }
>>
>> +int vnc_display_disable_login(DisplayState *ds)
>> +{
>> +    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
>> +
>> +    if (!vs) {
>> +        return -1;
>> +    }
>> +
>> +    if (vs->password) {
>> +        qemu_free(vs->password);
>> +    }
>> +
>> +    vs->password = NULL;
>> +    vs->auth = VNC_AUTH_VNC;
>> +
>> +    return 0;
>> +}
>> +
>>   int vnc_display_password(DisplayState *ds, const char *password)
>>   {
>>       VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
>> @@ -2492,19 +2510,18 @@ int vnc_display_password(DisplayState *ds, const char *password)
>>           return -1;
>>       }
>>
>> +    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->password) {
>>           qemu_free(vs->password);
>>           vs->password = NULL;
>>       }
>> -    if (password&&  password[0]) {
>> -        if (!(vs->password = qemu_strdup(password)))
>> -            return -1;
>> -        if (vs->auth == VNC_AUTH_NONE) {
>> -            vs->auth = VNC_AUTH_VNC;
>> -        }
>> -    } else {
>> -        vs->auth = VNC_AUTH_NONE;
>> -    }
>> +    vs->password = qemu_strdup(password);
>> +    vs->auth = VNC_AUTH_VNC;
>>
>>       return 0;
>>   }
>>      
> Looks good, assuming the addition of the missing 'return 0' you already
> mentioned
>    

Yup.  Thanks.

Regards,

Anthony Liguori
> Daniel
>

On Thu, Feb 03, 2011 at 10:35:51AM -0600, Anthony Liguori wrote:
> On 02/03/2011 10:29 AM, Daniel P. Berrange wrote:
> >On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote:
> >>commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
> >>change vnc password command that changed the behavior of setting the VNC
> >>password to an empty string from disabling login to disabling authentication.
> >>
> >>This commit refactors the code to eliminate this overloaded semantics in
> >>vnc_display_password and instead introduces the vnc_display_disable_login.   The
> >>monitor implementation then determines the behavior of an empty or missing
> >>string.
> >Personally I think this is a little overkill&  just reverting the
> >original patch was fine, but from a functional POV your patch
> >produces the same results, so I won't argue.
> 
> For 0.15, I'd like to introduce a new set of commands such that we
> don't multiplex the change command anymore.   This refactoring lays
> the ground work for that.
> 
> For instance, if you created a block device with the name 'vnc',
> you'd get very unexpected results!  Multiplexing based on special
> values on top of existing commands is pretty evil.

Doesn't Gerd's 'set_password' command already replace the functionality
of the 'change vnc' command. So we should likely declare 'change vnc'
as deprecated in 0.14 and remove it in 0.16

Regards,
Daniel

On 02/03/2011 11:02 AM, Daniel P. Berrange wrote:
> On Thu, Feb 03, 2011 at 10:35:51AM -0600, Anthony Liguori wrote:
>    
>> On 02/03/2011 10:29 AM, Daniel P. Berrange wrote:
>>      
>>> On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote:
>>>        
>>>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
>>>> change vnc password command that changed the behavior of setting the VNC
>>>> password to an empty string from disabling login to disabling authentication.
>>>>
>>>> This commit refactors the code to eliminate this overloaded semantics in
>>>> vnc_display_password and instead introduces the vnc_display_disable_login.   The
>>>> monitor implementation then determines the behavior of an empty or missing
>>>> string.
>>>>          
>>> Personally I think this is a little overkill&   just reverting the
>>> original patch was fine, but from a functional POV your patch
>>> produces the same results, so I won't argue.
>>>        
>> For 0.15, I'd like to introduce a new set of commands such that we
>> don't multiplex the change command anymore.   This refactoring lays
>> the ground work for that.
>>
>> For instance, if you created a block device with the name 'vnc',
>> you'd get very unexpected results!  Multiplexing based on special
>> values on top of existing commands is pretty evil.
>>      
> Doesn't Gerd's 'set_password' command already replace the functionality
> of the 'change vnc' command. So we should likely declare 'change vnc'
> as deprecated in 0.14 and remove it in 0.16
>    

Yup.  But it doesn't let you disable login.  Since that was a feature of 
'change vnc', I think we need to provide a proper interface to do this.

Likewise, we need a new interface for changing the block device.  The 
way password setting is handled is fubar right now.

I've got some new commands documented in a git tree if you're interested.

http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json

Regards,

Anthony Liguori

> Regards,
> Daniel
>

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/03/2011 11:02 AM, Daniel P. Berrange wrote:
>> On Thu, Feb 03, 2011 at 10:35:51AM -0600, Anthony Liguori wrote:
>>    
>>> On 02/03/2011 10:29 AM, Daniel P. Berrange wrote:
>>>      
>>>> On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote:
>>>>        
>>>>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
>>>>> change vnc password command that changed the behavior of setting the VNC
>>>>> password to an empty string from disabling login to disabling authentication.
>>>>>
>>>>> This commit refactors the code to eliminate this overloaded semantics in
>>>>> vnc_display_password and instead introduces the vnc_display_disable_login.   The
>>>>> monitor implementation then determines the behavior of an empty or missing
>>>>> string.
>>>>>          
>>>> Personally I think this is a little overkill&   just reverting the
>>>> original patch was fine, but from a functional POV your patch
>>>> produces the same results, so I won't argue.
>>>>        
>>> For 0.15, I'd like to introduce a new set of commands such that we
>>> don't multiplex the change command anymore.   This refactoring lays
>>> the ground work for that.
>>>
>>> For instance, if you created a block device with the name 'vnc',
>>> you'd get very unexpected results!  Multiplexing based on special
>>> values on top of existing commands is pretty evil.
>>>      
>> Doesn't Gerd's 'set_password' command already replace the functionality
>> of the 'change vnc' command. So we should likely declare 'change vnc'
>> as deprecated in 0.14 and remove it in 0.16
>>    
>
> Yup.  But it doesn't let you disable login.  Since that was a feature
> of 'change vnc', I think we need to provide a proper interface to do
> this.
>
> Likewise, we need a new interface for changing the block device.  The
> way password setting is handled is fubar right now.

Not just that.

> I've got some new commands documented in a git tree if you're interested.
>
> http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json

The command to insert media into an empty block backend needs to support
same range of media options as the command to create a block backend
with media (that command is currently caught within drive_add,
struggling to get out).

My efforts to provide exactly that got bogged down last year by
protracted design discussions on the finer points of its QMP interface,
and the need to clean up and fix N+1 things before I can even start
attacking the goal problem.  Fortunately, much of the preliminary work
got in.  Unfortunately, I haven't been able to work on the rest, lately
:(

Media change is just a convenience command fusing eject and insert.  For
even more convenience, we can make it inherit option defaults from the
media being replaced.

On 01/31/11 21:43, Anthony Liguori wrote:
> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
> change vnc password command that changed the behavior of setting the VNC
> password to an empty string from disabling login to disabling authentication.
>
> This commit refactors the code to eliminate this overloaded semantics in
> vnc_display_password and instead introduces the vnc_display_disable_login.   The
> monitor implementation then determines the behavior of an empty or missing
> string.

Hmm, now about simply never ever changing vs->auth?

cheers,
   Gerd

On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
> On 01/31/11 21:43, Anthony Liguori wrote:
>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a 
>> regression in the
>> change vnc password command that changed the behavior of setting the VNC
>> password to an empty string from disabling login to disabling 
>> authentication.
>>
>> This commit refactors the code to eliminate this overloaded semantics in
>> vnc_display_password and instead introduces the 
>> vnc_display_disable_login.   The
>> monitor implementation then determines the behavior of an empty or 
>> missing
>> string.
>
> Hmm, now about simply never ever changing vs->auth?

If auth is none and you do a vnc change password "" then if we don't set 
vs->auth to vnc, it won't have the desired effect.  I really dislike the 
semantics of this command but that was a past mistake.

Regards,

Anthony Liguori

>
> cheers,
>   Gerd
>

On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote:
> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
> >On 01/31/11 21:43, Anthony Liguori wrote:
> >>commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a
> >>regression in the
> >>change vnc password command that changed the behavior of setting the VNC
> >>password to an empty string from disabling login to disabling
> >>authentication.
> >>
> >>This commit refactors the code to eliminate this overloaded semantics in
> >>vnc_display_password and instead introduces the
> >>vnc_display_disable_login.   The
> >>monitor implementation then determines the behavior of an empty
> >>or missing
> >>string.
> >
> >Hmm, now about simply never ever changing vs->auth?
> 
> If auth is none and you do a vnc change password "" then if we don't
> set vs->auth to vnc, it won't have the desired effect.  I really
> dislike the semantics of this command but that was a past mistake.

Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw.
If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT
and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme.
So we really do want the change password command to leave 'vs->auth'
alone completely - just change the password string, with no side effects
on auth methods. If an app intends to use the change password command
it will have already launched QEMU with neccessary -vnc flags to set the
desired vs->auth and vs->subauth methods.

Regards,
Daniel

On 02/14/11 13:10, Anthony Liguori wrote:
> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
>> On 01/31/11 21:43, Anthony Liguori wrote:
>>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a
>>> regression in the
>>> change vnc password command that changed the behavior of setting the VNC
>>> password to an empty string from disabling login to disabling
>>> authentication.
>>>
>>> This commit refactors the code to eliminate this overloaded semantics in
>>> vnc_display_password and instead introduces the
>>> vnc_display_disable_login. The
>>> monitor implementation then determines the behavior of an empty or
>>> missing
>>> string.
>>
>> Hmm, now about simply never ever changing vs->auth?
>
> If auth is none and you do a vnc change password "" then if we don't set
> vs->auth to vnc, it won't have the desired effect.

If you want a password-protected vnc session you should better 
explicitly say so using '-vnc :0,password', otherwise you'll have a 
window (between qemu start and setting the password) where vnc clients 
can connect without a password.

Going from "none" to "vnc" automagically when setting a password 
encourages this insecure way to enable password protection.  IMHO we 
should stop doing this.  There are backward compatibility issues though 
as qemu did this for quite some time ...

Going from "vnc" to "none" automagically when setting a empty password 
is a no-go from a security point of view, especially as older qemu 
versions did *not* do that.

I don't think we'll need a monitor command to switch authentication 
methods on the fly.  YMMV.

cheers,
   Gerd

On 02/14/2011 06:24 AM, Daniel P. Berrange wrote:
> On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote:
>    
>> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
>>      
>>> On 01/31/11 21:43, Anthony Liguori wrote:
>>>        
>>>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a
>>>> regression in the
>>>> change vnc password command that changed the behavior of setting the VNC
>>>> password to an empty string from disabling login to disabling
>>>> authentication.
>>>>
>>>> This commit refactors the code to eliminate this overloaded semantics in
>>>> vnc_display_password and instead introduces the
>>>> vnc_display_disable_login.   The
>>>> monitor implementation then determines the behavior of an empty
>>>> or missing
>>>> string.
>>>>          
>>> Hmm, now about simply never ever changing vs->auth?
>>>        
>> If auth is none and you do a vnc change password "" then if we don't
>> set vs->auth to vnc, it won't have the desired effect.  I really
>> dislike the semantics of this command but that was a past mistake.
>>      
> Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw.
>    

But this is the semantics of the command.  I agree it's stupid but a 
security flaw is a regression and this is not a regression.

This is why the set-password command no longer does any of this nonsense.

> If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT
> and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme.
> So we really do want the change password command to leave 'vs->auth'
> alone completely - just change the password string, with no side effects
> on auth methods. If an app intends to use the change password command
> it will have already launched QEMU with neccessary -vnc flags to set the
> desired vs->auth and vs->subauth methods.
>    

I think I see how this could work but I'm not sure it's worth doing.  
I'd rather just leave the (bad) semantics of this command alone and 
deprecate the interface.

Regards,

Anthony Liguori

> Regards,
> Daniel
>

On 02/14/2011 07:56 AM, Gerd Hoffmann wrote:
> On 02/14/11 13:10, Anthony Liguori wrote:
>> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
>>> On 01/31/11 21:43, Anthony Liguori wrote:
>>>> commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a
>>>> regression in the
>>>> change vnc password command that changed the behavior of setting 
>>>> the VNC
>>>> password to an empty string from disabling login to disabling
>>>> authentication.
>>>>
>>>> This commit refactors the code to eliminate this overloaded 
>>>> semantics in
>>>> vnc_display_password and instead introduces the
>>>> vnc_display_disable_login. The
>>>> monitor implementation then determines the behavior of an empty or
>>>> missing
>>>> string.
>>>
>>> Hmm, now about simply never ever changing vs->auth?
>>
>> If auth is none and you do a vnc change password "" then if we don't set
>> vs->auth to vnc, it won't have the desired effect.
>
> If you want a password-protected vnc session you should better 
> explicitly say so using '-vnc :0,password', otherwise you'll have a 
> window (between qemu start and setting the password) where vnc clients 
> can connect without a password.
>
> Going from "none" to "vnc" automagically when setting a password 
> encourages this insecure way to enable password protection.  IMHO we 
> should stop doing this.  There are backward compatibility issues 
> though as qemu did this for quite some time ...

Yeah, change vnc is deprecated, set-password has sane semantics.

> Going from "vnc" to "none" automagically when setting a empty password 
> is a no-go from a security point of view, especially as older qemu 
> versions did *not* do that. ]

Yup, this was the reason for the CVE.

> I don't think we'll need a monitor command to switch authentication 
> methods on the fly.  YMMV.

Actually, I do, but that's far off on my radar screen.

One thing we're noticing is that QEMU is not terribly forgiving in a 
hosting environment.  If you make a mistake configuring something (like 
use the wrong auth type), you're only recourse is restarting the guest 
with the new options.  Restarting a guest of a paying customer == 
unhappy customer.

The more we can change without restarting a guest the better IMHO.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

On Mon, Feb 14, 2011 at 08:14:07AM -0600, Anthony Liguori wrote:
> On 02/14/2011 06:24 AM, Daniel P. Berrange wrote:
> >On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote:
> >>On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
> >>>On 01/31/11 21:43, Anthony Liguori wrote:
> >>>>commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a
> >>>>regression in the
> >>>>change vnc password command that changed the behavior of setting the VNC
> >>>>password to an empty string from disabling login to disabling
> >>>>authentication.
> >>>>
> >>>>This commit refactors the code to eliminate this overloaded semantics in
> >>>>vnc_display_password and instead introduces the
> >>>>vnc_display_disable_login.   The
> >>>>monitor implementation then determines the behavior of an empty
> >>>>or missing
> >>>>string.
> >>>Hmm, now about simply never ever changing vs->auth?
> >>If auth is none and you do a vnc change password "" then if we don't
> >>set vs->auth to vnc, it won't have the desired effect.  I really
> >>dislike the semantics of this command but that was a past mistake.
> >Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw.
> 
> But this is the semantics of the command.  I agree it's stupid but a
> security flaw is a regression and this is not a regression.
> 
> This is why the set-password command no longer does any of this nonsense.
> 
> >If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT
> >and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme.
> >So we really do want the change password command to leave 'vs->auth'
> >alone completely - just change the password string, with no side effects
> >on auth methods. If an app intends to use the change password command
> >it will have already launched QEMU with neccessary -vnc flags to set the
> >desired vs->auth and vs->subauth methods.
> 
> I think I see how this could work but I'm not sure it's worth doing.
> I'd rather just leave the (bad) semantics of this command alone and
> deprecate the interface.

If you make it blindly set 'vs->auth = VNC' then you haven't fully
fixed the security flawed, because you will be downgrading from
'vencrypt+vnc' auth to just 'vnc' which loose all your data encryption
capabilities. Just reverting the previous commit 52c18be9e99dabe295321153fda7fce9f76647ac
fully addresses the problems leaving the command side-effect free
as it was originally designed & implemented.

Regards,
Daniel