search

Jigsaw-Code / Intra

An experimental tool that allows you to test new DNS-over-HTTPS services on Android
Apache License 2.0
1.57k stars 263 forks source link

retrier: confirm splitHello to io.Writer.Write #520

Closed ignoramous closed 3 months ago

ignoramous commented 3 months ago

The way our Intra fork is written, writing to retrier (DuplexConn) via io.Copy results in panic when total written bytes from TLS fragmentation exceeds the length of the input bytes; expected since this behaviour violates the io.Writer.Write contract:

It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. Write must return a non-nil error if it returns n < len(p).

https://github.com/celzero/firestack/commit/ba0c57c3851ce8c08c2f392b57a1890a936c803c

ignoramous commented 3 months ago

Since this is a private function, and we are not using the returned bytes

splitter (which impls io.Writer via DuplexConn) does use n returned from splitHello.

https://github.com/Jigsaw-Code/Intra/blob/27637e0ed497dbd1e862750b14f9a5c0be368f15/Android/app/src/go/intra/split/direct_split.go#L57

Please do not use this function elsewhere. For reusable TLS fragmentation, use the Outline SDK. (We plan to refactor our code using the Outline SDK in the future as well)

Ah, thanks.

We may or may not subtract the header length from n, depending on the bytes that has been written ... I recommend leaving it unchanged

I didn't quite catch this. I guess what you're saying is, the splits might have written n / m bytes (partially so) that need to be subtracted from to account for record size changes due to fragmentation?

You reckon we change splitter.Write to confirm to io.Writer, or make more accurate accounting related changes in splitHello, or close this PR (as this issue needs to be fixed elsewhere, presumably in the outline-sdk)?

jyyi1 commented 3 months ago

Thanks @ignoramous for your response. I suggest closing this PR if its purpose is solely for third-party use (Outline SDK is a more suitable API with proper handling of written bytes), since we'll be refactoring this logic anyway.

However, if you're aware of any Intra use cases that might be affected, we may need to fix it.

ignoramous commented 3 months ago

we'll be refactoring this logic anyway.

Gotcha.

However, if you're aware of any Intra use cases that might be affected, we may need to fix it.

Well, only splitter (aka direct_split) but it isn't used by Intra anymore, even though (as it was recently brought to my attention), direct_split in fact works better in face of "residual censorship" than the retrier.

(appreciate for your input. closing...)