hongzhi-gao commented on PR #3606: URL: https://github.com/apache/thrift/pull/3606#issuecomment-4849559826
> ### Code review > Found 6 issues (adversarially verified): > > 1. `SSLContext(SSL_CTX*, false)` stores the raw pointer without calling `SSL_CTX_up_ref`. If the external owner frees the context while this `SSLContext` is alive, the subsequent `SSL_new(ctx_)` call in `createSSL()` uses freed memory. > > https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L216-L221 > > Verified: no `SSL_CTX_up_ref` call present in PR code; constructor absent in base. > > 2. Adding `bool takeOwnership_` to `SSLContext` changes `sizeof(SSLContext)` from 8 to 16 bytes. `SSLContext` is a concrete public class in an installed header; no soname bump or ABI versioning mechanism exists in the project. > > https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L365-L374 > > Verified: member absent in base, no SOVERSION in CMakeLists.txt, no `.map`/`.sym` files under `lib/cpp/`. > > 3. Both new tests exercise `takeOwnership=true` only (`wrapped_ssl_context` passes `true` explicitly; `wrapped_ssl_context_null` throws before the ownership branch). The destructor branch where `SSL_CTX_free` is skipped (`takeOwnership_=false`) has no test. > > https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/test/SecurityTest.cpp#L286-L306 > > Verified: `grep -n "takeOwnership\|, false" SecurityTest.cpp` returns no output on PR code; branch absent in base. > > 4. The new constructor's Doxygen does not warn that callers are responsible for enforcing a minimum TLS version. `SSLContext(SSLProtocol)` applies `SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1` unconditionally; the new constructor wraps the context as-is and the test creates it with `TLS_method()` and no version restrictions. > > https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L367-L374 > > Verified: no `SSL_OP_NO_TLSv1*` in new constructor; existing floor confirmed at [`TSSLSocket.cpp:210-212`](https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L210-L212); constructor absent in base. > > 5. When `takeOwnership=false`, the caller must keep the `SSL_CTX*` alive at least as long as the `SSLContext` and all sockets created from it. This is not documented in the new constructor's Doxygen. The factory class has an explicit parallel warning ([`TSSLSocket.h:200-206`](https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L200-L206): _"It is the responsibility of the code using TSSLSocketFactory to ensure that the factory lifetime exceeds the lifetime of any sockets it might create"_). > > https://github.com/apache/thrift/blob/08700779e125daaf3ab9c97fba6ef2e3ef70fa20/lib/cpp/src/thrift/transport/TSSLSocket.h#L367-L374 > > Verified: `@param takeOwnership` documents only the `true` case; factory warning confirmed in base and PR; constructor absent in base. > > 6. `AGENTS.md` ยง6 and its Quick Reference checklist require TLS configuration changes to be cross-checked against `doc/thrift-threat-model.md` before merging. The PR body contains no mention of this. `doc/thrift-threat-model.md` is present in the repo. (Note: per ยง6, any details about trust-boundary implications belong in the JIRA ticket [THRIFT-6073](https://issues.apache.org/jira/browse/THRIFT-6073), not in the PR description or commit messages.) > > Verified: AGENTS.md requirement confirmed; PR body grep for "threat" returns no output; `doc/thrift-threat-model.md` exists at PR head. > > ๐ค Generated with [Claude Code](https://claude.ai/code) > > - If this code review was useful, please react with ๐. Otherwise, react with ๐. **#1 / #2 / #3 / #5 โ ownership API:** Dropped the `takeOwnership` parameter and `takeOwnership_` member entirely. The injected constructor is now `explicit SSLContext(SSL_CTX* ctx)` and unconditionally takes ownership; the destructor always calls `SSL_CTX_free`. This removes the non-owning / UAF path, restores the pre-change object layout (pointer-only), and makes the ownership contract unambiguous. **#4 โ TLS version floor:** Expanded the constructor Doxygen to note that, unlike `SSLContext(SSLProtocol)` with default `SSLTLS`, the injected path does not apply `SSL_OP_NO_TLSv1*` and callers must configure a protocol floor on the `SSL_CTX` before wrapping if needed. **#6 โ threat model:** Cross-checked against `doc/thrift-threat-model.md` per AGENTS.md ยง6; details recorded in JIRA THRIFT-6073 (not in the PR description, per project guidance). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
