Skip to content

Conversation

@rlaager
Copy link
Contributor

@rlaager rlaager commented Jul 11, 2025

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'

I also fixed a variable type while I was there. This is a second commit.

rlaager added 2 commits July 11, 2025 18:43
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 <[email protected]>
strlen() returns a size_t.

Signed-off-by: Richard Laager <[email protected]>
@rlaager rlaager force-pushed the fix-check_ssh-buffer-overflow branch from 74a1b57 to 1f2acfd Compare July 11, 2025 23:44
@waja waja added this to the 2.4.1 milestone Jul 12, 2025
@waja waja added the check_ssh label Jul 12, 2025
@RincewindsHat
Copy link
Member

Hi @rlaager,
Thanks, that looks reasonable on a first glance. I will take a look into it in time.

@RincewindsHat
Copy link
Member

@rlaager, thanks for fix and the analysis. I failed to replicate the overflow situation, but I do see the "off by one" situation.

@RincewindsHat RincewindsHat merged commit b05087d into monitoring-plugins:master Aug 1, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants