25.3.8-fips: openssl-cmake: shim fixes for MongoDB by mkmkme · Pull Request #1451 · Altinity/ClickHouse

mkmkme · 2026-02-26T11:50:54Z

This commit fixes three issues that led to MongoDB integration tests failing. I tried to explain each of the fixes with a proper documentation to the function/macro.

Changelog category (leave one):

Not for changelog (changelog entry is not required)

CI/CD Options

Exclude tests:

Regression jobs to run:

mkmkme · 2026-02-27T09:20:53Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7223ea8e64

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

contrib/openssl-cmake/include/evp_API_shim.h

mkmkme · 2026-02-27T09:52:16Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ccd7df1c1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-02-27T09:59:10Z

contrib/openssl-cmake/include/evp_API_shim.h

+    SSL *ssl = (SSL *)BIO_get_data(b);
+    if (!ssl)


Validate BIO type before casting BIO_get_data to SSL

BIO_do_handshake now assumes b is always an SSL BIO and immediately casts BIO_get_data(b) to SSL *, but BIO_get_data for non-SSL BIOs returns other internal structs. If a caller invokes handshake on a wrapped chain head (e.g., a filter/connect BIO above SSL), SSL_get_rbio/SSL_do_handshake will run on an invalid pointer and can crash. The previous BIO_ctrl(...BIO_C_DO_STATE_MACHINE...) path delegated through BIO ctrl handlers instead of dereferencing arbitrary BIO payloads.

Useful? React with 👍 / 👎.

The only call site for this function in the entire codebase is mongoc-stream-tls-openssl.c:569, and openssl->bio is assigned at line 793 from bio_ssl, which is the SSL BIO created by BIO_new_ssl.

chatgpt-codex-connector · 2026-02-27T09:59:11Z

contrib/openssl-cmake/include/evp_API_shim.h

 #define BIO_get_ssl(b, sslp) \
-    BIO_ctrl(b, BIO_C_GET_SSL, 0, (char *)(sslp))
-#define BIO_do_handshake(b) \
-    BIO_ctrl(b, BIO_C_DO_STATE_MACHINE, 0, NULL)
+    (*(SSL **)(sslp) = (SSL *)BIO_get_data(b))


Preserve BIO_ctrl chain semantics in BIO_get_ssl shim

Replacing BIO_get_ssl with direct BIO_get_data(b) extraction drops BIO_ctrl forwarding semantics and only works when b is the SSL BIO itself. On chained BIOs, this macro will store the top BIO's private data pointer into sslp rather than the SSL object, which can later cause invalid SSL operations. OpenSSL's macro (BIO_ctrl(...BIO_C_GET_SSL...)) supports wrappers by routing through ctrl handlers.

Useful? React with 👍 / 👎.

In our use case (MongoDB contrib), every call for BIO_get_ssl is proven to be on the SSL BIO directly and called right after BIO_new_ssl

Enmk

LGTM

DimensionWieldr · 2026-04-02T01:05:33Z

AI audit returned 1 defect (potential memory leak). I think this one needs review.

Confirmed defects



  Medium: BIO reference count leak in `BIO_do_handshake`

  • Impact: The transport BIO (bio_mongoc_shim) is leaked on every MongoDB TLS connection teardown. Each leaked BIO is ~64-128 bytes. The freed-but-referenced BIO_METHOD (freed
    separately at mongoc-stream-tls-openssl.c:93) creates a dangling pointer inside the leaked BIO, though it is never dereferenced.
  • Anchor: contrib/openssl-cmake/include/evp_API_shim.h / BIO_do_handshake() lines 127-129
  • Trigger: Any MongoDB TLS connection in FIPS mode. Lifecycle: mongoc_stream_tls_openssl_new (line 766: BIO_push) → _mongoc_stream_tls_openssl_handshake (line 569:
    BIO_do_handshake) → _mongoc_stream_tls_openssl_destroy (line 90: BIO_free_all).
  • Why defect: SSL_set_bio(ssl, bio, bio) with rbio == wbio internally calls BIO_up_ref(rbio) once (OpenSSL ssl_lib.c:1557-1558: "If the two arguments are equal then one fewer 
    reference is granted by the caller than we want to take"). SSL_free later calls BIO_free_all on both rbio and wbio (2 decrements). The BIO chain walk adds 1 more decrement.
    Total decrements at cleanup = 3. The shim adds 2 manual BIO_up_ref + 1 internal = 3 extra refs on top of the initial refcount of 1 → final refcount = 4 − 3 = 1 (never freed).
    Only 1 manual BIO_up_ref is needed.
  • Fix direction: Remove one BIO_up_ref(BIO_next(b)) call (line 127 or 128).
  • Regression test: Open and close MongoDB TLS connections in FIPS mode under LSAN/ASAN; verify no leaked BIO allocations.


   contrib/openssl-cmake/include/evp_API_shim.h lines 127-129

          BIO_up_ref(BIO_next(b));
          BIO_up_ref(BIO_next(b));   // one too many
          SSL_set_bio(ssl, BIO_next(b), BIO_next(b));

mkmkme · 2026-04-02T14:56:33Z

AI audit returned 1 defect (potential memory leak). I think this one needs review.

Confirmed defects



  Medium: BIO reference count leak in `BIO_do_handshake`

  • Impact: The transport BIO (bio_mongoc_shim) is leaked on every MongoDB TLS connection teardown. Each leaked BIO is ~64-128 bytes. The freed-but-referenced BIO_METHOD (freed
    separately at mongoc-stream-tls-openssl.c:93) creates a dangling pointer inside the leaked BIO, though it is never dereferenced.
  • Anchor: contrib/openssl-cmake/include/evp_API_shim.h / BIO_do_handshake() lines 127-129
  • Trigger: Any MongoDB TLS connection in FIPS mode. Lifecycle: mongoc_stream_tls_openssl_new (line 766: BIO_push) → _mongoc_stream_tls_openssl_handshake (line 569:
    BIO_do_handshake) → _mongoc_stream_tls_openssl_destroy (line 90: BIO_free_all).
  • Why defect: SSL_set_bio(ssl, bio, bio) with rbio == wbio internally calls BIO_up_ref(rbio) once (OpenSSL ssl_lib.c:1557-1558: "If the two arguments are equal then one fewer 
    reference is granted by the caller than we want to take"). SSL_free later calls BIO_free_all on both rbio and wbio (2 decrements). The BIO chain walk adds 1 more decrement.
    Total decrements at cleanup = 3. The shim adds 2 manual BIO_up_ref + 1 internal = 3 extra refs on top of the initial refcount of 1 → final refcount = 4 − 3 = 1 (never freed).
    Only 1 manual BIO_up_ref is needed.
  • Fix direction: Remove one BIO_up_ref(BIO_next(b)) call (line 127 or 128).
  • Regression test: Open and close MongoDB TLS connections in FIPS mode under LSAN/ASAN; verify no leaked BIO allocations.


   contrib/openssl-cmake/include/evp_API_shim.h lines 127-129

          BIO_up_ref(BIO_next(b));
          BIO_up_ref(BIO_next(b));   // one too many
          SSL_set_bio(ssl, BIO_next(b), BIO_next(b));

This seems to be a real issue. Thanks! I'll fix it asap.

Reference: https://github.com/aws/aws-lc/blob/AWS-LC-FIPS-2.0.0/ssl/ssl_lib.cc#L748

DimensionWieldr · 2026-04-06T16:44:54Z

Fixed by #1616 . LGTM

openssl-cmake: shim fixes for MongoDB

7223ea8

chatgpt-codex-connector bot reviewed Feb 27, 2026

View reviewed changes

contrib/openssl-cmake/include/evp_API_shim.h Outdated Show resolved Hide resolved

contrib/openssl-cmake/include/evp_API_shim.h Show resolved Hide resolved

review fixes

2ccd7df

chatgpt-codex-connector bot reviewed Feb 27, 2026

View reviewed changes

Enmk approved these changes Feb 27, 2026

View reviewed changes

Enmk merged commit 24e5691 into releases/25.3.8-fips Feb 27, 2026
137 of 167 checks passed

svb-alt added fips Work related to Altinity FIPS releases fips-25.3 labels Mar 11, 2026

svb-alt added the fips-25.3.8.30001 label Mar 31, 2026

DimensionWieldr added the verified-with-issues Verified by QA and issues found. label Apr 2, 2026

DimensionWieldr mentioned this pull request Apr 2, 2026

25.3.8-FIPS: enable all libraries #1444

Merged

25 tasks

DimensionWieldr added verified Approved for release and removed verified-with-issues Verified by QA and issues found. labels Apr 6, 2026

DimensionWieldr mentioned this pull request Apr 6, 2026

25.3.8-fips: fixes for verification issues #1616

Merged

27 tasks

Conversation

mkmkme commented Feb 26, 2026

Changelog category (leave one):

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

mkmkme commented Feb 27, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

mkmkme commented Feb 27, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Enmk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DimensionWieldr commented Apr 2, 2026

Uh oh!

mkmkme commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DimensionWieldr commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkmkme commented Apr 2, 2026 •

edited

Loading