Created attachment 2565 [details]
Clear failed remove forwardings in remote_forwards list for Mux mode

When requesting a port forwarding using a control socket, it is kept in a list so that subsequent requests for the same host and port can return early. The problem is that it is kept in the list even if the forwarding failed, leading to subsequent forwarding request to apparently succeed (ssh returns 0 to the shell).

* How to reproduce (tested on git's master 307bb40277ca2c32e97e61d70d1ed74b571fd6ba):

1) Launch ssh in "master" mode
    # ssh -N -F tunnel.conf tunnel

where tunnel.conf is like
    Host tunnel
        User xxx
        Port xxx
        HostName xxx.xxx.xxx.xxx
        IdentityFile xxx/xxx/id_rsa
        ControlPath /var/run/tunnel.sock
        ControlMaster yes
        BatchMode yes
        

2) Request a first port redirection (succeeds):
    # ssh -S /var/run/tunnel.sock -O forward -R 0.0.0.0:50004:192.168.0.1:1234 localhost

3) Request a second redirection on the same port, but to a second IP (which fails, as it should):
    # ssh -S /var/run/tunnel.sock -O forward -R 0.0.0.0:50004:192.168.0.2:1234 localhost

4) Repeat the previous command (which I think should fail, but does not):
    # ssh -S /var/run/tunnel.sock -O forward -R 0.0.0.0:50004:192.168.0.2:1234 localhost

* Expected result: at step 4), the shell should get a non-zero exit code
* Actual result: the shell gets a zero exit code


* What happens:

    - During the call 3), the port forwarding is requested to the server, and the forwarding is added to the configuration (calling add_remote_forward(&options, &fwd) at mux.c:786)
    - when the forwarding fails, which we know in mux_confirm_remote_forward(), this forwarding is not cleared from the list
    - when calling 4), some call to compare_forward(&fwd, options.remote_forwards + i) (at mux.c:741) returns true, hence process_mux_open_fwd() returns with success, and in the end, 0 is returned to the shell.


* Proposed fix:
    When we learn about a forwarding failure in mux_confirm_remote_forward(), we should clear the associated entry in options.remote_forwards.
    A patch is attached that does exactly that (doing the same as in process_mux_close_fwd()).


Does this seem reasonable ? It's the first time that I look at OpenSSH's code, so I might be missing lots of subtleties.