[pve-devel] [PATCH qemu-server] vncproxy: allow to request a generated VNC password

[Dominik Csapak]

looks good to me and works as expected

if we wanted we could probably make this the default by either
* applying the novnc patch
* alternatively putting the generated password at the beginning of the 
vncticket and removing it before verifying in the websocket api call
this would be ok since the node where the api call is made is always the 
local node (tunneling is over ssh)

but since it really does not matter if we do it at all, we can
simply omit the novnc patch for now and leave it optional

Tested-By: Dominik Csapak <d.csapak at proxmox.com>
Reviewed-By: Dominik Csapak <d.csapak at proxmox.com>

On 6/18/20 6:20 PM, Thomas Lamprecht wrote:
> We used the VNC API $ticket as password for VNC, but QEMU limits the
> password to the first 8 chars and ignores the rest[0].
> As our tickets start with a static string (e.g., "PVE") the entropy
> was a bit limited.
> 
> For Proxmox VE this does not matters much as the noVNC viewer
> provided by has to go always over the API call, and so a valid
> ticket and correct permissions for the requested VM are enforced
> anyway.
> 
> This patch helps external users, which often use NoVNC-Websockify,
> circumventing the API and relying solely on the VNC password to avoid
> snooping on VNC sessions.
> 
> A 'generate-password' parameter is added, if set a password from good
> entropy (using libopenssl) is generated.
> 
> For simplicity of mapping random bits to ranges we extract 6 bit of
> entropy per character and add the integer value of '!' (first
> printable ASCII char) to that. This way we get 64^8 possibilities,
> which even with millions of guesses per second one would need years
> of guessing and mostly just DDOS the server with websocket upgrade
> requests.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> 
> We could also extract the last 8 chars of the ticket, but as that ticket is
> only secure as a whole, as seen with this issue, I'd like to avoid that
> 
> Initially I had a variant with using perls core rand and all 95 printable ascii
> chars, but that wasn't crypthographical secure
> 
>   PVE/API2/Qemu.pm | 39 ++++++++++++++++++++++++++++++++++++---
>   1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 3e0d59f..dcb364d 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -7,6 +7,7 @@ use Net::SSLeay;
>   use POSIX;
>   use IO::Socket::IP;
>   use URI::Escape;
> +use Crypt::OpenSSL::Random;
>   
>   use PVE::Cluster qw (cfs_read_file cfs_write_file);;
>   use PVE::RRD;
> @@ -1589,6 +1590,19 @@ __PACKAGE__->register_method({
>   	return undef;
>       }});
>   
> +# uses good entropy, each char is limited to 6 bit to get printable chars simply
> +my $gen_rand_chars = sub {
> +    my ($length) = @_;
> +
> +    die "invalid length $length" if $length < 1;
> +
> +    my $min = ord('!'); # first printable ascii
> +    my @rand_bytes = split '', Crypt::OpenSSL::Random::random_bytes($length);
> +    my $str = join('', map { chr((ord($_) & 0x3F) + $min) } @rand_bytes);
> +
> +    return $str;
> +};
> +
>   my $sslcert;
>   
>   __PACKAGE__->register_method({
> @@ -1610,6 +1624,12 @@ __PACKAGE__->register_method({
>   		type => 'boolean',
>   		description => "starts websockify instead of vncproxy",
>   	    },
> +	    'generate-password' => {
> +		optional => 1,
> +		type => 'boolean',
> +		default => 0,
> +		description => "Generates a random password to be used as ticket instead of the API ticket.",
> +	    },
>   	},
>       },
>       returns => {
> @@ -1617,6 +1637,12 @@ __PACKAGE__->register_method({
>   	properties => {
>   	    user => { type => 'string' },
>   	    ticket => { type => 'string' },
> +	    password => {
> +		optional => 1,
> +		description => "Returned if requested with 'generate-password' param."
> +		    ." Consists of printable ASCII characters ('!' .. '~').",
> +		type => 'string',
> +	    },
>   	    cert => { type => 'string' },
>   	    port => { type => 'integer' },
>   	    upid => { type => 'string' },
> @@ -1644,6 +1670,10 @@ __PACKAGE__->register_method({
>   	my $authpath = "/vms/$vmid";
>   
>   	my $ticket = PVE::AccessControl::assemble_vnc_ticket($authuser, $authpath);
> +	my $password = $ticket;
> +	if ($param->{'generate-password'}) {
> +	    $password = $gen_rand_chars->(8);
> +	}
>   
>   	$sslcert = PVE::Tools::file_get_contents("/etc/pve/pve-root-ca.pem", 8192)
>   	    if !$sslcert;
> @@ -1680,7 +1710,7 @@ __PACKAGE__->register_method({
>   			'-perm', 'Sys.Console'];
>   
>   		if ($param->{websocket}) {
> -		    $ENV{PVE_VNC_TICKET} = $ticket; # pass ticket to vncterm
> +		    $ENV{PVE_VNC_TICKET} = $password; # pass ticket to vncterm
>   		    push @$cmd, '-notls', '-listen', 'localhost';
>   		}
>   
> @@ -1690,7 +1720,7 @@ __PACKAGE__->register_method({
>   
>   	    } else {
>   
> -		$ENV{LC_PVE_TICKET} = $ticket if $websocket; # set ticket with "qm vncproxy"
> +		$ENV{LC_PVE_TICKET} = $password if $websocket; # set ticket with "qm vncproxy"
>   
>   		$cmd = [@$remcmd, "/usr/sbin/qm", 'vncproxy', $vmid];
>   
> @@ -1725,13 +1755,16 @@ __PACKAGE__->register_method({
>   
>   	PVE::Tools::wait_for_vnc_port($port);
>   
> -	return {
> +	my $res = {
>   	    user => $authuser,
>   	    ticket => $ticket,
>   	    port => $port,
>   	    upid => $upid,
>   	    cert => $sslcert,
>   	};
> +	$res->{password} = $password if $param->{'generate-password'};
> +
> +	return $res;
>       }});
>   
>   __PACKAGE__->register_method({
>