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.csa...@proxmox.com>
Reviewed-By: Dominik Csapak <d.csa...@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.lampre...@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({



_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to