[pve-devel] [PATCH http-server 2/2] Fix #1684 WebSocket proxy behind a buffered proxy.

[Thomas Lamprecht]

On 5/25/18 3:20 PM, René Jochum wrote:
> The given patch fixes incoming WebSocket traffic behind buffered Proxies
> like Nginx.
> 
> This fixes the "blank screen" problem users reported on the forums.
> 
> To have a fully working Nginx Reverse Proxy setup its important to
> update nginx to 1.14 from the upstream repo.

With git show -w this patch looks actually not that big anymore..
IMO it's good practice to mention if most of the changes are just
indentation ones, reviewers get less scared then and the patch will
find it's way faster into git :)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index 382eab4..eac788b 100755
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -429,7 +429,7 @@ sub websocket_proxy {
            my $hdlreader = sub {
                my ($hdl) = @_;

-               my $len = length($hdl->{rbuf});
+               while (my $len = length($hdl->{rbuf})) {
                    return if $len < 2;

                    my $hdr = unpack('C', substr($hdl->{rbuf}, 0, 1));
@@ -463,7 +463,7 @@ sub websocket_proxy {

                    return if $len < ($offset + 4 + $payload_len);

-               my $data = substr($hdl->{rbuf}, 0, $len, ''); # now consume data
+                   my $data = substr($hdl->{rbuf}, 0, $offset + 4 + $payload_len, ''); # now consume data

                    my @mask = (unpack('C', substr($data, $offset+0, 1)),
                        unpack('C', substr($data, $offset+1, 1)),
@@ -494,6 +494,7 @@ sub websocket_proxy {
                    } else {
                        die "received unhandled websocket opcode $opcode\n";
                    }
+               }
            };

            my $proto = $reqstate->{proto} ? $reqstate->{proto}->{str} : 'HTTP/1.1';


So, AFAIU, we read more than our calculated payload length and
dropped the remaining data, thus potentially corrupting the next
read.
Further we may get two or more websocket frames in one callback,
but we did only process at max one at any callback, thus if multiple
got buffered together (i.e. NGINX) and after that nothing got send
(thus the callback not called again) we would never process these.

Could you confirm these thesis? And please add this to the commit message,
i.e., what was wrong, why the bug occurred else it's really hard to follow
why/what you do in the changes.

That all said: The changes itself look actually good!

> ---
>  PVE/APIServer/AnyEvent.pm | 95 ++++++++++++++++++++++++-----------------------
>  1 file changed, 48 insertions(+), 47 deletions(-)
> 
> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
> index 382eab4..eac788b 100755
> --- a/PVE/APIServer/AnyEvent.pm
> +++ b/PVE/APIServer/AnyEvent.pm
> @@ -429,70 +429,71 @@ sub websocket_proxy {
>  	    my $hdlreader = sub {
>  		my ($hdl) = @_;
>  
> -		my $len = length($hdl->{rbuf});
> -		return if $len < 2;
> +		while (my $len = length($hdl->{rbuf})) {
> +		    return if $len < 2;
>  
> -		my $hdr = unpack('C', substr($hdl->{rbuf}, 0, 1));
> -		my $opcode = $hdr & 0b00001111;
> -		my $fin = $hdr & 0b10000000;
> +		    my $hdr = unpack('C', substr($hdl->{rbuf}, 0, 1));
> +		    my $opcode = $hdr & 0b00001111;
> +		    my $fin = $hdr & 0b10000000;
>  
> -		die "received fragmented websocket frame\n" if !$fin;
> +		    die "received fragmented websocket frame\n" if !$fin;
>  
> -		my $rsv = $hdr & 0b01110000;
> -		die "received websocket frame with RSV flags\n" if $rsv;
> +		    my $rsv = $hdr & 0b01110000;
> +		    die "received websocket frame with RSV flags\n" if $rsv;
>  
> -		my $payload_len = unpack 'C', substr($hdl->{rbuf}, 1, 1);
> +		    my $payload_len = unpack 'C', substr($hdl->{rbuf}, 1, 1);
>  
> -		my $masked = $payload_len & 0b10000000;
> -		die "received unmasked websocket frame from client\n" if !$masked;
> +		    my $masked = $payload_len & 0b10000000;
> +		    die "received unmasked websocket frame from client\n" if !$masked;
>  
> -		my $offset = 2;
> -		$payload_len = $payload_len & 0b01111111;
> -		if ($payload_len == 126) {
> -		    return if $len < 4;
> -		    $payload_len = unpack('n', substr($hdl->{rbuf}, $offset, 2));
> -		    $offset += 2;
> -		} elsif ($payload_len == 127) {
> -		    return if $len < 10;
> -		    $payload_len = unpack('Q>', substr($hdl->{rbuf}, $offset, 8));
> -		    $offset += 8;
> -		}
> +		    my $offset = 2;
> +		    $payload_len = $payload_len & 0b01111111;
> +		    if ($payload_len == 126) {
> +			return if $len < 4;
> +			$payload_len = unpack('n', substr($hdl->{rbuf}, $offset, 2));
> +			$offset += 2;
> +		    } elsif ($payload_len == 127) {
> +			return if $len < 10;
> +			$payload_len = unpack('Q>', substr($hdl->{rbuf}, $offset, 8));
> +			$offset += 8;
> +		    }
>  
> -		die "received too large websocket frame (len = $payload_len)\n"
> -		    if ($payload_len > $max_payload_size) || ($payload_len < 0);
> +		    die "received too large websocket frame (len = $payload_len)\n"
> +			if ($payload_len > $max_payload_size) || ($payload_len < 0);
>  
> -		return if $len < ($offset + 4 + $payload_len);
> +		    return if $len < ($offset + 4 + $payload_len);
>  
> -		my $data = substr($hdl->{rbuf}, 0, $len, ''); # now consume data
> +		    my $data = substr($hdl->{rbuf}, 0, $offset + 4 + $payload_len, ''); # now consume data
>  
> -		my @mask = (unpack('C', substr($data, $offset+0, 1)),
> -			    unpack('C', substr($data, $offset+1, 1)),
> -			    unpack('C', substr($data, $offset+2, 1)),
> -			    unpack('C', substr($data, $offset+3, 1)));
> +		    my @mask = (unpack('C', substr($data, $offset+0, 1)),
> +			unpack('C', substr($data, $offset+1, 1)),
> +			unpack('C', substr($data, $offset+2, 1)),
> +			unpack('C', substr($data, $offset+3, 1)));
>  
> -		$offset += 4;
> +		    $offset += 4;
>  
> -		my $payload = substr($data, $offset, $payload_len);
> +		    my $payload = substr($data, $offset, $payload_len);
>  
> -		for (my $i = 0; $i < $payload_len; $i++) {
> -		    my $d = unpack('C', substr($payload, $i, 1));
> -		    my $n = $d ^ $mask[$i % 4];
> -		    substr($payload, $i, 1, pack('C', $n));
> -		}
> +		    for (my $i = 0; $i < $payload_len; $i++) {
> +			my $d = unpack('C', substr($payload, $i, 1));
> +			my $n = $d ^ $mask[$i % 4];
> +			substr($payload, $i, 1, pack('C', $n));
> +		    }
>  
> -		$payload = decode_base64($payload) if !$binary;
> +		    $payload = decode_base64($payload) if !$binary;
>  
> -		if ($opcode == 1 || $opcode == 2) {
> -		    $reqstate->{proxyhdl}->push_write($payload) if $reqstate->{proxyhdl};
> -		} elsif ($opcode == 8) {
> -		    my $statuscode = unpack ("n", $payload);
> -		    print "websocket received close. status code: '$statuscode'\n" if $self->{debug};
> +		    if ($opcode == 1 || $opcode == 2) {
> +			$reqstate->{proxyhdl}->push_write($payload) if $reqstate->{proxyhdl};
> +		    } elsif ($opcode == 8) {
> +			my $statuscode = unpack ("n", $payload);
> +			print "websocket received close. status code: '$statuscode'\n" if $self->{debug};
>  		    if ($reqstate->{proxyhdl}) {
> -			$reqstate->{proxyhdl}->push_shutdown();
> +				$reqstate->{proxyhdl}->push_shutdown();
> +		    }
> +			$hdl->push_shutdown();
> +		    } else {
> +			die "received unhandled websocket opcode $opcode\n";
>  		    }
> -		    $hdl->push_shutdown();
> -		} else {
> -		    die "received unhandled websocket opcode $opcode\n";
>  		}
>  	    };
>  
>