Preventing VNC servers from sending unhandled server packets

[Kounotori-ssbm]

I use UltraVNC. Sometimes it sends variable unhandled server packets. ruby-vnc don't handle the packet types, so it raises NotImplementedError exception... I need stopping it.

lib/net/vnc.rb

    def read_packet type
      case type
      ...
      else
        raise NotImplementedError, 'unhandled server packet type - %d' % type
      end
    end

Error message

/usr/local/lib/ruby/gems/2.5.0/gems/ruby-vnc-1.1.0/lib/net/vnc.rb:333:in `read_packet':
unhandled server packet type - 7 (NotImplementedError)

* packet type is variable. For other packet type example: 6, 8, 17 ... and so on

Once sending "client to server messages", VNC servers will send only messages which client can handle. So, we need to send initial data on connection. https://vncdotool.readthedocs.io/en/0.8.0/rfbproto.html#client-to-server-messages

[aquasync]

Hmm, my only issue with this is it essentially makes vncrec an unconditional dependency, defeating the point of the delayed require of 'net/rfb/frame_buffer'. I guess we could just throw in the towel on that. I'm unclear on what issue this is solving. Would any "client to server" message suffice to avoid these unwanted messages? If so can we just use something with lower overhead than a frame buffer update (eg clipboard)?

[Kounotori-ssbm]

I tried replace _load_frame_buffer to clipboard = 'hoge' in VNC#connect method but it has still received unwanted messages...

These unwanted messages seem to be "Protocol Extensions", maybe it is related to "pseudo-encoding". So maybe, we have to send information of encoding to stop them. Encoding is sent by _load_frame_buffer (it declares only "RAW" encoding is used by default).

And originally, I think we should tell client's handleable information to servers on connecting.

If frame buffer update would have not-low overhead, no occurrence of too many unwanted messages would be worth of it.

[lobin-z0x50]

Hi @aquasync, I think you are right.

In order for some VNC servers to stop sending extended instructions, I guess just send ‘set_encoding’ as soon as possible, as explicitly indicate that we have no extended instruction that can be handled.

If we extract the process of sending ‘set_encoding’ from the FrameBuffer class so that it can be called individually, I think that dependency on FrameBuffer can be kept to a minimum.

[aquasync]

Thanks for your thoughts @lobin-z0x50. I was thinking of something along those lines; perhaps calling set_encodings and set_pixel_format but avoiding request_update_fb until requested. It would mean adding state such that subsequent screenshot requests would default to incremental after the first though or some other refactor.

While I had a preference for avoiding an unconditional dependency, it seems that this negotiation is needed and I'm not sure that it's worth the additional complexity to avoid. I'm more inclined to just proceed with this change.

[lobin-z0x50]

As a result of my debug, the problem was not just extended instructions.

I found that the server (e.g. Ultra VNC) may send FrameBufferUpdate even if the client has not sent FrameBufferUpdateRequest. If _load_frame_buffer is not called, since @fb is nil in read_packet, the frame buffer data is stored in the socket buffer without being discarded, but it is read byte by byte by packet_reading_thread, and protocol can not be interpreted correctly.

The same applies to SetColourMapEntries.

In order to deal with this problem, we must do correctly interpret the protocol when receiving FrameBufferUpdate or SetColourMapEntries and discard any appropriate bytes. But if we do it, I think that it is much easier to call _load_frame_buffer from the beginning.

If _load_frame_buffer is not called, at the timing when protocol interpretation goes wrong, we should all the received data after that are all discarded.

[aquasync]

Hi @lobin-z0x50, thanks for taking a closer look. Curious that it can pre-emptively decide to push framebuffer updates. Seems like it's best to just support it unconditionally.

[lobin-z0x50]

Thank you for your decision @aquasync . I think so too. Without merging this PR, probably many user will be in trouble. because the protocol can not be interpreted correctly. I think that this PR should be approved. I don't have any comments.