From 661ecff45c5f4c41c22ef9fd4fe308100b97d6bf Mon Sep 17 00:00:00 2001 From: Richard Laager Date: Fri, 11 Jul 2025 18:19:31 -0500 Subject: check_ssh: Fix buffer overflow A buffer overflow was occurring when the server responded with: Exceeded MaxStartups\r\n glibc would then abort() with the following output: *** buffer overflow detected ***: terminated It was the memset() that was overflowing the buffer. But the memmove() needed fixing too. First off, there was an off-by-one error in both the memmove() and memset(). byte_offset was already set to the start of the data _past_ the newline (i.e. len + 1). For the memmove(), incrementing that by 1 again lost the first character of the additional output. For the memset(), this causes a buffer overflow. Second, the memset() has multiple issues. The comment claims that it was NULing (sic "null") the "rest". However, it has no idea how long the "rest" is, at this point. It was NULing BUFF_SZ - byte_offset + 1. After fixing the off-by-one / buffer overflow, it would be NULing BUFF_SZ - byte_offset. But that doesn't make any sense. The length of the first line has no relation to the length of the second line. For a quick-and-dirty test, add something like this just inside the while loop: memcpy(output, "Exceeded MaxStartups\r\nnext blah1 blah2 blah3 blah4\0", sizeof("Exceeded MaxStartups\r\nnext blah1 blah2 blah3 blah4\0")); And, after the memmove(), add: printf("output='%s'\n", output); If you fix the memset() buffer overflow, it will output: output='ext blah1 blah2 blah3 ' As you can see, the first character is lost. If you then fix the memmove(), it will output: output='next blah1 blah2 blah3' Note that this is still losing the "blah4". After moving the memset() after byte_offset is set to the new strlen() of output, then it works correctly: output='next blah1 blah2 blah3 blah4' Signed-off-by: Richard Laager --- plugins/check_ssh.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/plugins/check_ssh.c b/plugins/check_ssh.c index 9d0d7cde..fd082e11 100644 --- a/plugins/check_ssh.c +++ b/plugins/check_ssh.c @@ -273,12 +273,14 @@ int ssh_connect(mp_check *overall, char *haddr, int hport, char *desired_remote_ } if (version_control_string == NULL) { - /* move unconsumed data to beginning of buffer, null rest */ - memmove((void *)output, (void *)(output + byte_offset + 1), BUFF_SZ - len + 1); - memset(output + byte_offset + 1, 0, BUFF_SZ - byte_offset + 1); + /* move unconsumed data to beginning of buffer */ + memmove((void *)output, (void *)(output + byte_offset), BUFF_SZ - byte_offset); /*start reading from end of current line chunk on next recv*/ byte_offset = strlen(output); + + /* NUL the rest of the buffer */ + memset(output + byte_offset, 0, BUFF_SZ - byte_offset); } } else { byte_offset += recv_ret; -- cgit v1.2.3-74-g34f1 From 1f2acfd1c6577db6e3d385614922e32ac9fad03f Mon Sep 17 00:00:00 2001 From: Richard Laager Date: Fri, 11 Jul 2025 18:38:42 -0500 Subject: check_ssh: Correct type on len variable strlen() returns a size_t. Signed-off-by: Richard Laager --- plugins/check_ssh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/check_ssh.c b/plugins/check_ssh.c index fd082e11..2c76fa84 100644 --- a/plugins/check_ssh.c +++ b/plugins/check_ssh.c @@ -255,7 +255,7 @@ int ssh_connect(mp_check *overall, char *haddr, int hport, char *desired_remote_ byte_offset = 0; char *index = NULL; - unsigned long len = 0; + size_t len = 0; while ((index = strchr(output + byte_offset, '\n')) != NULL) { /*Partition the buffer so that this line is a separate string, * by replacing the newline with NUL*/ -- cgit v1.2.3-74-g34f1