Fix check ssh buffer overflow #2133
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A buffer overflow was occurring when the server responded with:
Exceeded MaxStartups\r\nglibc would then abort() with the following output:
*** buffer overflow detected ***: terminatedIt 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 1again 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 ofthe 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:
And, after the memmove(), add:
If you fix the memset() buffer overflow, it will output:
As you can see, the first character is lost.
If you then fix the memmove(), it will output:
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:
I also fixed a variable type while I was there. This is a second commit.