Update to 18.4.1 #81
Merged
Update to 18.4.1 #81
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We need to ensure that cc20-mt and cc20-serial are in the kex proposal string. However, the prior method created a small memory leak. This patch resolves that leak. Passes regression and fuzz testing.
Trying to assign nulls to values in ssh-rsa.c ssh_rsa_equal.
Trying to assign nulls to values in ssh-dss.c ssh_dss_equal.
This change explicitly allows the use of OpenSSL 3.4. Configure.ac has a switch statement that depends on the reported version of OpenSSL. We need to update these tests for each point release of OpenSSL.
This reverts commit 3db1f6e.
This reverts commit 9abcc4e.
Three primary changes: 1) Reduce SSHBUF_MAX_SIZE to 128MB. This may impact performance so it's subject to change. 2) Only grow the channel input and channel output buffers in sshbuf_allocate. Right now I'm doing a string match on the labels. If this ends up being something we'll use long term we'll want to make the labels an enum for faster comparisons. 3) In sshbuf_check_reserve we are no longer checking aganst max_size/2 - len <= size - offset but against max_size - len. This seems to prevent the output buffer from thowing SSH_ERR_NO_BUFFER_SPACE. This has not been fully tested and may not be the right solution.
This patch extends the sshbuf struct by adding a type field (int). This field is populated by entries from the sshbuf_types enum. This is set via the sshbuf_type() function. This type field is used in sshbuff_allocate to determine if a buffer (channel input and output) need to be grown to support auto-tuning.
I realized that the existing sshbuf.c might have had multiple conflicting attempts to fix issues found in previous versions. So I started from the original sshbuf.c and added the accelerated buffer growth back into it. This seems to work in that it's not trying to grow past the maximum buffer size and it's passing the regression tests.
/* we need to reserve a small amount of overhead on the input buffer
* or we can enter into a pathological state during bulk
* data transfers. We use a fraction of the max size as we want it to scale
* with the size of the input buffer. If we do it for all of the buffers
* we fail the regression unit tests. This seems like a reasonable
* solution. Of course, I still need to figure out *why* this is
* happening and come up with an actual fix. TODO
* cjr 4/19/2024 */
Removed references to items that are no longer implemented. Removed the timestamp element of the sshbuf struct. Removed time_diff from sshbuf.c as that used the buffer time stamp.
I was passing a string instead of the int from the enum. This version also tries to start the oss-fuzz integration.
The HPNBufferLimit option was intended to clamp the inbound flow control buffer to half of the available window. Unfortunately, it looks like it's been broken for some time with a tendancy to reduce the window to zero and cut the connection. As this option was specifically to deal with one special case and it's broken, fixing it doesn't seem like the right thing to do.
|
MWD has too much on their plate at the moment so we are skipping the second review for this. Pushing this out to RC status on my authority. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes an issue with the channel output buffer growing to unsustainable sizes causing disconnects in some situations. It also results in sshbuf code that more closely conforms to the OpenSSH source while retaining the buffer enhancements. This passes all regression tests, unit tests, and functionality tests.
Additionally, We've remove the HPNBufferLimit option as it turns out that it wasn't working as advertised. More often than not it would cause an immediate connection termination. As this option is not widely used, if at all, I feel that removing it is the best course of action.
Also, minor fixes including a small memory leak in kex.c and compatibility for OpenSSL 3.4.