Skip to content
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.

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

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

Open
davidben opened this issue Jul 27, 2022 · 12 comments
Open

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

davidben opened this issue Jul 27, 2022 · 12 comments
Labels
topic-SSL type-bug An unexpected behavior, bug, or error type-security A security issue

Comments

@davidben
Copy link
Contributor

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.

@davidben davidben added the type-bug An unexpected behavior, bug, or error label Jul 27, 2022
@Neustradamus
Copy link

Neustradamus commented Jul 27, 2022

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:

  • tls-unique for TLS =< 1.2
  • tls-server-end-point
  • tls-exporter for TLS = 1.3

Linked to:

@tiran
Copy link
Member

tiran commented Jul 28, 2022

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
Copy link
Contributor Author

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
Copy link
Member

tiran commented Jul 28, 2022

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
Copy link
Member

tiran commented Jul 28, 2022

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
Copy link
Contributor Author

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

@stephanbosch
Copy link

stephanbosch commented Nov 8, 2023

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
Copy link

Neustradamus commented Nov 8, 2023

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
Copy link

stephanbosch commented Nov 9, 2023

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
Copy link

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

@stephanbosch
Copy link

@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
Copy link

Dear @python team,

It is possible to look for @tiran PR:

Security is important!

  • tls-unique for TLS =< 1.2
  • tls-server-end-point
  • tls-exporter for TLS = 1.3

Several projects/products wait you...

@erlend-aasland erlend-aasland added the type-security A security issue label Jan 4, 2024
Neustradamus referenced this issue Oct 26, 2024
…o as

to get channel binding data for the current SSL session (only the
"tls-unique" channel binding is implemented).  This allows the
implementation of certain authentication mechanisms such as SCRAM-SHA-1-PLUS.

Patch by Jacek Konieczny.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-SSL type-bug An unexpected behavior, bug, or error type-security A security issue
Projects
None yet
Development

No branches or pull requests

6 participants