On Fri, Feb 11, 2011 at 08:16:17PM -0700, Eric Stadtherr wrote:
> On 02/11/2011 12:15 PM, DRC wrote:
> >On 2/11/11 12:06 PM, Eric Stadtherr wrote:
> >>The port conflict arises when you start two vncviewers with the "-via"
> >>flag back-to-back pointed to two different servers. In lockstep, both
> >>viewers will:
> >>
> >> 1. look for a free TCP port on the local host within a pre-determined
> >> sequence (by binding and then closing a socket)
> >> 2. start an SSH connection to the specified host, with local port
> >> forwarding specified on the chosen port
> >> 3. connect the viewer's RFB connection to the local SSH port
> >>
> >>If the SSH connection takes long enough (a few seconds, perhaps), the
> >>port found by viewer #1 in step 1 will still free when viewer #2 goes
> >>through the same process, and viewer #2 will try to start SSH with the
> >>same local port. The second SSH will fail to bind to the local port, but
> >>nobody notices that. The second viewer then connects to the first
> >>viewer's local SSH process in step 3, and voila - the second viewer is
> >>now pointing to the first server. This is especially bad if these
> >>viewers are started by different users, and the second user doesn't have
> >>the credentials to connect to the first viewer's SSH server on their own.
> >>
> >>I use VNC in an environment where multiple users are all starting
> >>multiple VNC sessions in the morning (one for each screen / server
> >>combination). They've seen this bug a few times, but fortunately it has
> >>heretofore only resulted in cross-connects of the same user's viewers.
> >Yeah, I think the problem is that we're trying to iterate through a
> >specific port range ourselves to find a free port rather than letting
> >the system pick one for us. That leads to the race condition you
> >describe. I think that if we replace our port picking code as follows,
> >it should fix the issue. Of course, this means we're no longer
> >confining our local port to the range of 5500-5599. Does anyone see a
> >problem with that? I don't.
>
> I see a couple problems with this solution:
>
> 1. This method still closes the socket after identifying a port,
> which would allow the port to be allocated again before
> /usr/bin/ssh finally gets around to opening the forwarding channel
> and binding to the local port. A second vncviewer process started
> in close proximity is admittedly *less* likely to choose that same
> port, but the possibility still exists. The likelihood would be a
> function of how the operating system assigns ephemeral ports,
> which is certainly platform dependent and hard to predict.
> 2. The port number you'll end up with is an ephemeral port. Although
> explicitly binding to and listening on an ephemeral port is
> certainly allowed by ssh, it is also a little unconventional.
DRC's patch seems like better approach for me than integration with
libssh.
However I would extend the patch a little. After we successfully bind(3)
a socket then we can spawn /usr/bin/ssh. We can probably use exec*(3)
instead of system(3) call and check if ssh was able to bind the port.
If not then we can try to find another free port & spawn ssh again.
For example 20 times by default. It's right in theory SSH tunnel
estabilishment might still fail but in practise it won't happen.
What is your opinion about this proposal?
Regards, Adam
>
> >Index: common/network/TcpSocket.cxx
> >===================================================================
> >--- common/network/TcpSocket.cxx (revision 4280)
> >+++ common/network/TcpSocket.cxx (working copy)
> >@@ -71,7 +71,7 @@
> > /* Tunnelling support. */
> > int network::findFreeTcpPort (void)
> > {
> >- int sock, port;
> >+ int sock;
> > struct sockaddr_in addr;
> > memset(&addr, 0, sizeof(addr));
> > addr.sin_family = AF_INET;
> >@@ -80,15 +80,16 @@
> > if ((sock = socket (AF_INET, SOCK_STREAM, 0))< 0)
> > throw SocketException ("unable to create socket", errorNumber);
> >
> >- for (port = TUNNEL_PORT_OFFSET + 99; port> TUNNEL_PORT_OFFSET; port--) {
> >- addr.sin_port = htons ((unsigned short) port);
> >- if (bind (sock, (struct sockaddr *)&addr, sizeof (addr)) == 0) {
> >- closesocket (sock);
> >- return port;
> >- }
> >- }
> >- throw SocketException ("no free port in range", 0);
> >- return 0;
> >+ addr.sin_port = 0;
> >+ if (bind (sock, (struct sockaddr *)&addr, sizeof (addr))< 0)
> >+ throw SocketException ("unable to find free port", errorNumber);
> >+
> >+ socklen_t n = sizeof(addr);
> >+ if (getsockname (sock, (struct sockaddr *)&addr,&n)< 0)
> >+ throw SocketException ("unable to get port number", errorNumber);
> >+
> >+ closesocket (sock);
> >+ return ntohs(addr.sin_port);
> > }
> >
> >
> >>No insult taken - this is a good discussion to have about a relatively
> >>significant implementation change.
> >>
> >>I sympathize with the pain introduced by GnuTLS, but this SSH change is
> >>different in several respects:
> >>
> >> 1. The SSH change fixes an existing capability, and doesn't introduce
> >> a new capability
> >Try out the patch above and see if it fixes the problem.
> >
> >
> >> 2. The existing SSH tunnel capability already had the same level of
> >> external dependency, albeit a runtime dependency on /usr/bin/ssh.
> >> Granted, the new implementation makes it a build-time dependency
> >> or a dynamic library dependency instead of a child process
> >> dependency. I would go so far as asserting that libssh is a more
> >> portable dependency than a hardcoded reference to "/usr/bin/ssh".
> >There is nothing preventing a -via option on Windows. It just hasn't
> >been implemented yet. We could use, for instance, PLink as the
> >tunneling process rather than /usr/bin/ssh. Also, on Unix, the VIA
> >command line can be configured at run time to use something other than
> >/usr/bin/ssh.
> >
> >
>
> Understood. Although I still think that libssh would get us one step
> closer to Windows tunneling support than the existing system() call
> method. libssh keeps vncviewer from having to encode the
> platform-specific semantics of child process invocation and
> management.
>
>
> >> 3. vncviewer is hardly unwieldy - have you tried building The GIMP?
> >> Wink Seriously, though, the existing "-via" logic would never
> >> have worked on Windows, so libssh might actually be a step towards
> >> better portability. Windows distros can be happily configured and
> >> built without libssh support, which leaves Windows users no better
> >> or worse off than they are today.
> >> 5. Only users that require/desire the SSH tunneling capability would
> >> be subject to any constraints imposed by the new implementation.
> >> This is different from the GnuTLS/VeNCrypt situation where the
> >> normal "vncviewer<server>:<screen>" case is affected.
> >Yeah, but one of the key things I want to avoid is a million different
> >configurations of TigerVNC. If everyone has their own "special sauce",
> >then a lot of features don't get tested -- particularly the features
> >that are difficult to build, like GnuTLS or (hypothetically) libssh.
> >TigerVNC may have different statistics, but with TurboVNC, the Windows
> >viewer is about 10x more popular than the Unix viewer, so if I want
> >something tested on the viewer side, I need it in the Windows viewer first.
> >
> >
>
> I guess I haven't seen any pain associated with building libssh. It
> sounds like you had a bad experience with GnuTLS (and you're not the
> only one!), but libssh doesn't depend on GnuTLS. I honestly can't
> speak to the ease of building libssh on Windows, but only because I
> don't have any Windows machines or VM's on which to play.
>
> >>These are good topics to discuss, and I'd be happy to participate in the
> >>discussion.
> >>
> >>However, I'd still like to know how you and the rest of the community
> >>feel about this particular SSH change, and the implementation questions
> >>I raised. As I said, I don't think it has the same mainstream
> >>consequences as GnuTLS, and might actually be a step towards supporting
> >>the SSH tunneling capability on non-Unix platforms.
> >>
> >>What do you think?
> >If we can fix the existing solution, that would be my preference.
> >
> >------------------------------------------------------------------------------
> >The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
> >Pinpoint memory and threading errors before they happen.
> >Find and fix more than 250 security defects in the development cycle.
> >Locate bottlenecks in serial and parallel code that limit performance.
> >http://p.sf.net/sfu/intel-dev2devfeb
> >_______________________________________________
> >Tigervnc-devel mailing list
> >Tigervnc-devel@lists.sourceforge.net
> >https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
>
> ------------------------------------------------------------------------------
> The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
> Pinpoint memory and threading errors before they happen.
> Find and fix more than 250 security defects in the development cycle.
> Locate bottlenecks in serial and parallel code that limit performance.
> http://p.sf.net/sfu/intel-dev2devfeb
> _______________________________________________
> Tigervnc-devel mailing list
> Tigervnc-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
--
Adam Tkac, Red Hat, Inc.
------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb
_______________________________________________
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
