Fix SSL sessions returning uninitialized memory - #124
Merged
Conversation
SeanTAllen
force-pushed
the
issue-123-read-buf-size
branch
from
August 1, 2026 16:29
ae31dde to
c369b1e
Compare
SeanTAllen
force-pushed
the
issue-123-read-buf-size
branch
from
August 1, 2026 16:33
c369b1e to
ce4f8cd
Compare
`read` sized `_read_buf` before the decrypt and then treated its size as the count of bytes decrypted. Two of its exits left those apart, so a caller got uninitialized heap as application data. It had two `SSL_read` call sites and nothing made them agree: one truncated on some exits, the other on none. Collapsing them to one call site is what makes a path that skips the reconciliation impossible to write, rather than adding a second truncate a later edit can miss again. `send` is the same defect against `BIO_read`. Nothing says why it would be allowed to return bytes OpenSSL did not write while `read` is not, so it is fixed here too. Its narrowed length is #121's and is untouched. Matching `SSL_get_error` results on bare integers needed a comment to be readable, so the results are named instead. That covers `receive` as well as `read`. The dead `max` binding sat inside the rewritten sizing logic, so it goes with it. Closes #123 Closes #117
SeanTAllen
force-pushed
the
issue-123-read-buf-size
branch
from
August 1, 2026 16:37
ce4f8cd to
d8e7a2b
Compare
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
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.
SSL.readsized its buffer from the length it was about to request, then returned that size as the number of bytes decrypted. On two of its exits those numbers did not match, and the caller got uninitialized heap back as application data.SSL.sendhad the same defect againstBIO_readand gets the same fix.readalso had twoSSL_readcall sites, one that truncated the buffer on some exits and one that truncated on none. It has one call site and one truncate now, so a path that skips the truncate can't be written.SSL_get_errorresults were matched as bare integers inreadandreceive; they are named values on a_SSLErrorCodeprimitive now. The deadmaxbinding #117 names was part of the sizing logic being rewritten, so it comes out here too.The
if pending > 0clamp is an allocation bound now, not a correctness guard. ShrinkinglentoSSL_pending's count used to be what kept the returned size right. The size comes offSSL_read's return on every path now, so the clamp only bounds the allocation. Delete the whole block and every test still passes with every returned byte correct.Reaching the two new lines in
sendtakes two gibibytes or more of queued ciphertext, the case #121 narrows to. Both lines can be deleted with the full suite green.The missing
elseon theSSL_get_errormatch, #120, is now reachable from a path that could not reach it before. Previously only theSSL_pending == 0path reached it.Five open issues cite
ssl/net/ssl.ponyline numbers we move here: #118, #119, #120, #121, #122. Renumbering isn't enough for #121. It describesreadas having twoSSL_readcall sites and sayssendreturns uninitialized heap to the peer, and neither holds after this.Closes #123
Closes #117