Ensure TigerVNC isn't accessible via the network #151
Conversation
|
I will automatically update this comment whenever this PR is modified |
| echo "::group::Security - Verify TCP ports wasn't opened" | ||
| ports=(5800 5801 5900 5901) | ||
| for port in "${ports[@]}" | ||
| do | ||
| docker exec -it $container_id timeout --preserve-status 1 nc -vz localhost $port | tee -a /dev/stderr | \ | ||
| grep --quiet succeeded && { echo "Failed security check - port $port open" && SECURITY_OK=false; } || echo "Passed security check - port $port not opened" | ||
| done | ||
| echo "::endgroup::" |
There was a problem hiding this comment.
This isn't testing exactly what jupyter-remote-desktop-proxy starts, it instead tests how we start the vncserver here - ideally we would test it against what is dynamically started.
Currently our test suites relies on a docker container with a jupyter server being started.
An ideal test would ensure no localhost:port is listened to, and not just 0.0.0.0:port is listened to. We could perhaps do that from outside the container if we started it with --network host or execed into it before trying to open a connection.
| # TurboVNC and TigerVNC share the same origin and both use a Perl script | ||
| # as the executable vncserver. We can determine if vncserver is TigerVNC | ||
| # by searching tigervnc string in the Perl script. | ||
| # | ||
| # The content of the vncserver executable can differ depending on how | ||
| # TigerVNC and TurboVNC has been distributed. Below are files known to be | ||
| # read in some situations: | ||
| # | ||
| # - https://github.com/TigerVNC/tigervnc/blob/v1.13.1/unix/vncserver/vncserver.in | ||
| # - https://github.com/TurboVNC/turbovnc/blob/3.1.1/unix/vncserver.in | ||
| # | ||
| with open(vncserver) as vncserver_file: | ||
| vncserver_file_text = vncserver_file.read().casefold() | ||
| is_turbovnc = "turbovnc" in vncserver_file_text |
There was a problem hiding this comment.
This detection logic is what we had before, so I re-introduced it now that we needed to distinguish the VNC servers again.
| vnc_args = [vncserver, '-rfbunixpath', "{unix_socket}", "-rfbport", "-1"] | ||
| if is_turbovnc: | ||
| # turbovnc doesn't handle being passed -rfbport -1, but turbovnc also | ||
| # defaults to not opening a TCP port which is what we want to ensure | ||
| vnc_args = [vncserver, '-rfbunixpath', "{unix_socket}"] |
There was a problem hiding this comment.
Unknown vnc servers will receive -rfbport -1 now, and could perhaps due to that fail to start like TurboVNC would, but I figure its better we are safe than sorry about this if we have some weak support for unknown VNC servers.
|
Thank you for the quick review @minrk!! |
With 3.0.0, we claimed that UNIX sockets would be used instead of TCP ports, but when using TigerVNC its TCP port remained open because we didn't explicitly disable it as was needed.
This is a patch for that, addressing GHSA-vrq4-9hc3-cgp7.