sftp: Harmonize return values from sftp server callbacks

Terminate also the SFTP server when the fatal error comes from the message
handlers.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
Reviewed-by: Pavol Žáčik <pzacik@redhat.com>
Merge-Request: <https://gitlab.com/libssh/libssh-mirror/-/merge_requests/849>
(cherry picked from commit 1726018198)

include/libssh/sftpserver.h

@@ -43,6 +43,15 @@ extern "C" {
  * @{
  */
 
+/**
+ * @brief Macro to declare an SFTP message callback function.
+ *
+ * @param name  The name of the callback function to declare.
+ *
+ * @return SSH_OK for properly processed messages (including errors reported to
+ *         client over the channel and SSH_ERROR for protocol errors
+ *         that the channel callback should treat as fatal.
+ */
 #define SSH_SFTP_CALLBACK(name) \
     static int name(sftp_client_message message)
 

src/sftpserver.c

@@ -1160,7 +1160,7 @@ process_open(sftp_client_message client_msg)
                 strerror(saved_errno));
         status = unix_errno_to_ssh_stat(saved_errno);
         sftp_reply_status(client_msg, status, "Write error");
-        return SSH_ERROR;
+        return SSH_OK;
     }
 
     h = calloc(1, sizeof (struct sftp_handle));
@@ -1219,7 +1219,7 @@ process_read(sftp_client_message client_msg)
         SSH_LOG(SSH_LOG_PROTOCOL,
                 "error seeking file fd: %d at offset: %" PRIu64,
                 fd, client_msg->offset);
-        return SSH_ERROR;
+        return SSH_OK;
     }
 
     buffer = malloc(client_msg->len);
@@ -1234,7 +1234,7 @@ process_read(sftp_client_message client_msg)
         sftp_reply_status(client_msg, SSH_FX_FAILURE, NULL);
         SSH_LOG(SSH_LOG_PROTOCOL, "read file error!");
         free(buffer);
-        return SSH_ERROR;
+        return SSH_OK;
     } else if (readn > 0) {
         sftp_reply_data(client_msg, buffer, readn);
     } else {
@@ -1280,13 +1280,13 @@ process_write(sftp_client_message client_msg)
         SSH_LOG(SSH_LOG_PROTOCOL,
                 "error seeking file at offset: %" PRIu64,
                 client_msg->offset);
-        return SSH_ERROR;
+        return SSH_OK;
     }
     written = ssh_writen(fd, msg_data, len);
     if (written != (ssize_t)len) {
         sftp_reply_status(client_msg, SSH_FX_FAILURE, "Write error");
         SSH_LOG(SSH_LOG_PROTOCOL, "file write error!");
-        return SSH_ERROR;
+        return SSH_OK;
     }
 
     sftp_reply_status(client_msg, SSH_FX_OK, NULL);
@@ -1333,7 +1333,7 @@ process_close(sftp_client_message client_msg)
         sftp_reply_status(client_msg, SSH_FX_BAD_MESSAGE, "Invalid handle");
     }
 
-    return SSH_OK;
+    return ret;
 }
 
 static int
@@ -1355,7 +1355,7 @@ process_opendir(sftp_client_message client_msg)
     dir = opendir(dir_name);
     if (dir == NULL) {
         sftp_reply_status(client_msg, SSH_FX_NO_SUCH_FILE, "No such directory");
-        return SSH_ERROR;
+        return SSH_OK;
     }
 
     h = calloc(1, sizeof (struct sftp_handle));
@@ -1563,7 +1563,6 @@ process_readdir(sftp_client_message client_msg)
 static int
 process_mkdir(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *filename = sftp_client_message_get_filename(client_msg);
     uint32_t msg_flags = client_msg->attr->flags;
     uint32_t permission = client_msg->attr->permissions;
@@ -1586,18 +1585,16 @@ process_mkdir(sftp_client_message client_msg)
         int saved_errno = errno;
         SSH_LOG(SSH_LOG_PROTOCOL, "failed to mkdir: %s", strerror(saved_errno));
         status = unix_errno_to_ssh_stat(saved_errno);
-        ret = SSH_ERROR;
     }
 
     sftp_reply_status(client_msg, status, NULL);
 
-    return ret;
+    return SSH_OK;
 }
 
 static int
 process_rmdir(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *filename = sftp_client_message_get_filename(client_msg);
     int status = SSH_FX_OK;
     int rv;
@@ -1613,12 +1610,11 @@ process_rmdir(sftp_client_message client_msg)
     rv = rmdir(filename);
     if (rv < 0) {
         status = unix_errno_to_ssh_stat(errno);
-        ret = SSH_ERROR;
     }
 
     sftp_reply_status(client_msg, status, NULL);
 
-    return ret;
+    return SSH_OK;
 }
 
 static int
@@ -1657,7 +1653,6 @@ process_realpath(sftp_client_message client_msg)
 static int
 process_lstat(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *filename = sftp_client_message_get_filename(client_msg);
     struct sftp_attributes_struct attr;
     struct stat st;
@@ -1678,19 +1673,17 @@ process_lstat(sftp_client_message client_msg)
         SSH_LOG(SSH_LOG_PROTOCOL, "lstat failed: %s", strerror(saved_errno));
         status = unix_errno_to_ssh_stat(saved_errno);
         sftp_reply_status(client_msg, status, NULL);
-        ret = SSH_ERROR;
     } else {
         stat_to_filexfer_attrib(&st, &attr);
         sftp_reply_attr(client_msg, &attr);
     }
 
-    return ret;
+    return SSH_OK;
 }
 
 static int
 process_stat(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *filename = sftp_client_message_get_filename(client_msg);
     struct sftp_attributes_struct attr;
     struct stat st;
@@ -1711,20 +1704,18 @@ process_stat(sftp_client_message client_msg)
         SSH_LOG(SSH_LOG_PROTOCOL, "lstat failed: %s", strerror(saved_errno));
         status = unix_errno_to_ssh_stat(saved_errno);
         sftp_reply_status(client_msg, status, NULL);
-        ret = SSH_ERROR;
     } else {
         stat_to_filexfer_attrib(&st, &attr);
         sftp_reply_attr(client_msg, &attr);
     }
 
-    return ret;
+    return SSH_OK;
 }
 
 static int
 process_setstat(sftp_client_message client_msg)
 {
     int rv;
-    int ret = SSH_OK;
     int status = SSH_FX_OK;
     uint32_t msg_flags = client_msg->attr->flags;
     const char *filename = sftp_client_message_get_filename(client_msg);
@@ -1746,7 +1737,7 @@ process_setstat(sftp_client_message client_msg)
                     strerror(saved_errno));
             status = unix_errno_to_ssh_stat(saved_errno);
             sftp_reply_status(client_msg, status, NULL);
-            return rv;
+            return SSH_OK;
         }
     }
 
@@ -1759,7 +1750,7 @@ process_setstat(sftp_client_message client_msg)
                     strerror(saved_errno));
             status = unix_errno_to_ssh_stat(saved_errno);
             sftp_reply_status(client_msg, status, NULL);
-            return rv;
+            return SSH_OK;
         }
     }
 
@@ -1772,7 +1763,7 @@ process_setstat(sftp_client_message client_msg)
                     strerror(saved_errno));
             status = unix_errno_to_ssh_stat(saved_errno);
             sftp_reply_status(client_msg, status, NULL);
-            return rv;
+            return SSH_OK;
         }
     }
 
@@ -1793,7 +1784,7 @@ process_setstat(sftp_client_message client_msg)
                     strerror(saved_errno));
             status = unix_errno_to_ssh_stat(saved_errno);
             sftp_reply_status(client_msg, status, NULL);
-            return rv;
+            return SSH_OK;
         }
 #else
         struct _utimbuf tf;
@@ -1809,19 +1800,18 @@ process_setstat(sftp_client_message client_msg)
                     strerror(saved_errno));
             status = unix_errno_to_ssh_stat(saved_errno);
             sftp_reply_status(client_msg, status, NULL);
-            return rv;
+            return SSH_OK;
         }
 #endif
     }
 
     sftp_reply_status(client_msg, status, NULL);
-    return ret;
+    return SSH_OK;
 }
 
 static int
 process_readlink(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *filename = sftp_client_message_get_filename(client_msg);
     char buf[PATH_MAX];
     int len = -1;
@@ -1843,13 +1833,12 @@ process_readlink(sftp_client_message client_msg)
         status = unix_errno_to_ssh_stat(saved_errno);
         err_msg = ssh_str_error(status);
         sftp_reply_status(client_msg, status, err_msg);
-        ret = SSH_ERROR;
     } else {
         buf[len] = '\0';
         sftp_reply_name(client_msg, buf, NULL);
     }
 
-    return ret;
+    return SSH_OK;
 }
 
 /* Note, that this function is using reversed order of the arguments than the
@@ -1860,7 +1849,6 @@ process_readlink(sftp_client_message client_msg)
 static int
 process_symlink(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *destpath = sftp_client_message_get_filename(client_msg);
     const char *srcpath = ssh_string_get_char(client_msg->data);
     int status = SSH_FX_OK;
@@ -1881,18 +1869,16 @@ process_symlink(sftp_client_message client_msg)
         status = unix_errno_to_ssh_stat(saved_errno);
         SSH_LOG(SSH_LOG_PROTOCOL, "symlink failed: %s", strerror(saved_errno));
         sftp_reply_status(client_msg, status, "Write error");
-        ret = SSH_ERROR;
     } else {
         sftp_reply_status(client_msg, SSH_FX_OK, "write success");
     }
 
-    return ret;
+    return SSH_OK;
 }
 
 static int
 process_remove(sftp_client_message client_msg)
 {
-    int ret = SSH_OK;
     const char *filename = sftp_client_message_get_filename(client_msg);
     int rv;
     int status = SSH_FX_OK;
@@ -1910,12 +1896,11 @@ process_remove(sftp_client_message client_msg)
         int saved_errno = errno;
         SSH_LOG(SSH_LOG_PROTOCOL, "unlink failed: %s", strerror(saved_errno));
         status = unix_errno_to_ssh_stat(saved_errno);
-        ret = SSH_ERROR;
     }
 
     sftp_reply_status(client_msg, status, NULL);
 
-    return ret;
+    return SSH_OK;
 }
 
 static int
@@ -1952,7 +1937,7 @@ process_extended_statvfs(sftp_client_message client_msg)
         SSH_LOG(SSH_LOG_PROTOCOL, "statvfs failed: %s", strerror(saved_errno));
         status = unix_errno_to_ssh_stat(saved_errno);
         sftp_reply_status(client_msg, status, NULL);
-        return SSH_ERROR;
+        return SSH_OK;
     }
 
     sftp_statvfs = calloc(1, sizeof(struct sftp_statvfs_struct));
@@ -2138,6 +2123,10 @@ sftp_channel_default_data_callback(UNUSED_PARAM(ssh_session session),
         sftp_client_message_free(msg);
         if (rc != SSH_OK) {
             SSH_LOG(SSH_LOG_PROTOCOL, "process sftp failed!");
+            ssh_channel_send_eof(sftp->channel);
+            ssh_channel_close(sftp->channel);
+            // Leave freeing resources on caller
+            return rc;
         }
 
         undecoded_len -= decode_len;

tests/server/torture_sftpserver.c

@@ -1271,6 +1271,9 @@ static void torture_server_sftp_handle_overrun(void **state)
     handle = sftp_open(sftp, name, O_WRONLY | O_CREAT, 0700);
     assert_non_null(handle);
 
+    rc = sftp_close(handle);
+    assert_int_equal(rc, SSH_OK);
+
     /* Craft an malicious SFTP packet trying to access handle 256
      * (SFTP_HANDLES) */
     buffer = ssh_buffer_new();
@@ -1297,9 +1300,6 @@ static void torture_server_sftp_handle_overrun(void **state)
     sftp_message_free(msg);
     assert_int_equal(status->status, SSH_FX_INVALID_HANDLE);
     status_msg_free(status);
-
-    rc = sftp_close(handle);
-    assert_int_equal(rc, SSH_OK);
 }
 
 static void torture_server_sftp_payload_overrun(void **state)