Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f80d634
CDRIVER-3228 store and free client cert context
kevinAlbs May 9, 2025
8f2f8b4
free on successful load of client cert
kevinAlbs May 9, 2025
099028d
free `hKey`
kevinAlbs May 9, 2025
2b04fcf
free pem file and cert when loading CA file
kevinAlbs May 9, 2025
5dc1e0f
release provider context on error
kevinAlbs May 9, 2025
93ca528
NUL terminate pem file contents
kevinAlbs May 9, 2025
38a9129
remove commented out printf
kevinAlbs May 9, 2025
61e39c5
refactor: add `read_file_and_null_terminate` helper
kevinAlbs May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 122 additions & 60 deletions src/libmongoc/src/mongoc/mongoc-secure-channel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,92 @@
#define SECBUFFER_ALERT 17
#endif

// `read_file_and_null_terminate` reads a file into a NUL-terminated string.
// On success: returns a NUL-terminated string and (optionally) sets `*out_len` excluding NUL.
// On error: returns NULL.
static char *
read_file_and_null_terminate (const char *filename, size_t *out_len)
{
BSON_ASSERT_PARAM (filename);
BSON_OPTIONAL_PARAM (out_len);

bool ok = false;
char *contents = NULL;
char errmsg_buf[BSON_ERROR_BUFFER_SIZE];

FILE *file = fopen (filename, "rb");
if (!file) {
MONGOC_ERROR ("Failed to open file: '%s' with error: '%s'",
filename,
bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));
goto fail;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

goto fail;
}

long file_len = ftell (file);
Copy link
Collaborator

@eramongodb eramongodb May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
long file_len = ftell (file);
const long file_len = ftell (file);

Const correctness.

if (file_len < 0) {
MONGOC_ERROR ("Failed to get length of file: '%s' with error: '%s'",
filename,
bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));
goto fail;
}

if (file_len > LONG_MAX - 1) {
MONGOC_ERROR ("Failed to get length of file: '%s'. File too large", filename);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MONGOC_ERROR ("Failed to get length of file: '%s'. File too large", filename);
MONGOC_ERROR ("Length of file '%s' is too large", filename);

Misleading: this is not failure to obtain the length of the file, but an invalid file length which may prevent reservation of the null terminating byte.

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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

goto fail;
}
Comment on lines +81 to +90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Read the whole file into one nul-terminated string:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Read the whole file into one nul-terminated string:
// Read the whole file into one null-terminated string:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ASCII character is often written as NUL or "nul", with "null" specifically referring to a pointer instead.

contents = (char *) bson_malloc ((size_t) file_len + 1u);
contents[file_len] = '\0';
if ((size_t) file_len != fread (contents, 1, file_len, file)) {
if (feof (file)) {
MONGOC_ERROR ("Unexpected EOF reading file: '%s'", filename);
goto fail;
} else {
MONGOC_ERROR ("Failed to read file: '%s' with error: '%s'",
filename,
bson_strerror_r (errno, errmsg_buf, sizeof errmsg_buf));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fread sets ferror on failure, not errno.

goto fail;
}
}
if (out_len) {
*out_len = (size_t) file_len;
}

ok = true;
fail:
if (file) {
fclose (file); // Ignore error.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fclose (file); // Ignore error.
(void) fclose (file); // Ignore error.

The error being ignored is the return value.

}
if (!ok) {
bson_free (contents);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bson_free (contents);
SecureZeroMemory (contents, file_len);
bson_free (contents);

Ensure any partially-read contents are securely overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're trying to be very careful about leaking private key data into the address space we should avoid buffered reads entirely and use fileapi directly.

(For example, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile)

contents = NULL;
}
return contents;
}


PCCERT_CONTEXT
mongoc_secure_channel_setup_certificate_from_file (const char *filename)
{
char *pem;
FILE *file;
bool ret = false;
bool success;
HCRYPTKEY hKey;
long pem_length;
size_t pem_length;
HCRYPTPROV provider;
CERT_BLOB public_blob;
const char *pem_public;
Expand All @@ -60,25 +137,11 @@ mongoc_secure_channel_setup_certificate_from_file (const char *filename)
DWORD encrypted_private_len = 0;
LPBYTE encrypted_private = NULL;


file = fopen (filename, "rb");
if (!file) {
MONGOC_ERROR ("Couldn't open file '%s'", filename);
return NULL;
}

fseek (file, 0, SEEK_END);
pem_length = ftell (file);
fseek (file, 0, SEEK_SET);
if (pem_length < 1) {
MONGOC_ERROR ("Couldn't determine file size of '%s'", filename);
return NULL;
pem = read_file_and_null_terminate (filename, &pem_length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to comment on the lines above this, but unfortunately, GitHub.

The usage of encrypted_private[_len] here is confusing, since we explicitly do not support encrypted private keys. I think this means to be "encoded" rather than "encrypted".

if (!pem) {
goto fail;
}

pem = (char *) bson_malloc0 (pem_length);
fread ((void *) pem, 1, pem_length, file);
fclose (file);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to comment on the use of strlen (pem_public) below: This is redundant since we already know the location of the NUL. The length could be calculated as (pem_length - (pem_public - pem)) instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to avoid calling CryptDecodeObjectEx twice, there's CRYPT_DECODE_ALLOC_FLAG.

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

pem_public = strstr (pem, "-----BEGIN CERTIFICATE-----");
pem_private = strstr (pem, "-----BEGIN ENCRYPTED PRIVATE KEY-----");

Expand Down Expand Up @@ -192,6 +255,7 @@ mongoc_secure_channel_setup_certificate_from_file (const char *filename)
goto fail;
}

HCRYPTKEY hKey;
/* https://msdn.microsoft.com/en-us/library/windows/desktop/aa380207%28v=vs.85%29.aspx
*/
success = CryptImportKey (provider, /* hProv */
Expand All @@ -202,21 +266,25 @@ mongoc_secure_channel_setup_certificate_from_file (const char *filename)
&hKey); /* phKey, OUT */
if (!success) {
MONGOC_ERROR ("CryptImportKey for private key failed with error 0x%.8X", (unsigned int) GetLastError ());
CryptReleaseContext (provider, 0);
goto fail;
}
CryptDestroyKey (hKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right but I can't find a good reference that explains why this doesn't destroy the underlying key. It would be cool to add this if you have something handy. I see the CryptDestroyKey MSDN page but there seem to be two contradicting statements there:

Many CSPs overwrite the memory where the key was held before freeing it. However, the underlying public/private key pair is not destroyed by this function. Only the handle is destroyed.

and:

Keys take up both operating system's memory space and the CSP's memory space. Some CSPs are implemented in hardware with limited memory resources. Applications must destroy all keys with the CryptDestroyKey function when they are finished with them.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

success = CertSetCertificateContextProperty (cert, /* pCertContext */
CERT_KEY_PROV_HANDLE_PROP_ID, /* dwPropId */
0, /* dwFlags */
(const void *) provider); /* pvData */
if (success) {
TRACE ("%s", "Successfully loaded client certificate");
return cert;
if (!success) {
MONGOC_ERROR ("Can't associate private key with public key: 0x%.8X", (unsigned int) GetLastError ());
goto fail;
}

MONGOC_ERROR ("Can't associate private key with public key: 0x%.8X", (unsigned int) GetLastError ());
TRACE ("%s", "Successfully loaded client certificate");
ret = true;

fail:
SecureZeroMemory (pem, pem_length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

   if (pem) {
      SecureZeroMemory (pem, pem_length);
      bson_free (pem);
   }

If read_file_and_null_terminate fails and returns NULL, this call to SecureZeroMemory is invalid (it does not accept a null pointer according to its _In_ annotation).

entering windows exception handler
exception '(access violation)' at '0x00007FF6A2010AE3', terminating
begin stack trace
test-libmongoc : RtlSecureZeroMemory
test-libmongoc : mongoc_secure_channel_setup_certificate_from_file

Expand All @@ -231,7 +299,14 @@ mongoc_secure_channel_setup_certificate_from_file (const char *filename)
bson_free (blob_private);
}

return NULL;
if (!ret) {
if (cert) {
CertFreeCertificateContext (cert);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
return NULL;
}

return cert;
}

PCCERT_CONTEXT
Expand All @@ -244,62 +319,42 @@ mongoc_secure_channel_setup_certificate (mongoc_stream_tls_secure_channel_t *sec
bool
mongoc_secure_channel_setup_ca (mongoc_stream_tls_secure_channel_t *secure_channel, mongoc_ssl_opt_t *opt)
{
FILE *file;
long length;
bool ok = false;
char *pem = NULL;
const char *pem_key;
HCERTSTORE cert_store = NULL;
PCCERT_CONTEXT cert = NULL;
DWORD encrypted_cert_len = 0;
LPBYTE encrypted_cert = NULL;
Comment on lines 327 to 328
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


file = fopen (opt->ca_file, "rb");
if (!file) {
MONGOC_ERROR ("Couldn't open file '%s'", opt->ca_file);
return false;
}

fseek (file, 0, SEEK_END);
length = ftell (file);
fseek (file, 0, SEEK_SET);
if (length < 1 || length > LONG_MAX - 1) {
MONGOC_WARNING ("Couldn't determine file size of '%s'", opt->ca_file);
fclose (file);
return false;
}

// Read the whole file into one nul-terminated string
pem_key = (const char *) bson_malloc0 ((size_t) length + 1u);
bool read_ok = (size_t) length == fread ((void *) pem_key, 1, length, file);
fclose (file);
if (!read_ok) {
MONGOC_WARNING ("Couldn't read certificate file '%s'", opt->ca_file);
pem = read_file_and_null_terminate (opt->ca_file, NULL);
if (!pem) {
return false;
}

/* If we have private keys or other fuzz, seek to the good stuff */
pem_key = strstr (pem_key, "-----BEGIN CERTIFICATE-----");
/*printf ("%s\n", pem_key);*/
pem_key = strstr (pem, "-----BEGIN CERTIFICATE-----");

if (!pem_key) {
MONGOC_WARNING ("Couldn't find certificate in '%s'", opt->ca_file);
return false;
goto fail;
}

if (!CryptStringToBinaryA (pem_key, 0, CRYPT_STRING_BASE64HEADER, NULL, &encrypted_cert_len, NULL, NULL)) {
MONGOC_ERROR ("Failed to convert BASE64 public key. Error 0x%.8X", (unsigned int) GetLastError ());
return false;
goto fail;
}

encrypted_cert = (LPBYTE) LocalAlloc (0, encrypted_cert_len);
if (!CryptStringToBinaryA (pem_key, 0, CRYPT_STRING_BASE64HEADER, encrypted_cert, &encrypted_cert_len, NULL, NULL)) {
MONGOC_ERROR ("Failed to convert BASE64 public key. Error 0x%.8X", (unsigned int) GetLastError ());
return false;
goto fail;
}

cert = CertCreateCertificateContext (X509_ASN_ENCODING, encrypted_cert, encrypted_cert_len);
if (!cert) {
MONGOC_WARNING ("Could not convert certificate");
return false;
goto fail;
}


Expand All @@ -311,17 +366,24 @@ mongoc_secure_channel_setup_ca (mongoc_stream_tls_secure_channel_t *secure_chann

if (cert_store == NULL) {
MONGOC_ERROR ("Error opening certificate store");
return false;
goto fail;
}

if (CertAddCertificateContextToStore (cert_store, cert, CERT_STORE_ADD_USE_EXISTING, NULL)) {
TRACE ("%s", "Added the certificate !");
if (!CertAddCertificateContextToStore (cert_store, cert, CERT_STORE_ADD_USE_EXISTING, NULL)) {
MONGOC_WARNING ("Failed adding the cert");
CertCloseStore (cert_store, 0);
return true;
goto fail;
}
MONGOC_WARNING ("Failed adding the cert");
CertCloseStore (cert_store, 0);

TRACE ("%s", "Added the certificate !");
CertCloseStore (cert_store, 0);
ok = true;
fail:
LocalFree (encrypted_cert);
if (cert) {
CertFreeCertificateContext (cert);
}
bson_free (pem);
return false;
}

Expand Down
1 change: 1 addition & 0 deletions src/libmongoc/src/mongoc/mongoc-stream-tls-secure-channel-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ typedef enum {
typedef struct {
CredHandle cred_handle;
TimeStamp time_stamp;
PCCERT_CONTEXT cert; /* optional client cert. Kept to free later */
Copy link
Collaborator

@eramongodb eramongodb May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PCCERT_CONTEXT cert; /* optional client cert. Kept to free later */
PCCERT_CONTEXT cert; /* Owning. Optional client cert. */

Minor wording suggestion.

} mongoc_secure_channel_cred;

typedef struct {
Expand Down
7 changes: 7 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-stream-tls-secure-channel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ _mongoc_stream_tls_secure_channel_destroy (mongoc_stream_t *stream)
/* if the handle was not cached and the refcount is zero */
TRACE ("%s", "clear credential handle");
FreeCredentialsHandle (&secure_channel->cred->cred_handle);
if (secure_channel->cred->cert != NULL) {
CertFreeCertificateContext (secure_channel->cred->cert);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating headers reveal the pCertContext is annotated with _In_opt_ indicating null pointers are permitted. Therefore this manual null check is likely unnecessary.

}
bson_free (secure_channel->cred);
}

Expand Down Expand Up @@ -932,6 +935,10 @@ mongoc_stream_tls_secure_channel_new (mongoc_stream_t *base_stream, const char *
schannel_cred.grbitEnabledProtocols = SP_PROT_TLS1_1_CLIENT | SP_PROT_TLS1_2_CLIENT;

secure_channel->cred = (mongoc_secure_channel_cred *) bson_malloc0 (sizeof (mongoc_secure_channel_cred));
if (cert) {
// Store client cert to free later.
secure_channel->cred->cert = cert;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Microsoft docs are vague about lifetimes and reference vs. copy semantics. When I'm in doubt about lifetimes and memory semantics, I like to look for Rust bindings. If my reading of schannel-rs is correct, AcquireCredentialsHandle has copy semantics and it's safe to free the certificate immediately after obtaining the credential handle without storing it separately.

https://github.com/steffengy/schannel-rs/blob/master/src/schannel_cred.rs

}

/* Example:
* https://msdn.microsoft.com/en-us/library/windows/desktop/aa375454%28v=vs.85%29.aspx
Expand Down