From 9d281987456f9c120b6171ebc316fa41cf60bf60 Mon Sep 17 00:00:00 2001 From: Tobiasz Laskowski Date: Thu, 5 Dec 2024 00:15:03 +0000 Subject: [PATCH 1/6] Use windows api if ssl cert verification fails This avoids rejecting certificates that should be accepted, see #80192 --- ...4-12-05-00-21-11.gh-issue-80192.28PTj4.rst | 1 + Modules/_ssl.c | 115 +++++++++++++++++- 2 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst diff --git a/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst b/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst new file mode 100644 index 00000000000000..e9a2b76257ba44 --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst @@ -0,0 +1 @@ +Fixed valid ssl certificates being rejected. diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 59c414f9ce1ceb..0eb5e6b9db4e63 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2996,6 +2996,119 @@ static PyType_Spec PySSLSocket_spec = { .slots = PySSLSocket_slots, }; +#ifdef _MSC_VER +static unsigned char * +_get_cert_bytes(X509 *cert, int *length) +{ + unsigned char *cert_bytes; + int cert_bytes_length = i2d_X509(cert, NULL); + if (cert_bytes_length <= 0) + { + return NULL; + } + + cert_bytes = PyMem_RawMalloc(cert_bytes_length); + if (cert_bytes == NULL) + { + return NULL; + } + + if (i2d_X509(cert, &cert_bytes) <= 0) + { + return NULL; + } + + *length = cert_bytes_length; + + /* i2d_X509 moves the input pointer to the end of the data */ + return cert_bytes - cert_bytes_length; +} + + +static int +_verify_callback(int preverify_ok, X509_STORE_CTX *ctx) +{ + int ret = 0; + /* windows api calls below do not check for hostname mismatch */ + if (preverify_ok || X509_STORE_CTX_get_error(ctx) == X509_V_ERR_HOSTNAME_MISMATCH) + { + return preverify_ok; + } + + HCERTSTORE store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, NULL); + if (store == NULL) + { + return ret; + } + + int cert_bytes_length; + unsigned char *cert_bytes = _get_cert_bytes(X509_STORE_CTX_get_current_cert(ctx), &cert_bytes_length); + if (cert_bytes == NULL) + { + goto error_1; + } + + PCCERT_CONTEXT primary_context = NULL; + if (!CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, cert_bytes, cert_bytes_length, CERT_STORE_ADD_REPLACE_EXISTING, &primary_context)) + { + goto error_2; + } + PCCERT_CHAIN_CONTEXT chain_context = NULL; + CERT_CHAIN_PARA parameters = {0}; + if (!CertGetCertificateChain(NULL, primary_context, NULL, store, ¶meters, 0, NULL, &chain_context)) + { + goto error_3; + } + CERT_CHAIN_POLICY_PARA policy_parameters = {0}; + CERT_CHAIN_POLICY_STATUS policy_status = {0}; + if (!CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_SSL, chain_context, &policy_parameters, &policy_status)) + { + goto error_4; + } + + switch(policy_status.dwError) { + case CERT_TRUST_NO_ERROR: + ret = 1; + break; + case TRUST_E_CERT_SIGNATURE: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_SIGNATURE_FAILURE); + break; + case CERT_E_UNTRUSTEDROOT: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_UNTRUSTED); + break; + case CERT_E_CHAINING: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT); + break; + case CERT_E_EXPIRED: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_HAS_EXPIRED); + break; + case CERT_E_INVALID_POLICY: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_INVALID_POLICY_EXTENSION); + break; + case CERT_E_WRONG_USAGE: + case CERT_E_CRITICAL: + case CERT_E_PURPOSE: + case CERT_E_ROLE: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_INVALID_PURPOSE); + break; + case CERT_E_VALIDITYPERIODNESTING: + default: + X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION); + } +error_4: + CertFreeCertificateChain(chain_context); +error_3: + CertFreeCertificateContext(primary_context); +error_2: + PyMem_RawFree(cert_bytes); +error_1: + CertCloseStore(store, 0); + return ret; +} +#else +#define _verify_callback NULL +#endif + /* * _SSLContext objects */ @@ -3024,7 +3137,7 @@ _set_verify_mode(PySSLContext *self, enum py_ssl_cert_requirements n) /* bpo-37428: newPySSLSocket() sets SSL_VERIFY_POST_HANDSHAKE flag for * server sockets and SSL_set_post_handshake_auth() for client. */ - SSL_CTX_set_verify(self->ctx, mode, NULL); + SSL_CTX_set_verify(self->ctx, mode, _verify_callback); return 0; } From d3da5df46d52bb7be79af71c1cb2458b20eb3274 Mon Sep 17 00:00:00 2001 From: Tobiasz Laskowski Date: Thu, 5 Dec 2024 01:07:22 +0000 Subject: [PATCH 2/6] Use const where appropriate --- Modules/_ssl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 0eb5e6b9db4e63..a2046a36b591a9 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2997,8 +2997,8 @@ static PyType_Spec PySSLSocket_spec = { }; #ifdef _MSC_VER -static unsigned char * -_get_cert_bytes(X509 *cert, int *length) +static const unsigned char * +_get_cert_bytes(const X509 *cert, int *length) { unsigned char *cert_bytes; int cert_bytes_length = i2d_X509(cert, NULL); @@ -3042,7 +3042,7 @@ _verify_callback(int preverify_ok, X509_STORE_CTX *ctx) } int cert_bytes_length; - unsigned char *cert_bytes = _get_cert_bytes(X509_STORE_CTX_get_current_cert(ctx), &cert_bytes_length); + const unsigned char *cert_bytes = _get_cert_bytes(X509_STORE_CTX_get_current_cert(ctx), &cert_bytes_length); if (cert_bytes == NULL) { goto error_1; @@ -3100,7 +3100,7 @@ _verify_callback(int preverify_ok, X509_STORE_CTX *ctx) error_3: CertFreeCertificateContext(primary_context); error_2: - PyMem_RawFree(cert_bytes); + PyMem_RawFree((void *)cert_bytes); error_1: CertCloseStore(store, 0); return ret; From b8f5bd5211ce6b835c56ede7752b01c2d5e7f54c Mon Sep 17 00:00:00 2001 From: Tobiasz Laskowski Date: Thu, 5 Dec 2024 16:49:06 +0000 Subject: [PATCH 3/6] Refactor error translation into separate function --- Modules/_ssl.c | 67 ++++++++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index a2046a36b591a9..9230b3a3ecfbf9 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -3025,6 +3025,32 @@ _get_cert_bytes(const X509 *cert, int *length) } +static int +_translate_policy_status_error(int error) +{ + switch (error) { + case TRUST_E_CERT_SIGNATURE: + return X509_V_ERR_CERT_SIGNATURE_FAILURE; + case CERT_E_UNTRUSTEDROOT: + return X509_V_ERR_CERT_UNTRUSTED; + case CERT_E_CHAINING: + return X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT; + case CERT_E_EXPIRED: + return X509_V_ERR_CERT_HAS_EXPIRED; + case CERT_E_INVALID_POLICY: + return X509_V_ERR_INVALID_POLICY_EXTENSION; + case CERT_E_WRONG_USAGE: + case CERT_E_CRITICAL: + case CERT_E_PURPOSE: + case CERT_E_ROLE: + return X509_V_ERR_INVALID_PURPOSE; + case CERT_E_VALIDITYPERIODNESTING: + default: + return X509_V_ERR_APPLICATION_VERIFICATION; + } +} + + static int _verify_callback(int preverify_ok, X509_STORE_CTX *ctx) { @@ -3061,41 +3087,18 @@ _verify_callback(int preverify_ok, X509_STORE_CTX *ctx) } CERT_CHAIN_POLICY_PARA policy_parameters = {0}; CERT_CHAIN_POLICY_STATUS policy_status = {0}; - if (!CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_SSL, chain_context, &policy_parameters, &policy_status)) + if (CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_SSL, chain_context, + &policy_parameters, &policy_status)) { - goto error_4; + if (policy_status.dwError == CERT_TRUST_NO_ERROR) { + ret = 1; + } + else { + int err = _translate_policy_status_error(policy_status.dwError); + X509_STORE_CTX_set_error(ctx, err); + } } - switch(policy_status.dwError) { - case CERT_TRUST_NO_ERROR: - ret = 1; - break; - case TRUST_E_CERT_SIGNATURE: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_SIGNATURE_FAILURE); - break; - case CERT_E_UNTRUSTEDROOT: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_UNTRUSTED); - break; - case CERT_E_CHAINING: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT); - break; - case CERT_E_EXPIRED: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_HAS_EXPIRED); - break; - case CERT_E_INVALID_POLICY: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_INVALID_POLICY_EXTENSION); - break; - case CERT_E_WRONG_USAGE: - case CERT_E_CRITICAL: - case CERT_E_PURPOSE: - case CERT_E_ROLE: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_INVALID_PURPOSE); - break; - case CERT_E_VALIDITYPERIODNESTING: - default: - X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION); - } -error_4: CertFreeCertificateChain(chain_context); error_3: CertFreeCertificateContext(primary_context); From c9ad0a362f549050e1296a8fa05fab390b8e94d4 Mon Sep 17 00:00:00 2001 From: Tobiasz Laskowski Date: Thu, 5 Dec 2024 16:51:57 +0000 Subject: [PATCH 4/6] Fix PEP 7 violations --- Modules/_ssl.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 9230b3a3ecfbf9..b36097e3d5d25e 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -3002,19 +3002,16 @@ _get_cert_bytes(const X509 *cert, int *length) { unsigned char *cert_bytes; int cert_bytes_length = i2d_X509(cert, NULL); - if (cert_bytes_length <= 0) - { + if (cert_bytes_length <= 0) { return NULL; } cert_bytes = PyMem_RawMalloc(cert_bytes_length); - if (cert_bytes == NULL) - { + if (cert_bytes == NULL) { return NULL; } - if (i2d_X509(cert, &cert_bytes) <= 0) - { + if (i2d_X509(cert, &cert_bytes) <= 0) { return NULL; } @@ -3056,32 +3053,38 @@ _verify_callback(int preverify_ok, X509_STORE_CTX *ctx) { int ret = 0; /* windows api calls below do not check for hostname mismatch */ - if (preverify_ok || X509_STORE_CTX_get_error(ctx) == X509_V_ERR_HOSTNAME_MISMATCH) + if (preverify_ok + || X509_STORE_CTX_get_error(ctx) == X509_V_ERR_HOSTNAME_MISMATCH) { return preverify_ok; } - HCERTSTORE store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, NULL); - if (store == NULL) - { + HCERTSTORE store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, + CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, + NULL); + if (store == NULL) { return ret; } int cert_bytes_length; - const unsigned char *cert_bytes = _get_cert_bytes(X509_STORE_CTX_get_current_cert(ctx), &cert_bytes_length); - if (cert_bytes == NULL) - { + const X509 *cert = X509_STORE_CTX_get_current_cert(ctx); + const BYTE *cert_bytes = _get_cert_bytes(cert, &cert_bytes_length); + if (cert_bytes == NULL) { goto error_1; } PCCERT_CONTEXT primary_context = NULL; - if (!CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, cert_bytes, cert_bytes_length, CERT_STORE_ADD_REPLACE_EXISTING, &primary_context)) + if (!CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, + cert_bytes, cert_bytes_length, + CERT_STORE_ADD_REPLACE_EXISTING, + &primary_context)) { goto error_2; } PCCERT_CHAIN_CONTEXT chain_context = NULL; CERT_CHAIN_PARA parameters = {0}; - if (!CertGetCertificateChain(NULL, primary_context, NULL, store, ¶meters, 0, NULL, &chain_context)) + if (!CertGetCertificateChain(NULL, primary_context, NULL, store, + ¶meters, 0, NULL, &chain_context)) { goto error_3; } From 80b21bdd8309e6bc01c10fa8eb4ac7f3dbf05426 Mon Sep 17 00:00:00 2001 From: Tobiasz Laskowski Date: Thu, 5 Dec 2024 17:10:11 +0000 Subject: [PATCH 5/6] Add reference to news entry --- .../next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst b/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst index e9a2b76257ba44..ac39c50dddba7a 100644 --- a/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst +++ b/Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst @@ -1 +1 @@ -Fixed valid ssl certificates being rejected. +Fixed valid :mod:`ssl` certificates being rejected. From 2435d4d6e408debb55b794df88bee42230096317 Mon Sep 17 00:00:00 2001 From: Tobiasz Laskowski Date: Thu, 5 Dec 2024 17:49:39 +0000 Subject: [PATCH 6/6] Fix error translation resulting in test fail CERT_E_UNTRUSTEDROOT: A certification chain processed correctly but terminated in a root certificate that is not trusted by the trust provider. X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY: The issuer certificate could not be found: this occurs if the issuer certificate of an untrusted certificate cannot be found. - https://docs.openssl.org/master/man3/X509_STORE_CTX_get_error/#error-codes - https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-cert_chain_policy_status#members --- Modules/_ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index b36097e3d5d25e..faaa3feb23793d 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -3029,7 +3029,7 @@ _translate_policy_status_error(int error) case TRUST_E_CERT_SIGNATURE: return X509_V_ERR_CERT_SIGNATURE_FAILURE; case CERT_E_UNTRUSTEDROOT: - return X509_V_ERR_CERT_UNTRUSTED; + return X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY; case CERT_E_CHAINING: return X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT; case CERT_E_EXPIRED: