doc: clarify SSL_SESSION ownership in PSK use session callback#29771
doc: clarify SSL_SESSION ownership in PSK use session callback#29771kovan wants to merge 1 commit intoopenssl:masterfrom
Conversation
paulidale
left a comment
There was a problem hiding this comment.
It would be less likely to confuse if later references to psksess used s->psksession.
|
Okay, I don't see how the combination of "Fix double free when PSK session callback returns same session" and "Fix memory leak when PSK callback returns same session" addresses [1]; it seems to me, based on the latter commit, that uprefing of existing session is missing in the callback provided in the issue, provided the contract is indeed implemented the way described in the commit message of the latter commit. Moreover, the dance with [1] #28267 |
2c6a162 to
89e1076
Compare
|
+1 to @esyr comment. The documentation for SSL_CTX_set_psk_client_callback indicates that: Thats a bit of a round about way to say it, but any hold it has over that memory once the callback is issued and returns is gone from the callback. The library owns it at that point and is responsible for freeing it. The only way it works is if the callback adheres to 1 of 2 rules:
|
ssl/statem/extensions_clnt.c
Outdated
| * return the same session without calling SSL_SESSION_up_ref(). | ||
| */ | ||
| if (psksess != NULL) | ||
| SSL_SESSION_up_ref(psksess); |
There was a problem hiding this comment.
This in fact is reversing the guidance given in https://docs.openssl.org/master/man3/SSL_CTX_set_psk_client_callback/#description
In that, if the callback is incrementing the reference count of the session on its own, then libcrypto will never wind up freeing this session, requiring that some code alongside the callback do so at some later time when its sure its no longer in use (which may be un-knowable to the application implementing the callback).
We have to rely on the callback doing the right thing here. You can see an example of how its done with with psk_use_session_cb in s_client. It either allocates a new SSL_SESSION on each callback, or, in the event one was read in from a file, ensures that we have a reference to it by calling SSL_SESSION_up_ref on each callback. With this change, in the latter case, we will leak a session.
|
OK, I stop. I really don't know how to solve this, guys. Every path I tried has some undesired side-effect. |
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes openssl#28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
89e1076 to
b70c580
Compare
esyr
left a comment
There was a problem hiding this comment.
Maybe a tad verbose, but looks fine to me as-is.
|
This pull request is ready to merge |
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes #28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Fri Feb 13 14:36:50 2026 (Merged from #29771) (cherry picked from commit 6d646a9)
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes #28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Fri Feb 13 14:36:50 2026 (Merged from #29771)
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes #28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Fri Feb 13 14:36:50 2026 (Merged from #29771) (cherry picked from commit 6d646a9)
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes #28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Fri Feb 13 14:36:50 2026 (Merged from #29771) (cherry picked from commit 6d646a9)
|
Merged to all the active branches. |
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes #28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Fri Feb 13 14:36:50 2026 (Merged from #29771) (cherry picked from commit 6d646a9)
Document that when the psk_use_session callback is invoked multiple times and wishes to return the same SSL_SESSION pointer, it must call SSL_SESSION_up_ref() first since ownership is transferred on each call. This prevents use-after-free errors from incorrect callback implementations. Fixes #28267 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Fri Feb 13 14:36:50 2026 (Merged from #29771) (cherry picked from commit 6d646a9)
Summary
psk_use_sessioncallback may be called multiple timesSSL_SESSION_up_ref()firstSSL_SESSION_dup()as an alternative approachThis is a documentation-only fix. As noted by @bob-beck in the issue, the existing behavior is correct per the documented contract - the issue was that the documentation wasn't explicit enough about the implications when the callback is called multiple times.
Fixes #28267
Test plan
make doc-nitsto verify documentation standards🤖 Generated with Claude Code