diff --git a/include/libssh/sftpserver.h b/include/libssh/sftpserver.h index 01b17c7f..9982830c 100644 --- a/include/libssh/sftpserver.h +++ b/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) diff --git a/src/sftpserver.c b/src/sftpserver.c index 7bca414d..b3ae409d 100644 --- a/src/sftpserver.c +++ b/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; diff --git a/tests/server/torture_sftpserver.c b/tests/server/torture_sftpserver.c index 5e431c4e..3e63e489 100644 --- a/tests/server/torture_sftpserver.c +++ b/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)