lib/libssh
1
0
Fork 0
mirror of https://git.libssh.org/projects/libssh.git synced 2025-12-08 03:42:12 +03:00
Code Activity

sftp: Avoid dead code and possible overruns

Browse Source 
Coverity did not like the DEADCODE on 64b architecture.

After better look, the SFTP packet have read/write lengths in uint32 so
we really can not use the limits up to the uint64. Therefore we cap the
limits to uint32 while parsing and ignore any large value.

We then cap our reads/writes to this value, which is safe to cast to uint32
for wire format.

Thanks Coverity CID 1589435, CID 1589434, CID 1589432, CID 1589431

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
This commit is contained in:
Jakub Jelen
2025-01-13 14:57:17 +01:00
parent 9eaa7a6394
commit d887e1320a
2 changed files with 46 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
src/sftp.c
View File

@@ -1140,7 +1140,7 @@ sftp_read(sftp_file handle, void *buf, size_t count)
ssh_string datastring = NULL; ssh_string datastring = NULL;
size_t datalen; size_t datalen;
ssh_buffer buffer = NULL; ssh_buffer buffer = NULL;
uint32_t id; uint32_t id, read_len;
int rc; int rc;
if (handle == NULL) { if (handle == NULL) {
@@ -1159,13 +1159,11 @@ sftp_read(sftp_file handle, void *buf, size_t count)
* *
* TODO: We should iterate over the blocks rather than writing less than * TODO: We should iterate over the blocks rather than writing less than
* requested to provide less surprises to the calling applications. * requested to provide less surprises to the calling applications.
*
* The limits are in theory uint64, but packet contain data length in uint32
* so in practice, the limit will never be larger than UINT32_MAX
*/ */
if (count > sftp->limits->max_read_length) { read_len = (uint32_t)MIN(sftp->limits->max_read_length, count);
if (sftp->limits->max_read_length > SIZE_MAX) {
return SSH_ERROR;
}
count = (size_t)sftp->limits->max_read_length;
}
buffer = ssh_buffer_new(); buffer = ssh_buffer_new();
if (buffer == NULL) { if (buffer == NULL) {
@@ -1180,7 +1178,7 @@ sftp_read(sftp_file handle, void *buf, size_t count)
id, id,
handle->handle, handle->handle,
handle->offset, handle->offset,
count); read_len);
if (rc != SSH_OK) { if (rc != SSH_OK) {
ssh_set_error_oom(sftp->session); ssh_set_error_oom(sftp->session);
SSH_BUFFER_FREE(buffer); SSH_BUFFER_FREE(buffer);
@@ -1392,7 +1390,7 @@ sftp_write(sftp_file file, const void *buf, size_t count)
sftp_message msg = NULL; sftp_message msg = NULL;
sftp_status_message status; sftp_status_message status;
ssh_buffer buffer = NULL; ssh_buffer buffer = NULL;
uint32_t id; uint32_t id, write_len;
ssize_t len; ssize_t len;
size_t packetlen; size_t packetlen;
int rc; int rc;
@@ -1418,21 +1416,20 @@ sftp_write(sftp_file file, const void *buf, size_t count)
* *
* TODO: We should iterate over the blocks rather than writing less than * TODO: We should iterate over the blocks rather than writing less than
* requested to provide less surprises to the calling applications. * requested to provide less surprises to the calling applications.
*
* The limits are in theory uint64, but packet contain data length in uint32
* so in practice, the limit will never be larger than UINT32_MAX
*/ */
if (count > sftp->limits->max_write_length) { write_len = (uint32_t)MIN(sftp->limits->max_write_length, count);
if (sftp->limits->max_write_length > SIZE_MAX) {
return SSH_ERROR;
}
count = (size_t)sftp->limits->max_write_length;
}
rc = ssh_buffer_pack(buffer, rc = ssh_buffer_pack(buffer,
"dSqdP", "dSqdP",
id, id,
file->handle, file->handle,
file->offset, file->offset,
count, /* len of datastring */ write_len, /* len of datastring */
(size_t)count, buf); (size_t)write_len,
buf);
if (rc != SSH_OK) { if (rc != SSH_OK) {
ssh_set_error_oom(sftp->session); ssh_set_error_oom(sftp->session);
SSH_BUFFER_FREE(buffer); SSH_BUFFER_FREE(buffer);
@@ -1464,9 +1461,9 @@ sftp_write(sftp_file file, const void *buf, size_t count)
sftp_set_error(sftp, status->status); sftp_set_error(sftp, status->status);
switch (status->status) { switch (status->status) {
case SSH_FX_OK: case SSH_FX_OK:
file->offset += count; file->offset += write_len;
status_msg_free(status); status_msg_free(status);
return count; return write_len;
default: default:
break; break;
} }
@@ -1474,7 +1471,7 @@ sftp_write(sftp_file file, const void *buf, size_t count)
SSH_REQUEST_DENIED, SSH_REQUEST_DENIED,
"SFTP server: %s", "SFTP server: %s",
status->errormsg); status->errormsg);
file->offset += count; file->offset += write_len;
status_msg_free(status); status_msg_free(status);
return -1; return -1;
default: default:
@@ -2687,6 +2684,11 @@ static sftp_limits_t sftp_parse_limits(sftp_session sftp, ssh_buffer buf)
sftp_set_error(sftp, SSH_FX_FAILURE); sftp_set_error(sftp, SSH_FX_FAILURE);
return NULL; return NULL;
} }
/* cap the max read and write length to UINT32_MAX as we really can not read
* nor write more as the len member of the SSH_FXP_READ/WRITE packets is
* uint32 */
limits->max_read_length = MIN(limits->max_read_length, UINT32_MAX);
limits->max_write_length = MIN(limits->max_write_length, UINT32_MAX);
return limits; return limits;
} }

49
src/sftp_aio.c
View File

@@ -55,7 +55,7 @@ ssize_t sftp_aio_begin_read(sftp_file file, size_t len, sftp_aio *aio)
sftp_session sftp = NULL; sftp_session sftp = NULL;
ssh_buffer buffer = NULL; ssh_buffer buffer = NULL;
sftp_aio aio_handle = NULL; sftp_aio aio_handle = NULL;
uint32_t id; uint32_t id, read_len;
int rc; int rc;
if (file == NULL || if (file == NULL ||
@@ -73,13 +73,12 @@ ssize_t sftp_aio_begin_read(sftp_file file, size_t len, sftp_aio *aio)
return SSH_ERROR; return SSH_ERROR;
} }
/* Apply a cap on the length a user is allowed to read */ /* Apply a cap on the length a user is allowed to read
if (len > sftp->limits->max_read_length) { *
if (sftp->limits->max_read_length > SIZE_MAX) { * The limits are in theory uint64, but packet contain data length in uint32
return SSH_ERROR; * so in practice, the limit will never be larger than UINT32_MAX
} */
len = (size_t)sftp->limits->max_read_length; read_len = (uint32_t)MIN(sftp->limits->max_read_length, len);
}
if (aio == NULL) { if (aio == NULL) {
ssh_set_error(sftp->session, SSH_FATAL, ssh_set_error(sftp->session, SSH_FATAL,
@@ -103,7 +102,7 @@ ssize_t sftp_aio_begin_read(sftp_file file, size_t len, sftp_aio *aio)
id, id,
file->handle, file->handle,
file->offset, file->offset,
len); read_len);
if (rc != SSH_OK) { if (rc != SSH_OK) {
ssh_set_error_oom(sftp->session); ssh_set_error_oom(sftp->session);
@@ -122,7 +121,7 @@ ssize_t sftp_aio_begin_read(sftp_file file, size_t len, sftp_aio *aio)
aio_handle->file = file; aio_handle->file = file;
aio_handle->id = id; aio_handle->id = id;
aio_handle->len = len; aio_handle->len = read_len;
rc = sftp_packet_write(sftp, SSH_FXP_READ, buffer); rc = sftp_packet_write(sftp, SSH_FXP_READ, buffer);
SSH_BUFFER_FREE(buffer); SSH_BUFFER_FREE(buffer);
@@ -132,9 +131,9 @@ ssize_t sftp_aio_begin_read(sftp_file file, size_t len, sftp_aio *aio)
} }
/* Assume we read len bytes from the file */ /* Assume we read len bytes from the file */
file->offset += len; file->offset += read_len;
*aio = aio_handle; *aio = aio_handle;
return len; return read_len;
} }
ssize_t sftp_aio_wait_read(sftp_aio *aio, ssize_t sftp_aio_wait_read(sftp_aio *aio,
@@ -312,7 +311,7 @@ ssize_t sftp_aio_begin_write(sftp_file file,
sftp_session sftp = NULL; sftp_session sftp = NULL;
ssh_buffer buffer = NULL; ssh_buffer buffer = NULL;
sftp_aio aio_handle = NULL; sftp_aio aio_handle = NULL;
uint32_t id; uint32_t id, write_len;
int rc; int rc;
if (file == NULL || if (file == NULL ||
@@ -338,13 +337,12 @@ ssize_t sftp_aio_begin_write(sftp_file file,
return SSH_ERROR; return SSH_ERROR;
} }
/* Apply a cap on the length a user is allowed to write */ /* Apply a cap on the length a user is allowed to write
if (len > sftp->limits->max_write_length) { *
if (sftp->limits->max_write_length > SIZE_MAX) { * The limits are in theory uint64, but packet contain data length in uint32
return SSH_ERROR; * so in practice, the limit will never be larger than UINT32_MAX
} */
len = (size_t)sftp->limits->max_write_length; write_len = (uint32_t)MIN(sftp->limits->max_write_length, len);
}
if (aio == NULL) { if (aio == NULL) {
ssh_set_error(sftp->session, SSH_FATAL, ssh_set_error(sftp->session, SSH_FATAL,
@@ -367,8 +365,9 @@ ssize_t sftp_aio_begin_write(sftp_file file,
id, id,
file->handle, file->handle,
file->offset, file->offset,
len, /* len of datastring */ write_len, /* len of datastring */
len, buf); (size_t)write_len,
buf);
if (rc != SSH_OK) { if (rc != SSH_OK) {
ssh_set_error_oom(sftp->session); ssh_set_error_oom(sftp->session);
@@ -387,7 +386,7 @@ ssize_t sftp_aio_begin_write(sftp_file file,
aio_handle->file = file; aio_handle->file = file;
aio_handle->id = id; aio_handle->id = id;
aio_handle->len = len; aio_handle->len = write_len;
rc = sftp_packet_write(sftp, SSH_FXP_WRITE, buffer); rc = sftp_packet_write(sftp, SSH_FXP_WRITE, buffer);
SSH_BUFFER_FREE(buffer); SSH_BUFFER_FREE(buffer);
@@ -397,9 +396,9 @@ ssize_t sftp_aio_begin_write(sftp_file file,
} }
/* Assume we wrote len bytes to the file */ /* Assume we wrote len bytes to the file */
file->offset += len; file->offset += write_len;
*aio = aio_handle; *aio = aio_handle;
return len; return write_len;
} }
ssize_t sftp_aio_wait_write(sftp_aio *aio) ssize_t sftp_aio_wait_write(sftp_aio *aio)