Skip to content

bpo-37702: Fix SSL certificate-store-handles leak #14999

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

Closed
neonene wants to merge 12 commits into from
Closed

bpo-37702: Fix SSL certificate-store-handles leak #14999

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8ba1079
bpo-37702: Fix SSL certificate-store-handles leak
neonene Jul 29, 2019
bdfd6ba
Update 2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst
neonene Aug 24, 2019
5bbf09a
Update Modules/_ssl.c
neonene Aug 24, 2019
f8b70e0
Update _ssl.c
neonene Aug 24, 2019
a814e6a
Update _ssl.c
neonene Aug 25, 2019
a62f754
bpo-37702: comment for function call to each handle
neonene Aug 25, 2019
f12ab78
close handles in ssl_collect_certificates()
neonene Aug 26, 2019
5e079bf
comments (correct english)
neonene Aug 26, 2019
2963e03
indent, comments (correct english)
neonene Aug 27, 2019
e47aa04
make the system store list a global static
neonene Aug 27, 2019
9c09ed7
surround #define replacement with parenthesis
neonene Aug 28, 2019
b03a553
add a function to close handles
neonene Aug 29, 2019
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
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix memory leak on Windows in creating an SSLContext object or
running urllib.request.urlopen('https://...').
101 changes: 69 additions & 32 deletions Modules/_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5544,51 +5544,49 @@ parseKeyUsage(PCCERT_CONTEXT pCertCtx, DWORD flags)
}

static HCERTSTORE
ssl_collect_certificates(const char *store_name)
ssl_collect_certificates(const char *store_name,
DWORD *system_stores, size_t system_stores_count,
HCERTSTORE *system_store_handles)
{
/* this function collects the system certificate stores listed in
* system_stores into a collection certificate store for being
* enumerated. The store must be readable to be added to the
* store collection.
*/

HCERTSTORE hCollectionStore = NULL, hSystemStore = NULL;
static DWORD system_stores[] = {
CERT_SYSTEM_STORE_LOCAL_MACHINE,
CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE,
CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY,
CERT_SYSTEM_STORE_CURRENT_USER,
CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY,
CERT_SYSTEM_STORE_SERVICES,
CERT_SYSTEM_STORE_USERS};
HCERTSTORE hCollectionStore, hSystemStore;
size_t i, storesAdded;
BOOL result;

hCollectionStore = CertOpenStore(CERT_STORE_PROV_COLLECTION, 0,
(HCRYPTPROV)NULL, 0, NULL);
if (!hCollectionStore) {
return NULL;
}
storesAdded = 0;
for (i = 0; i < sizeof(system_stores) / sizeof(DWORD); i++) {
for (i = 0; i < system_stores_count; i++) {
system_store_handles[i] = NULL;
hSystemStore = CertOpenStore(CERT_STORE_PROV_SYSTEM_A, 0,
(HCRYPTPROV)NULL,
CERT_STORE_READONLY_FLAG |
system_stores[i], store_name);
if (hSystemStore) {
result = CertAddStoreToCollection(hCollectionStore, hSystemStore,
CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG, 0);
if (result) {
++storesAdded;
system_store_handles[i] = hSystemStore;
if (CertAddStoreToCollection(hCollectionStore, hSystemStore,
CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG, 0)) {
storesAdded = 1;
}
}
}
if (storesAdded == 0) {
CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG);
return NULL;
if (storesAdded) {
return hCollectionStore;
}

return hCollectionStore;
CertCloseStore(hCollectionStore, 0);
for (i = 0; i < system_stores_count; i++) {
if (system_store_handles[i]) {
CertCloseStore(system_store_handles[i], 0);
}
}
return NULL;
}

/* code from Objects/listobject.c */
Expand Down Expand Up @@ -5626,12 +5624,25 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name)
PCCERT_CONTEXT pCertCtx = NULL;
PyObject *keyusage = NULL, *cert = NULL, *enc = NULL, *tup = NULL;
PyObject *result = NULL;
static DWORD system_stores[] = {
CERT_SYSTEM_STORE_LOCAL_MACHINE,
CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE,
CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY,
CERT_SYSTEM_STORE_CURRENT_USER,
CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY,
CERT_SYSTEM_STORE_SERVICES,
CERT_SYSTEM_STORE_USERS};
size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD);
HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)];

result = PyList_New(0);
if (result == NULL) {
return NULL;
}
hCollectionStore = ssl_collect_certificates(store_name);
hCollectionStore = ssl_collect_certificates(store_name,
system_stores,
system_stores_count,
system_store_handles);
if (hCollectionStore == NULL) {
Py_DECREF(result);
return PyErr_SetFromWindowsErr(GetLastError());
Expand Down Expand Up @@ -5686,15 +5697,21 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name)
Py_XDECREF(keyusage);
Py_XDECREF(tup);

/* CERT_CLOSE_STORE_FORCE_FLAG forces freeing of memory for all contexts
associated with the store, in this case our collection store and the
associated system stores. */
if (!CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG)) {
/* CertCloseStore() with any argument can't close handles together.
In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended),
we must close hCollectionStore before we close each hSystemStore. */
BOOL success = CertCloseStore(hCollectionStore, 0);
for (i = 0; i < system_stores_count; i++) {
if (system_store_handles[i] &&
!CertCloseStore(system_store_handles[i], 0)) {
success = 0;
}
}
if (!success) {
/* This error case might shadow another exception.*/
Py_XDECREF(result);
return PyErr_SetFromWindowsErr(GetLastError());
}

return result;
}

Expand All @@ -5718,12 +5735,25 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name)
PCCRL_CONTEXT pCrlCtx = NULL;
PyObject *crl = NULL, *enc = NULL, *tup = NULL;
PyObject *result = NULL;
static DWORD system_stores[] = {
CERT_SYSTEM_STORE_LOCAL_MACHINE,
CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE,
CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY,
CERT_SYSTEM_STORE_CURRENT_USER,
CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY,
CERT_SYSTEM_STORE_SERVICES,
CERT_SYSTEM_STORE_USERS};
size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD);
HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)];

result = PyList_New(0);
if (result == NULL) {
return NULL;
}
hCollectionStore = ssl_collect_certificates(store_name);
hCollectionStore = ssl_collect_certificates(store_name,
system_stores,
system_stores_count,
system_store_handles);
if (hCollectionStore == NULL) {
Py_DECREF(result);
return PyErr_SetFromWindowsErr(GetLastError());
Expand Down Expand Up @@ -5767,10 +5797,17 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name)
Py_XDECREF(enc);
Py_XDECREF(tup);

/* CERT_CLOSE_STORE_FORCE_FLAG forces freeing of memory for all contexts
associated with the store, in this case our collection store and the
associated system stores. */
if (!CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG)) {
/* CertCloseStore() with any argument can't close handles together.
In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended),
we must close hCollectionStore before we close each hSystemStore. */
BOOL success = CertCloseStore(hCollectionStore, 0);
for (i = 0; i < system_stores_count; i++) {
if (system_store_handles[i] &&
!CertCloseStore(system_store_handles[i], 0)) {
success = 0;
}
}
if (!success) {
/* This error case might shadow another exception.*/
Py_XDECREF(result);
return PyErr_SetFromWindowsErr(GetLastError());
Expand Down