CDRIVER-3228 fix memory leaks in SChannel cert loading #2009

kevinAlbs · 2025-05-09T13:59:22Z

Summary

This PR addresses several memory-related found in certificate loading for Secure Channel:

Fix several memory leaks.
NUL-terminate the PEM file for the client certificate.

Reading the PEM file is refactored with more error checks logs including the system error.

Background

Leaks were discovered when implementing PKCS#8 certificate support for (CDRIVER-4269). This may help motivate future addition of leak detection on Windows (CDRIVER-2529).

NUL-terminating the PEM file was similarly done in #1903. NUL-terminating is intended to avoid reading past beyond the string in later calls to strstr.

The Secure Channel implementation would likely benefit from a larger rewrite, and may be done as part of CDRIVER-4463. This PR intends to address the more severe issues and I expect can be backported with low risk of unexpected behavior change.

Do not return before `fail` label.

To ensure `strstr` does not read past memory on failure to find. This is consistent with reading in `mongoc_secure_channel_setup_ca`

ghost

Nice improvements in a pretty hairy area of code. It's always fun litigating subtleties of under-documented legacy APIs :)

I had a handful of comments but I don't think any of them should be treated as blockers. The type confusion possible from CERT_QUERY_CONTENT_FLAG_ALL might be my biggest concern but it's not new.

I think it's possible to simplify the changes to tls-secure-channel (freeing 'cert' early instead of storing it) but I haven't verified this beyond reading docs and code.

ghost · 2025-05-09T17:32:59Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c

+      MONGOC_ERROR ("Failed to get length of file: '%s'. File too large", filename);
+      goto fail;
+   }
+
+   if (0 != fseek (file, 0, SEEK_SET)) {
+      MONGOC_ERROR ("Failed to seek in file: '%s' with error: '%s'",
+                    filename,
+                    bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));
+      goto fail;
+   }


Just a thought, if we wanted to simplify the error reporting here, some of these are definitely worth catching at runtime but maybe not worth having a unique error string for. (I'm thinking that the LONG_MAX-1 check and the fseek to zero check are needed for security, but in practice will only fail due to unusual filesystems.)

I'm also noticing that below we have a possible overflow when the strlen of an individual key in the file is cast to DWORD (see public_blob.cbData). The security implication seems acceptably minor there, but it'd be a small improvement IMO to disallow file sizes larger than INT_MAX or so and remove that bit of attack surface.

not worth having a unique error string for

Agreed. Removed error log for the unlikely-to-encounter errors.

possible overflow when the strlen of an individual key in the file is cast to DWORD

Good spot. This is addressed by replacing the call of CryptQueryObject with CertCreateCertificateContext. Calculating the length is no longer needed.

src/libmongoc/src/mongoc/mongoc-secure-channel.c

ghost · 2025-05-09T19:03:52Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c


   /* https://msdn.microsoft.com/en-us/library/windows/desktop/aa376573%28v=vs.85%29.aspx
    */
+   // CertSetCertificateContextProperty takes ownership of `provider`.


This comment is very helpful! I'm noticing that the specific doc we could reference is on the CERT_KEY_PROV_HANDLE_PROP_ID property rather than the whole of CertSetCertificateContextProperty:

CERT_KEY_PROV_HANDLE_PROP_ID

Data type of pvData: A HCRYPTPROV value.

The HCRYPTPROV handle for the certificate's private key is set. The hCryptProv member of the CERT_KEY_CONTEXT structure is updated if it exists. If it does not exist, it is created with dwKeySpec and initialized by CERT_KEY_PROV_INFO_PROP_ID. If CERT_STORE_NO_CRYPT_RELEASE_FLAG is not set, the hCryptProv value is implicitly released either when the property is set to NULL or on the final freeing of the CERT_CONTEXT structure.

Revised comment.

ghost · 2025-05-09T19:22:59Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c

-   return NULL;
+   if (!ret) {
+      if (cert) {
+         CertFreeCertificateContext (cert);


I'm noticing that the type of object returned by CryptQueryObject() above can vary due to the use of CERT_QUERY_CONTENT_FLAG_ALL. I would comment on that directly, but GitHub doesn't seem to let me comment on lines that weren't in the diff. Anyway, I think we probably want CERT_QUERY_CONTENT_CERT above to constrain the return type.

(There's also CERT_QUERY_CONTENT_SERIALIZED_CERT, which is not described clearly in this API reference but I'm finding a 3rd party blog that claims "serialized certificates" are a Microsoft-specific format associated with the .sst file extension, https://medium.com/tenable-techblog/code-for-reading-windows-serialized-certificates-8634d3487ec7)

Doc reference: https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptqueryobject

Noticed this when looking at CertFreeCertificateContext call sites in this file: mongoc_secure_channel_setup_crl is using the wrong free function, there's a separate CertFreeCRLContext API for CRLs.

(Ref: https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptqueryobject)

I think we probably want CERT_QUERY_CONTENT_CERT above to constrain the return type.

Replaced call of CryptQueryObject with a call to CertCreateCertificateContext to address.

mongoc_secure_channel_setup_crl is using the wrong free function

Great spot. Debugging shows CryptQueryObject sets the output type (pdwContentType) to CERT_QUERY_CONTENT_CRL. According to documentation:

CERT_QUERY_CONTENT_CRL This parameter receives a pointer to a CRL_CONTEXT structure

Yet the C driver stored the result in a PCCERT_CONTEXT (Pointer-to-Const CERT_CONTEXT) and later passed to CertAddCertificateContextToStore. Despite this, a test with the CRL list appeared to work. I expect this worked "by accident" due to the CERT_CONTEXT and CRL_CONTEXT having the same data layout.

To avoid the incorrect pointer use, latest changes replace the call to CryptQueryObject. A test setting the CRL option is added.

src/libmongoc/src/mongoc/mongoc-secure-channel.c

src/libmongoc/src/mongoc/mongoc-stream-tls-secure-channel.c

src/libmongoc/src/mongoc/mongoc-secure-channel.c

ghost · 2025-05-10T03:24:27Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c

   DWORD encrypted_cert_len = 0;
   LPBYTE encrypted_cert = NULL;


"encrypted" is wrong here too, I think they meant "encoded".

src/libmongoc/src/mongoc/mongoc-stream-tls-secure-channel-private.h

src/libmongoc/src/mongoc/mongoc-stream-tls-secure-channel.c

src/libmongoc/src/mongoc/mongoc-secure-channel.c

eramongodb · 2025-05-12T15:23:48Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c

+   if (0 != fseek (file, 0, SEEK_SET)) {
+      MONGOC_ERROR ("Failed to seek in file: '%s' with error: '%s'",
+                    filename,
+                    bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));


Suggested change

bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));

bson_strerror_r (ferror (file), errmsg_buf, sizeof errmsg_buf));

Unlike fopen or ftell, fseek sets ferror in failure, not errno.

src/libmongoc/src/mongoc/mongoc-secure-channel.c

Encrypted keys are not supported. "encoded" is consistent with naming in WinCrypt API.

Avoids NULL deref if PEM file does not have public cert Add a regression test.

`CryptQueryObject` is deprecated. The flag `CERT_QUERY_CONTENT_FLAG_ALL` is likely incorrect (only certificate is expected)

Value is not currently checked by caller. Return true on success to match existing patterns.

Return was wrongly stored in a `CERT_CONTEXT` (needed `CRL_CONTEXT`). Use `CertCreateCRLContext` for consistency with other PEM-reading functions.

To match error expectations in C99.

To clarify error being returned is return value

kevinAlbs

Latest changes tested with this patch build and include:

Fix a NULL-deref if the client PEM file does not include a public certificate.
Refactor base64 decoding to a separate helper function.
Replace calls to CryptQueryObject.
- Fixes too-broad search for public key (CERT_QUERY_CONTENT_FLAG_ALL instead of CERT_QUERY_CONTENT_FLAG_CERT).
- Fixes incorrect pointer loading CRL (was using CERT_CONTEXT instead of CRL_CONTEXT).
Test CRL loading.

src/libmongoc/src/mongoc/mongoc-secure-channel.c

kevinAlbs · 2025-05-12T16:37:20Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c

+      MONGOC_ERROR ("Failed to get length of file: '%s'. File too large", filename);
+      goto fail;
+   }
+
+   if (0 != fseek (file, 0, SEEK_SET)) {
+      MONGOC_ERROR ("Failed to seek in file: '%s' with error: '%s'",
+                    filename,
+                    bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));
+      goto fail;
+   }


not worth having a unique error string for

Agreed. Removed error log for the unlikely-to-encounter errors.

possible overflow when the strlen of an individual key in the file is cast to DWORD

Good spot. This is addressed by replacing the call of CryptQueryObject with CertCreateCertificateContext. Calculating the length is no longer needed.

src/libmongoc/src/mongoc/mongoc-secure-channel.c

kevinAlbs · 2025-05-12T18:55:51Z

src/libmongoc/src/mongoc/mongoc-secure-channel.c


   /* https://msdn.microsoft.com/en-us/library/windows/desktop/aa376573%28v=vs.85%29.aspx
    */
+   // CertSetCertificateContextProperty takes ownership of `provider`.


Revised comment.

src/libmongoc/src/mongoc/mongoc-secure-channel.c

src/libmongoc/src/mongoc/mongoc-stream-tls-secure-channel.c

src/libmongoc/src/mongoc/mongoc-secure-channel.c

Co-authored-by: Ezra Chung <[email protected]>

This reverts commit 599638e.

This reverts commit 18d4168.

ghost

Looks good! The CRL test and API usage simplifications are a nice addition.

* Store and free client cert context * Free on successful load of client cert ** Do not return before `fail` label. * Free `hKey` * Free pem file and cert when loading CA file * Release provider context on error * NUL terminate pem file contents ** To ensure `strstr` does not read past memory on failure to find. * Remove unused printf * Add `read_file_and_null_terminate` helper * Rename `encrypted_*` to `encoded_*` ** Encrypted keys are not supported with SChannel. ** "encoded" is consistent with naming in WinCrypt API. * check if `pem_public` is NULL ** Avoids NULL deref if PEM file does not have public cert * Remove call to `CryptQueryObject` for public cert ** The flag `CERT_QUERY_CONTENT_FLAG_ALL` is likely incorrect (only certificate is expected) * Remove call to `CryptQueryObject` for CRL ** Return was wrongly stored in a `CERT_CONTEXT` (needed `CRL_CONTEXT`). ** Use `CertCreateCRLContext` for consistency with other PEM-reading functions. * Remove unused params --------- Co-authored-by: Ezra Chung <[email protected]>

#2015) * CDRIVER-5743: NUL terminate string, handle more error cases (#1903) * NUL termination and error handling fixes for mongoc_secure_channel_setup_ca * CDRIVER-5743 Followup fix for signedness warning (#1907) * CDRIVER-3228 fix memory leaks in SChannel cert loading (#2009) * Store and free client cert context * Free on successful load of client cert ** Do not return before `fail` label. * Free `hKey` * Free pem file and cert when loading CA file * Release provider context on error * NUL terminate pem file contents ** To ensure `strstr` does not read past memory on failure to find. * Remove unused printf * Add `read_file_and_null_terminate` helper * Rename `encrypted_*` to `encoded_*` ** Encrypted keys are not supported with SChannel. ** "encoded" is consistent with naming in WinCrypt API. * check if `pem_public` is NULL ** Avoids NULL deref if PEM file does not have public cert * Remove call to `CryptQueryObject` for public cert ** The flag `CERT_QUERY_CONTENT_FLAG_ALL` is likely incorrect (only certificate is expected) * Remove call to `CryptQueryObject` for CRL ** Return was wrongly stored in a `CERT_CONTEXT` (needed `CRL_CONTEXT`). ** Use `CertCreateCRLContext` for consistency with other PEM-reading functions. * Remove unused params --------- Co-authored-by: mdbmes <[email protected]> Co-authored-by: Ezra Chung <[email protected]>

kevinAlbs and others added 8 commits May 9, 2025 09:57

CDRIVER-3228 store and free client cert context

f80d634

free on successful load of client cert

8f2f8b4

Do not return before `fail` label.

free hKey

099028d

free pem file and cert when loading CA file

2b04fcf

release provider context on error

5dc1e0f

NUL terminate pem file contents

93ca528

To ensure `strstr` does not read past memory on failure to find. This is consistent with reading in `mongoc_secure_channel_setup_ca`

remove commented out printf

38a9129

refactor: add read_file_and_null_terminate helper

61e39c5

kevinAlbs force-pushed the leaks-only.C4296 branch from 1a0bce5 to 61e39c5 Compare May 9, 2025 14:15

kevinAlbs marked this pull request as ready for review May 9, 2025 16:31

kevinAlbs requested a review from a team as a code owner May 9, 2025 16:31

kevinAlbs requested review from rcsanchez97, eramongodb and a user and removed request for rcsanchez97 May 9, 2025 16:31

ghost approved these changes May 10, 2025

View reviewed changes

eramongodb requested changes May 12, 2025

View reviewed changes

kevinAlbs mentioned this pull request May 14, 2025

CDRIVER-6011 fix message of bson_strerror_r on Windows #2011

Merged

kevinAlbs added 12 commits May 15, 2025 12:57

rename encrypted_* to encoded_*

2130afd

Encrypted keys are not supported. "encoded" is consistent with naming in WinCrypt API.

remove unlikely-to-encounter logs

3fc2180

check if pem_public is NULL

04afd60

Avoids NULL deref if PEM file does not have public cert Add a regression test.

revise comment about ownership

e9cf761

refactor: add base64 decode helper

58c2273

remove call to CryptQueryObject for public cert

5e44853

`CryptQueryObject` is deprecated. The flag `CERT_QUERY_CONTENT_FLAG_ALL` is likely incorrect (only certificate is expected)

return ok value when adding CA

30583ef

Value is not currently checked by caller. Return true on success to match existing patterns.

remove call to CryptQueryObject for CRL

6899d96

Return was wrongly stored in a `CERT_CONTEXT` (needed `CRL_CONTEXT`). Use `CertCreateCRLContext` for consistency with other PEM-reading functions.

test loading CRL

6495cae

use const

e81bc5d

use NUL

c4ceedc

use ferror for fseek and fread

18d4168

To match error expectations in C99.

kevinAlbs added 6 commits May 15, 2025 12:57

clear and check for errno on failing fopen

599638e

add void cast

1b5c4b9

To clarify error being returned is return value

call SecureZeroMemory on failed read

0f0d51a

do not call SecureZeroMemory on NULL

958f5e0

reword comment

d17f848

remove unnecessary NULL checks

3d51fdb

kevinAlbs commented May 15, 2025

View reviewed changes

kevinAlbs requested review from eramongodb and a user May 15, 2025 17:41

format

119b529

eramongodb requested changes May 15, 2025

View reviewed changes

kevinAlbs and others added 5 commits May 15, 2025 15:36

add BSON_ASSERT_PARAM

cd76a7a

Co-authored-by: Ezra Chung <[email protected]>

Revert "clear and check for errno on failing fopen"

63e0082

This reverts commit 599638e.

Revert "use ferror for fseek and fread"

d8022a9

This reverts commit 18d4168.

remove unused params

0292e6a

fail gracefully on zero length base64

d888831

kevinAlbs requested a review from eramongodb May 15, 2025 20:34

eramongodb approved these changes May 15, 2025

View reviewed changes

ghost approved these changes May 16, 2025

View reviewed changes

kevinAlbs merged commit 378c177 into mongodb:master May 16, 2025
39 of 42 checks passed

kevinAlbs mentioned this pull request May 16, 2025

[r2.0] CDRIVER-3228 fix memory leaks in SChannel cert loading (#2009) #2014

Merged

kevinAlbs mentioned this pull request May 16, 2025

[r1.30] CDRIVER-3228 fix memory leaks in SChannel cert loading (#2009) #2015

Merged

	bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));
	bson_strerror_r (ferror (file), errmsg_buf, sizeof errmsg_buf));

CDRIVER-3228 fix memory leaks in SChannel cert loading #2009

CDRIVER-3228 fix memory leaks in SChannel cert loading #2009

Uh oh!

Conversation

kevinAlbs commented May 9, 2025

Summary

Background

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!