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

TLS/SNI parsing #322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

JonSnowWhite
Copy link

This PR updates the TLS/SNI parsing to do actual protocol-parsing. It is more flexible on the TLS legacy version numbers and case of the hostname (see TODO). Feel free to take it or leave it.

This is actually a left-over from when I tried to implement TLS record fragmentation into Goodbye DPI. I had to abandon that as it invalidates the TCP SQN.

@ValdikSS
Copy link
Owner

Thanks for the contribution, however I'm not sure whether it's a good idea to parse TLS record per se: there's no benefit for doing that (GoodbyeDPI doesn't use any data from ClientHello, at least at this point), it's slower, it's more error-prone.

This is actually a left-over from when I tried to implement TLS record fragmentation into Goodbye DPI. I had to abandon that as it invalidates the TCP SQN.

You can recompute SEQ of every packet in userspace, or more interesting, we can add this into WinDivert (kernelspace) driver if you're still interesting in that function. Modern WinDivert versions support TCP flows.

@JonSnowWhite
Copy link
Author

JonSnowWhite commented May 31, 2024

I agree: as long as other parts of the ClientHello are unnecessary your implementation is easier and faster.

I'd still vouch for the acceptance of 0x0303 and 0x0302 as valid TLS versions in the record as some programs might not use the middlebox compliant 0x0301 (on my system the cups implementation uses 0x0303, not sure about specific others). That should not impact/break anything else. just saw that you implemented 0x0303 :^)

What do you mean by computing the SEQ in userspace?

I haven't worked with WinDivert yet but that sounds good and I'd be happy to help!

@ValdikSS
Copy link
Owner

I'd still vouch for the acceptance of 0x0303

Done: 4c846c7

What do you mean by computing the SEQ in userspace?

Look: to perform TLS record fragmentation, we need to add additional headers, right? And we know the size of the resulting packets.

What we could do is:

  1. Capture and save initial SEQ data of the first SYN/ACK from the OS
  2. Make our packets, which would result in increased final packet data size
  3. Send these packets with seq subtracted from the OS value, to get the SEQ of the same value as it would have been without any packet modification after inflated packets are sent
  4. Done — we don't need to monitor the session any further

That probably would not work in all cases, but if we always have TLS and always do record fragmentation, it should work.

@JonSnowWhite
Copy link
Author

You want to decrease the SEQ number of the initial SYN-ACK packets, right?

As you said we always need TLS for that and I imagine the seq number changes for the SYN-ACK packets to be a hassle, we would need to map the right ACK to the right SYN. Is it possible to override a socket's internal SEQ counter? That should solve everything

@ValdikSS
Copy link
Owner

You want to decrease the SEQ number of the initial SYN-ACK packets, right?

Yes, this is correct, as well as

we always need TLS for that

I'll think on what could be done, maybe there's simpler solution.

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

Successfully merging this pull request may close these issues.

2 participants