session BUGFIX do not close SSH socket twice

Since libssh 0.10, ssh_disconnect() only closes the socket only if it
is not passed via options.
For session server, the SSH socket is not passed to libssh via options.
It is then closed twice in nc_session_free_transport(): once by libssh
in ssh_disconnect() and again by us. This leads to undefined behaviour
in multi-threaded programs.

Currently, we can not distinguish between a socket passed via options
and a socket owned by libssh for cleanup. There is no API in libssh
that can retrieve the socket FD passed via options.

Ensure to pass all sockets via options and handle ssh_disconnect()
behavior before libssh 0.10 (libnetconf2 requires libssh >= 0.9.5)
to prevent double close.

Fixes: 70e9062 ("session BUGFIX close SSH socket")
Signed-off-by: Ilyes Ben Hamouda <[email protected]>

Author: Ilyes Ben Hamouda

src/session.c

@@ -816,12 +816,18 @@ nc_session_free_transport(struct nc_session *session, int *multisession)
                     free(siter);
                 } while (session->ti.libssh.next != session);
             }
-            /* remember sock so we can close it */
-            sock = ssh_get_fd(session->ti.libssh.session);
             if (connected) {
-                /* does not close sock */
+                /* remember sock so we can close it */
+                sock = ssh_get_fd(session->ti.libssh.session);
+
+                /* clears sock but does not close it if passed via options (libssh >= 0.10) */
                 ssh_disconnect(session->ti.libssh.session);
+#if (LIBSSH_VERSION_MAJOR == 0 && LIBSSH_VERSION_MINOR < 10)
+                sock = -1;
+#endif
             }
+
+            /* closes sock if set */
             ssh_free(session->ti.libssh.session);
         } else {
             /* remove the session from the list */

src/session_server_ssh.c

@@ -2020,8 +2020,25 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt
     if (ssh_bind_accept_fd(sbind, session->ti.libssh.session, sock) == SSH_ERROR) {
         ERR(session, "SSH failed to accept a new connection (%s).", ssh_get_error(sbind));
         rc = -1;
+
+        /* Avoid closing the socket on failure to prevent a possible double close.
+         * On failure, sock may or not be set to the session. In theory, we should
+         * be able to compare sock with ssh_get_fd() and close it only if it was
+         * not set, for example:
+         *
+         *     if (ssh_get_fd(session) == sock)
+         *         sock = -1;
+         *
+         * However, if ssh_bind_accept_fd() fails to allocate the socket structure
+         * internally, calling ssh_get_fd() will dereference a NULL pointer due to
+         * a buggy behavior in libssh.
+         */
+        sock = -1;
         goto cleanup;
     }
+
+    /* use SSH_OPTIONS_FD so libssh won't close the socket in ssh_disconnect() */
+    ssh_options_set(session->ti.libssh.session, SSH_OPTIONS_FD, &sock);
     sock = -1;
 
     /* set to non-blocking */