Fix OpenSSL transport handshake correctness and error-path leaks (#5486)#5681
Conversation
- Add a default case to the handshake SSL_get_error switch so results such as SSL_ERROR_WANT_X509_LOOKUP/WANT_ASYNC (reachable with a user-supplied SSL_CTX) throw instead of busy-spinning. - Report the verify failure reason from X509_STORE_CTX_get_error(ctx) rather than the stale/generic SSL_get_verify_result(_ssl) inside the per-certificate verify callback. - Reject an empty IceSSL.CertFile before dereferencing &buffer[0] (undefined behavior on a zero-byte file). - Free leaked OpenSSL objects on error paths: the BIO when the context selection callback returns null, the popped extra-chain X509 when SSL_CTX_add_extra_chain_cert fails, and the X509 in decodeCertificate when X509_check_purpose fails. Addresses zeroc-ice#5486 (items 2, 3, 4, 6).
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes several OpenSSL transport edge cases found in the transport audit: prevents handshake busy-spins on unhandled SSL_get_error results, improves verification failure reporting, and plugs a few error-path leaks/UB scenarios.
Changes:
- Add a
defaultcase in the handshake error switch to throw onSSL_ERROR_WANT_*cases not driven by the event loop. - Improve certificate verification failure reporting by using
X509_STORE_CTX_get_error(ctx)within the per-certificate callback. - Prevent UB on empty cert files and free OpenSSL objects correctly on several failure paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/Ice/SSL/SSLUtil.cpp | Frees X509 on X509_check_purpose failure to avoid a leak. |
| cpp/src/Ice/SSL/OpenSSLTransceiverI.cpp | Fixes error-path BIO leak, avoids handshake busy-spin, and improves verify error source. |
| cpp/src/Ice/SSL/OpenSSLEngine.cpp | Rejects empty cert files before dereferencing buffer, and frees extra-chain cert on add failure. |
- In the handshake error switch, include the SSL_get_error code and the handshake return value in the default-case message. That case catches SSL_ERROR_WANT_* results for which the OpenSSL error queue is typically empty, so sslErrors() alone reports no actionable reason. - Report the resolved certificate path (the file actually read) in the empty-certificate message instead of the configured certFile value.
bernardnormier
left a comment
bernardnormier
left a comment
There was a problem hiding this comment.
Verified each fix against OpenSSL ownership semantics (popped c so no double-free, X509_STORE_CTX_get_error is the right in-callback reason source, local x leak). All correct. Changelog scoping looks right.
Addresses #5486 (items 2, 3, 4, 6). One of a set of PRs splitting the grouped transport audit.
default:to the handshakeSSL_get_errorswitch so results such asSSL_ERROR_WANT_X509_LOOKUP/WANT_ASYNC(reachable with a user-suppliedSSL_CTX) throw instead of busy-spinning.X509_STORE_CTX_get_error(ctx)rather than the stale/genericSSL_get_verify_result(_ssl)inside the per-certificate verify callback.IceSSL.CertFilebefore dereferencing&buffer[0](undefined behavior on a zero-byte file).BIOwhen the context selection callback returns null, the popped extra-chainX509whenSSL_CTX_add_extra_chain_certfails, and theX509indecodeCertificatewhenX509_check_purposefails.The OpenSSL backend is not built on macOS; verified by review + codex (OpenSSL ownership semantics checked against the docs). CI exercises the Linux/OpenSSL build.