Fix leak in SSL_set_tlsext_status_ocsp_resp#28894
Closed
rgacogne wants to merge 1 commit intoopenssl:masterfrom
Closed
Fix leak in SSL_set_tlsext_status_ocsp_resp#28894rgacogne wants to merge 1 commit intoopenssl:masterfrom
SSL_set_tlsext_status_ocsp_resp#28894rgacogne wants to merge 1 commit intoopenssl:masterfrom
Conversation
Contributor
Author
|
I see some tests are failing, so clearly this is not as straight-forward as it seemed in my own case. I'll look into it. |
t8m
added
branch: master
Member
The tests are failing because the test case was incorrectly changed in b1b4b15#diff-ac041dcce48346a57d70403803133a58fb4ac5764e173d7cbfd6db2c2df5afdcR1943 This free call should be removed. |
t8m
requested changes
Oct 14, 2025
ssl/s3_lib.c
Outdated
Comment on lines
3718
to
3719
|
|
||
| OPENSSL_free(parg); |
Member
There was a problem hiding this comment.
I think this would be a better (more backwards compatible with pre-3.6 versions) fix.
Suggested change
| OPENSSL_free(parg); | |
| sc->ext.ocsp.resp = parg; | |
| sc->ext.ocsp.resp_len = larg; |
Contributor
Author
There was a problem hiding this comment.
This is fine by me, but is it OK to have both sc->ext.ocsp.resp and sc->ext.ocsp.resp_ex set? The current code seems to try hard to avoid ending up in that situation, although it's not clear to me whether resp is still actually used.
Contributor
Author
There was a problem hiding this comment.
Done. I successfully tested the change locally.
The changes introduced in b1b4b15 cause the `resp` parameter to no longer be assigned to the `SSL` object. Since it is not freed either, and the caller used to expect that, the memory is now leaked. This commit assigns the value to the `SSL` object. There is an early error return when `sk_OCSP_RESPONSE_new_reserve` fails where the memory is not freed. I'm not sure whether it makes sense to free the memory in that case, as the documentation does not mention anything. Fixes openssl#28888 CLA: trivial Signed-off-by: Remi Gacogne <[email protected]>
Sashan
added
the
approval: review pending
Sashan
approved these changes
Oct 14, 2025
Contributor
Sashan
left a comment
Sashan
left a comment
There was a problem hiding this comment.
I agree with CLA trivial.
thank you for the fix.
InfoHunter
approved these changes
Oct 18, 2025
Member
|
Ping @t8m for a confirmation |
Collaborator
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
t8m
added a commit
to t8m/openssl
that referenced
this pull request
Nov 28, 2025
Fixes openssl#28888 Fixes b1b4b15 Instead of transferring the ownership of the single OCSP response to the SSL object, the multi-stapling PR modified the semantics of SSL_set_tlsext_status_ocsp_resp() to copying semantics. This change reverts the behavior to the previous one. Partially based on fix by Remi Gacogne: openssl#28894
Member
|
I took over this fix in PR #29251 |
openssl-machine
pushed a commit
that referenced
this pull request
Dec 1, 2025
Fixes #28888 Fixes b1b4b15 Instead of transferring the ownership of the single OCSP response to the SSL object, the multi-stapling PR modified the semantics of SSL_set_tlsext_status_ocsp_resp() to copying semantics. This change reverts the behavior to the previous one. Partially based on fix by Remi Gacogne: #28894 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from #29251)
openssl-machine
pushed a commit
that referenced
this pull request
Dec 1, 2025
Fixes #28888 Fixes b1b4b15 Instead of transferring the ownership of the single OCSP response to the SSL object, the multi-stapling PR modified the semantics of SSL_set_tlsext_status_ocsp_resp() to copying semantics. This change reverts the behavior to the previous one. Partially based on fix by Remi Gacogne: #28894 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from #29251) (cherry picked from commit 7e50e03)
cxx194832
pushed a commit
to cxx194832/openssl
that referenced
this pull request
Dec 12, 2025
Fixes openssl#28888 Fixes b1b4b15 Instead of transferring the ownership of the single OCSP response to the SSL object, the multi-stapling PR modified the semantics of SSL_set_tlsext_status_ocsp_resp() to copying semantics. This change reverts the behavior to the previous one. Partially based on fix by Remi Gacogne: openssl#28894 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29251)
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
5 participants
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.
The changes introduced in b1b4b15 cause the
respparameter to no longer be assigned to theSSLobject.Since it is not freed either, and the caller used to expect that, the memory is now leaked.
This commit frees
respright away. There is an early error return whensk_OCSP_RESPONSE_new_reservefails where the memory is not freed.I'm not sure whether it makes sense to free the memory in that case, as the documentation does not mention anything.
Fixes #28888
CLA: trivial
Checklist