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 |
| @@ -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.
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.
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