search

python / cpython

The Python programming language
https://www.python.org
Other
62.6k stars 30.04k forks source link

ssl module incorrectly supports tls-unique channel binding for TLS 1.3 #95341

Open davidben opened 2 years ago

davidben commented 2 years ago

CPython's get_channel_binding method implements the tls-unique channel binding for TLS 1.3: https://github.com/python/cpython/blob/main/Lib/test/test_ssl.py#L671-L681 https://github.com/python/cpython/blob/main/Modules/_ssl.c#L2705

But this is incorrect. tls-unique is vulnerable to a couple of attacks (3SHAKE, SLOTH), so it was left undefined in TLS 1.3. RFC 9266 defines a replacement tls-exporter binding, built with Export Keying Material instead.

Neustradamus commented 2 years ago

There are two possibilities for TLS Binding: TLS =< 1.2: RFC5929: Channel Bindings for TLS: https://tools.ietf.org/html/rfc5929 TLS = 1.3: RFC9266: Channel Bindings for TLS 1.3: https://tools.ietf.org/html/rfc9266

Details:

Linked to:

tiran commented 2 years ago

Meh, what a mess. Thanks for notifying us, David.

I have started a PR to implement tls-exporter on top of export keying materials API. Of course OpenSSL makes it annoyingly complicated to check for presence of ext master secret for TLS 1.2 connections. I had to fall back to SSL_ctrl. Did I get the RFC right regarding context value? If I understand the RFC correctly, then it wants an empty context, not no context.

davidben commented 2 years ago

Of course OpenSSL makes it annoyingly complicated to check for presence of ext master secret for TLS 1.2 connections. I had to fall back to SSL_ctrl.

Hmm. Does SSL_get_extms_support not work?

tiran commented 2 years ago

Hmm. Does SSL_get_extms_support not work?

Oh, there is a function for that? I somehow missed the fact that OpenSSL has an API function for extms...

tiran commented 2 years ago

Ah, it is implemented as a macro! That's why I could neither find a symbol in libssl nor a function in ssl/*.c.

davidben commented 2 years ago

Yeah all the SSL_ctrl nonsense is macros. I tried to convince upstream to make them real functions, but no dice.

stephanbosch commented 10 months ago

Looks like this Erlang library's C implementation provides an example on how to do this with OpenSSL.

https://github.com/processone/fast_tls/blob/c98c1a7d190201dc4113babed91fdfedac2bf42a/c_src/fast_tls.c#L1408

I implemented this for Dovecot SASL (in a feature branch) and to my frustration our python-based CI environment cannot be used to test it. I could test it against gsasl, which works fine.

Neustradamus commented 10 months ago

Dear all,

I think that you have seen the jabber.ru MITM:

@stephanbosch: You have done a lot of work about SCRAM in the past in Dovecot project, I am happy to see that you are on it, I can not wait to add Dovecot SASL in the list about -PLUS variants :)

Yes @prefiks has done improvements on it recently. A lot of softwares support all possibilities for Channel Binding.

It is important to know that it is needed for all SCRAM-SHA-*-PLUS (several RFCs listed in previous link) and specified in:

Thanks in advance.

PS: SCRAM has replaced old and unsecure DIGEST-MD5 and CRAM-MD5 which are in obsolete since 2011.

stephanbosch commented 10 months ago

Quickly patched python to run my CI test with tls-exporter channel binding support. This works. I have no idea of overall Python C coding, documentation and other steps needed to add a new Python feature and little time right now to dive into that. So, rather than a pull request, I've attached a patch to this comment which you can use as a starting point to add this for real.

python-tls-exporter.patch

Oh and it looks like I messed up the white space a bit.

Neustradamus commented 10 months ago

@stephanbosch: Have you seen this not-recent PR (but not merged)?

stephanbosch commented 10 months ago

@stephanbosch: Have you seen this not-recent PR (but not merged)?

* [gh-95341: Implement tls-exporter channel bindings and export key materials #95366](https://github.com/python/cpython/pull/95366)

No. That pr is a bit difficult to follow for me. Only the last commit makes sense to me. Anyway, judging by that last commit it seems to be on track for something that will work.

You can test interoperability with GnuSASL/GnuTLS; Dovecot support for tls-exporter and SCRAM-*-PLUS has low priority, so even though it is pretty much implemented, getting that merged will take some time.

Neustradamus commented 9 months ago

Dear @python team,

It is possible to look for @tiran PR:

Security is important!

Several projects/products wait you...