openssh bugs - Mar 2015 - [Bug 2363] New: With multiplexing, a forwarding is kept in the list of active forwardings even when it fails

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

            Bug ID: 2363
           Summary: With multiplexing, a forwarding is kept in the list of
                    active forwardings even when it fails
           Product: Portable OpenSSH
           Version: 6.7p1
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: yoann.ricordel at gmail.com

Created attachment 2565
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2565&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Yoann Ricordel <yoann.ricordel at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yoann.ricordel at gmail.com

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2565|0                           |1
        is obsolete|                            |
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 2594
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2594&action=edit
clear failed forwards in mux; check for previously-cleared entries

IMO it's worth checking that a particular entry isn't already cleared,
but I need to think more about where else might fail with zeroed-out
fwd entries

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2360

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Turns out clearing the forward is safe - we already do it in the cancel
path.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
   Attachment #2594|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2594|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
patch applied, this will be in openssh-6.9

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

--- Comment #4 from Yoann Ricordel <yoann.ricordel at gmail.com> ---
Yes I did it that way because I saw it was already done like that for
cancellings. Sorry I didn't answer quicker to the comment. Thank you
for your improvements and integration of the patch.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2363

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.