-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only free the read buffers if we're not using them #24394
Conversation
If we're part way through processing a record, or the application has not released all the records then we should not free our buffer because they are still needed. CVE-2024-4741
In order to ensure we do not have a UAF we reset the rl->packet pointer to NULL after we free it. Follow on from CVE-2024-4741
Test that attempting to free the buffers at points where they should not be freed works as expected. Follow on from CVE-2024-4741
The sslapitest has a helper function to load the dasync engine which is useful for testing pipelining. We would like to have the same facility from sslbuffertest, so we move the function to the common location ssltestlib.c Follow on from CVE-2024-4741
We extend the testing to test what happens when pipelining is in use. Follow on from CVE-2024-4741
See 3.1/3.0 backport in #24395 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question...
@@ -2129,7 +2137,10 @@ int tls_free_buffers(OSSL_RECORD_LAYER *rl) | |||
/* Read direction */ | |||
|
|||
/* If we have pending data to be read then fail */ | |||
if (rl->curr_rec < rl->num_recs || TLS_BUFFER_get_left(&rl->rbuf) != 0) | |||
if (rl->curr_rec < rl->num_recs | |||
|| rl->curr_rec != rl->num_released |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have question: is rl->curr_rec != rl->num_released
the right cmp operation here? The reason I'm asking is that elsewhere in this file we use a <
comparison. For example here tls_release_record()
:
1155 int tls_release_record(OSSL_RECORD_LAYER *rl, void *rechandle, size_t length)
1156 {
1157 TLS_RL_RECORD *rec = &rl->rrec[rl->num_released];
1158
1159 if (!ossl_assert(rl->num_released < rl->curr_rec)
1160 || !ossl_assert(rechandle == rec)) {
1161 /* Should not happen */
perhaps comparison type does not matter in this case. I'm rather curious if there is a reason for inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rl->num_released
should always be <= rl->curr_rec
.
In the case of the code that you reference we are asserting the stricter condition that rl->num_released < rl->curr_rec
since it would be a libssl bug to call tls_release_record
where rl->num_released >= rl->curr_rec
.
In this case it is the other way around. This function must fail if rl->curr_rec != rl->num_released
. It would be sufficient to fail if rl->num_released < rl->curr_rec
if we rely on the fact that rl->num_released
should never be greater than rl->curr_rec
- but I prefer the stricter check here.
Pushed. Thanks. |
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.
CVE-2024-4741