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

EAP-TLS 1.3 client is expecting session_tickets when it shouldn't #388

Open
Evengard opened this issue Jan 23, 2023 · 26 comments
Open

EAP-TLS 1.3 client is expecting session_tickets when it shouldn't #388

Evengard opened this issue Jan 23, 2023 · 26 comments
Assignees

Comments

@Evengard
Copy link

Evengard commented Jan 23, 2023

Hello!

It seems I stumbled upon something weird in EAP-TLS code for TLS 1.3.
The EAP-TLS code expects the handshake to complete with session tickets instead of both Client and Server sending TLS Finished. This is actually not needed for the EAP-TLS case (we already have all the needed information to complete the EAP-TLS flow - namely thee client certificate) and even more, it actually is different from other EAP-TLS 1.3 implementations (namely Windows 11 VPN client supports EAP-TLS 1.3, and it works without the session keys - even more, it actually breaks on them). Why this expectation was introduced? I see it was introduced specifically for TLS 1.3 only:

ppp/pppd/eap-tls.c

Lines 807 to 815 in ba7f7e0

int eaptls_is_init_finished(struct eaptls_session *ets)
{
if (ets->ssl && SSL_is_init_finished(ets->ssl))
{
if (ets->tls_v13)
return have_session_ticket;
else
return 1;
}

@Evengard
Copy link
Author

Evengard commented Jan 23, 2023

ppp/pppd/eap-tls.c

Lines 580 to 586 in ba7f7e0

/* Set up a SSL Session cache with a callback. This is needed for TLSv1.3+.
* During the initial handshake the server signals to the client early on
* that the handshake is finished, even before the client has sent its
* credentials to the server. The actual connection (and moment that the
* client sends its credentials) only starts after the arrival of the first
* session ticket. The 'ssl_new_session_cb' catches this ticket.
*/

This is kinda incorrect, as the TLS 1.3 handshake is considered completed when both server and client sent a TLS Finished message, and all we need at this point is just an ACK (or an EAP Success message actually) from the server side that the data was successfully received, we do NOT need to wait for the session tickets here.

@Neustradamus
Copy link
Member

@jjkeijser, @enaess: Can you look?

@enaess
Copy link
Contributor

enaess commented Jan 24, 2023

@Neustradamus not sure if I have much time right now to jump on this. I think much of this code is a brain child of @jjkeijser.

Perhaps if there is a simple way for us to reproduce this, e.g. a instance we can connect to to reproduce this or a set of instructions on how to setup an instance locally would help expedite the process, @Evengard

He could try to email @jjkeijser privately to share the details

@enaess
Copy link
Contributor

enaess commented Jan 24, 2023

Also, this shouldn't block a 2.5.0 release. While it would be nice to include it, I have a feeling there will be some complaints or bugs found that would demand a 2.5.1 release in the next 6-12 months

@jjkeijser
Copy link
Contributor

I did a quick analysys of the TLS 1.3 session ticket thing: I added TLS1.3 support in 2020, well before the RFC (RFC9190) that comes with it was approved. That RFC now says that a session ticket MAY be used. Why did I add the session ticket stuff to begin with? Because I was using OpenSSL 1.1+'s TLS1.3 implementation and for that I simply needed a session ticket to get it to work (and I had not found a way to disable TLS1.3 session tickets).

Having said that, next week I will have a look at the Windows 11 client and see how it works together with the ppp EAP-TLS code. I would like to come up with a solution where are scenarios are covered

  • pppd 2.5+ eap-tls 1.3 server + windows 11
  • pppd 2.5+ eap-tls 1.3 server + pppd 2.5+ eap-tls 1.3 client, with or without session tickets
  • pppd 2.5+ eap-tls 1.3 server + pppd 2.4.9 eap-tls 1.3 client, with or without session tickets
  • pppd 2.4.9 eap-tls 1.3 server + pppd 2.5+ eap-tls 1.3 client, with session tickets (as 2.4.9 does not disable them)

This will take some time to code and especially debug, so don't expect anything before mid next-week. If that is too long for the ppp 2.5.0 release, then I suggest to release it with the current semi-broken TLS1.3 support (and keep it marked as EXPERIMENTAL).

@Evengard
Copy link
Author

Evengard commented Jan 24, 2023

Just a heads up, disabling TLS 1.3 session tickets was added around OpenSSL 1.1.1, and is done like this:
SSL_CTX_set_num_tickets(ssl_ctx, 0);
I was actually developing the PPP stack for another VPN server (SoftEther), and implemented TLS 1.3 support for a number of different clients (actually managed to support the current PPPD implementation as well) so if you need any hints, I'd be happy to help.

@enaess
Copy link
Contributor

enaess commented Jan 30, 2023

Made any progress on this @jjkeijser

@jjkeijser
Copy link
Contributor

I have had a look at it and TLSv1.3 handling is broken in the current codebase, that is, it does not follow RFC9190. I am working on a patch but that patch will not be backwards compatible with the v2.4.9 code.

@Evengard
Copy link
Author

Surprisingly, I never knew there was a separate RFC for the EAP version of TLS 1.3... That's interesting.

@Evengard
Copy link
Author

Evengard commented Jan 31, 2023

A new information - Session Tickets on Windows actually works, as long as you send them in one chunk with the 0x00 AppData message as well directly after. Without the 0x00 AppData message Session Tickets under Windows won't work.

@Neustradamus
Copy link
Member

@jjkeijser: What do you think?

Respect the RFC9190 for 2.5.0 or not?

It is not possible to add a feature to use "the compatibility with 2.4.9"?
Linked to:

@jjkeijser
Copy link
Contributor

quick analysis:
In pppd 2.4.9 EAP-TLSv1.3 support is not working according to RFC9190. The current code does not write the 0x00 Appdata message at all, nor does it know how to process one , if a server sends one to it. This needs to be fixed for EAP-TLSv1.3 to work.

@Evengard : the reason there is actually an RFC for this is that the TLSv1.3 handshake changed WRT TLSv1.2, causing EAP-TLS to break. IMHO the 0x00 appdata message is little more than a kludge to work around this, but alas, this is what ended up the RFC texts. My original approach - rely on the session tickets - was just another kludge.

To change this is a bit of work that will take some time to write and above all, test. I think it would be good to include in v2.5.0 as it is kinda silly to bring out a "major new version" and not fix this.

@enaess
Copy link
Contributor

enaess commented Feb 1, 2023

The 2.5.0 release isn't under the pressure to make the next debian release anyways. That train left the station in early January. That means we can release when we are ready instead of putting a bandaid on something just to make it out the door.

This means, there is time where I can prep other upstream projects with PRs that will make their project compile with ppp-2.5.0. Package maintainers would likely appreciate that .. ;-)

@Evengard
Copy link
Author

Evengard commented Feb 2, 2023

The good news is, that sending the 0x00 AppData message alongside the session tickets doesn't break. So as long as the server sends both session tickets and the 0x00 AppData message, 2.4.9 client will work Just Fine.

@jjkeijser
Copy link
Contributor

over the past two days I spent several hours trying to send this "0x00 AppData message" from server to client (and to process/discard it on the client) but I have thus far failed.
I just cannot find how to send an "appdata" message instead of a message that is part of the EAP-TLS handshake - any help would be much appreciated. I've seen how hostapd does it, but I cannot translate it to the ways things are done in the pppd code.

@Evengard
Copy link
Author

Evengard commented Feb 3, 2023

It's actually pretty easy. The AppData is just regular encrypted TLS data.
TLS is just a wrapper around the inner protocol, right? All you need is to send through the TLS in encrypted form just one 0 byte. This will be the message.
In OpenSSL terms, you need to create an SSL BIO (BIO *ssl_bio = BIO_new(BIO_f_ssl())), attach it to your SSL object (BIO_set_ssl(ssl_bio, ssl, BIO_NOCLOSE)) and just write to that BIO after the handshake is complete and the authentication is successful

char data[1] = {0}
BIO_write(ssl_bio, data, 1);

After that you just get your output BIO and read it as usual - you'll have the AppData message in it

char tobesent[65536] = {0};
BIO_read(from_ssl, tobesent, 65536);

@Evengard
Copy link
Author

Evengard commented Feb 3, 2023

On the client you do basically the same thing, except in the other direction, you read from this same SSL BIO after pushing the data to the input BIO:

char datafromserver[65536];
int length = ...;
BIO_write(into_ssl, datafromserver, length);
char appmessage[65536] = {0};
int appmessagelength = BIO_read(ssl_bio, appmessage, 65536);
// Process appmessage

@enaess
Copy link
Contributor

enaess commented Feb 12, 2023

@jjkeijser made any progress on this?

@jjkeijser
Copy link
Contributor

No progress at all, as this is not high on my TODO list at the moment.

@enaess
Copy link
Contributor

enaess commented Feb 13, 2023

Fair. Do you still think this issue should block a 2.5.0 release?

I mean, if it was broken in 2.4.9 and it is still broken in 2.5.0, it isn't because of a regression.

Given all the changes (and features) that has been committed to depot since 2.4.9, it would be reasonable to think a 2.5.1 would be needed in a much shorter time horizon.

@jjkeijser
Copy link
Contributor

I never considered this a showstopper for 2.5.0 - there's a workaround (don't enable TLS 1.3). My hope was that I could fix it by staring at the code for a few hours, but alas, that did not work. So this will take more time which I don't have right now.

@Neustradamus
Copy link
Member

@jjkeijser: Have you looked it?

It will be nice to see before a 2.5.1 release build...

@jjkeijser
Copy link
Contributor

still no time to look at it - don't hold your breath for it

@Neustradamus
Copy link
Member

@jjkeijser: It is possible to look before 2.5.1 release?
It will be nice to solve this 1 year old ticket...

@Neustradamus
Copy link
Member

@jjkeijser: Have you progressed?

It will be nice to have a fix for 2.5.1...

@jjkeijser
Copy link
Contributor

No progress and like I said - don't hold your breath for it.
This is very low on my TODO list, so low that I had forgotten all about it for the past 8 months

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants