This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Expose SSL_CTX_set_cert_verify_callback
Type: security Stage: resolved
Components: SSL Versions: Python 3.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: David Peall, alex, christian.heimes, dstufft, janssen, msbrogli, ned.deily, ronaldoussoren, steve.dower
Priority: normal Keywords: patch

Created on 2016-11-19 21:36 by steve.dower, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
28747_1.patch steve.dower, 2016-11-19 21:37 review
28747_2.patch steve.dower, 2016-11-20 04:14
28747_3.patch steve.dower, 2016-11-21 20:22 review
Messages (19)
msg281230 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-19 21:36
As a prerequisite for fixing issues such as issue20916 (dynamic download/update of CAs and CRLs), we really need to be able to plug into the certificate verification function for OpenSSL.

This patch adds SSLContext._set_cert_verify_callback, which will allow Python code to inject its own verification function.

No other functionality is added, but I have proof-of-concept code that uses this patch to delegate all certificate handling to Windows and it works beautifully (better than I expected :) ).

If possible, I'd like to get this into Python 3.6. I intend to turn that proof-of-concept into an actual released library and would like to be able to do it sooner rather than later. Targeting 3.6 is the main reason I named the function with an underscore, but I'd be happy to drop it.
msg281231 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-19 21:37
Patch attached with code changes and basic tests. Doc changes to follow.
msg281233 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-19 21:39
Oh, I also made the SSL module chain exceptions properly. That's probably the biggest and scariest part of the change, but it can't have been overwriting exceptions before anyway (because it calls back into Python code to instantiate SSLError), so it's only going to chain in the new case of the callback function raising.
msg281235 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-11-19 23:08
Hi Steve,

there is a better approach to fix issue20916. The verify callback is not the correct API, because it is called too late. We want to hook into the cert resolution mechanism of OpenSSL and get trust anchors and CRLs in before OpenSSL builds the verification chain.

Instead of a verify cb we have to implement a X509_LOOKUP_METHOD with a get_by_subject(). The function looks up X509_LU_CRL or X509_LU_X509 by X509_NAME. The other lookups functions (fingerprint, issuer) aren't used to look up root CAs.

Then use some CAPI function like CertFindCertificateInStore() with CERT_FIND_SUBJECT_NAME to look up the cert, convert it to OpenSSL X509 object, copy the additional trust flags from Windows' cert type to the X509_CERT_AUX member of OpenSSL's X509 type.
msg281237 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-20 00:44
When I was stepping through, this callback avoided all of those lookups, so I don't understand how it's being called too late?

This approach basically takes the entire raw certificate and lets the OS do whatever it needs. OpenSSL doesn't ever have to crack it open at all (or at least when it does, it can assume the whole chain is trusted).

What am I missing here? I've got no doubt I'm missing something, as OpenSSL is possibly the most complicated code I've ever seen :)
msg281247 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-20 04:14
Few patch updates - better tests and some docs.
msg281260 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-11-20 10:52
IMHO SSL CTX set cert verify callback() is the wrong approach. Your are completely bypassing cert validation checks of OpenSSL. The callback has to build the chain and perform all checks on its own. By all checks I literally mean *all*, https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_cert_verify_callback(3)#WARNINGS

Basically you want to replace OpenSSL's X509 verification with Windows' cert validation and just leave the handshake and encryption to OpenSSL?
msg281262 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-20 12:53
> Basically you want to replace OpenSSL's X509 verification with Windows' cert validation and just leave the handshake and encryption to OpenSSL?

Yep. (See WinVerifyTrust for the Windows API I'm using.)
msg281318 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-21 02:30
Should have assigned this to me, as I expect I'll be the one to apply it.

Christian - I need to look to you for whether I've exposed the right function here and it's not adding security risk (obviously excluding a broken callback implementation). I *think* it's okay, but I trust your greater experience here.

3.6.0b4 is being tagged tomorrow and I think this is worth getting in - hence why I added Ned. There's no added functionality and no impact if the API isn't used. The latest patch removes the '_' prefix but happy to put it back and leave it as undocumented.

If neither of you have any concerns, I'll check it in. As I mentioned, at some point early in Python 3.6's release I should have a library available that lets the OS do certificate validation, but it needs this callback exposed.
msg281324 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-11-21 06:34
With the patch (_2), clang (and gcc 4.2) on macOS warn:

./Modules/_ssl.c:3968:7: warning: assigning to 'unsigned char *' from 'char *'
      converts between pointers to integer types with different sign [-Wpointer-sign]
    p = PyBytes_AS_STRING(enc_cert);
      ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
msg281325 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-11-21 06:45
And, as it stands, the tests fail (at least on macOS):

======================================================================
ERROR: test_set_cert_verify_callback (test.test_ssl.SimpleBackgroundTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/test/test_ssl.py", line 1782, in test_set_cert_verify_callback
    ctx._set_cert_verify_callback(callback)
AttributeError: 'SSLContext' object has no attribute '_set_cert_verify_callback'

======================================================================
ERROR: test_set_cert_verify_callback_error (test.test_ssl.SimpleBackgroundTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/test/test_ssl.py", line 1805, in test_set_cert_verify_callback_error
    ctx._set_cert_verify_callback(raise_error)
AttributeError: 'SSLContext' object has no attribute '_set_cert_verify_callback'

======================================================================
ERROR: test_set_cert_verify_callback_suppress_error (test.test_ssl.SimpleBackgroundTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/test/test_ssl.py", line 1827, in test_set_cert_verify_callback_suppress_error
    ctx._set_cert_verify_callback(raise_error)
AttributeError: 'SSLContext' object has no attribute '_set_cert_verify_callback'

----------------------------------------------------------------------
Ran 130 tests in 27.416s

FAILED (errors=3, skipped=8)
msg281326 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-11-21 06:50
We are two weeks from producing the release candidate for 3.6.0.  I don't think we should be rushing to add a new security-critical API which, IIUC, won't be used in the initial release anyway.  Let's target it for 3.7 after proper review and then we can decide whether it makes sense to backport to a 3.6.x maint release if the security issues it may solve warrant it.
msg281382 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-21 16:48
Whoops, that's what I get for renaming something. Easily fixed, but I'm happy to aim for 3.7.
msg281390 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-11-21 20:22
For the sake of review, I fixed the patch and rebased it on default.
msg283517 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-17 20:51
The current _3.patch builds on default without warning and the tests pass (_2.patch is the one Ned tried).

Any objections to committing this into 3.7?

What about 3.6.1? As an additive and easy to detect API, I think it's suitable, and I will certainly use it (right now my library's setup.py depends on having each libeay.lib from each original CPython build handy to get some of the functions out of it - these files are about 50MB each, so it's a little painful).

If it helps, I'm happy to add a warning to the docs that setting the callback may cause a loss of security if the callback does not properly validate the certificate (or some wording to that effect). Personally I think that's fairly well implied though, as there isn't any other obvious use for the callback.
msg283519 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-12-17 21:33
From a release manager perspective, I'm OK in principle with adding it to 3.6.1 as long as the ssl experts are OK with it.
msg284207 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-28 23:22
Any comment from the SSL experts?
msg309594 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-01-07 01:46
The change to make OpenSSL a separate DLL (on Windows, at least, which is all I really care about) means this function is available via ctypes. That's good enough for me, so I'll close this.
msg349645 - (view) Author: Marcelo Salhab Brogliato (msbrogli) Date: 2019-08-14 05:37
Exposing SSL_CTX_set_cert_verify_callback is useful when we want to use a Public Key Authentication, like it is done in the SSH Protocol.

Do you know any other way to use Public Key Authentication besides using SSL_CTX_set_cert_verify_callback?
History
Date User Action Args
2022-04-11 14:58:39adminsetgithub: 72933
2022-02-17 18:09:02christian.heimeslinkissue31242 superseder
2019-09-02 18:13:21David Peallsetnosy: + David Peall
2019-08-14 05:49:12yan12125setnosy: - yan12125
2019-08-14 05:37:22msbroglisetnosy: + msbrogli
messages: + msg349645
2018-01-07 01:46:30steve.dowersetstatus: open -> closed
resolution: out of date
messages: + msg309594

stage: patch review -> resolved
2017-02-24 10:05:54yan12125setnosy: + yan12125
2017-01-31 20:56:05ronaldoussorensetnosy: + ronaldoussoren
2016-12-28 23:22:22steve.dowersetmessages: + msg284207
2016-12-17 21:33:16ned.deilysetnosy: + janssen, alex, dstufft
messages: + msg283519
2016-12-17 20:51:14steve.dowersetmessages: + msg283517
2016-11-21 20:22:43steve.dowersetfiles: + 28747_3.patch

messages: + msg281390
2016-11-21 16:48:23steve.dowersetmessages: + msg281382
2016-11-21 06:50:57ned.deilysetmessages: + msg281326
versions: - Python 3.6
2016-11-21 06:45:50ned.deilysetmessages: + msg281325
2016-11-21 06:34:10ned.deilysetmessages: + msg281324
2016-11-21 02:30:23steve.dowersetassignee: christian.heimes -> steve.dower
messages: + msg281318
2016-11-20 12:53:35steve.dowersetmessages: + msg281262
2016-11-20 10:52:07christian.heimessetmessages: + msg281260
2016-11-20 04:14:37steve.dowersetfiles: + 28747_2.patch

messages: + msg281247
2016-11-20 00:44:42steve.dowersetmessages: + msg281237
2016-11-19 23:08:47christian.heimessetmessages: + msg281235
2016-11-19 21:39:21steve.dowersetmessages: + msg281233
2016-11-19 21:37:36steve.dowersetfiles: + 28747_1.patch
keywords: + patch
messages: + msg281231
2016-11-19 21:36:12steve.dowercreate