Open JonSnowWhite opened 8 months ago
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.
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!
I'd still vouch for the acceptance of 0x0303
Done: https://github.com/ValdikSS/GoodbyeDPI/commit/4c846c712dfedffd1e28405af4240ca6932414a3
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:
That probably would not work in all cases, but if we always have TLS and always do record fragmentation, it should work.
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
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.
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.