Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#26122 closed enhancement (wontfix)

obfs4: remove byte threshold for disconnection

Reported by: cypherpunks Owned by: dcf
Priority: Medium Milestone:
Component: Obfuscation/Obfsproxy Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by dcf)

As currently implemented, an obfs4 server disconnects an unauthenticated client after 8192–16383 received bytes or 30–90 seconds. (The exact values are chosen randomly from these ranges for each server.) The patch in comment:1 proposes to remove the byte threshold and keep the time threshold, as a mitigation against active-probing distinguishers such as the one in #26083.

Original description:

obfs4-spec.txt:

On the event of a failure at this point implementations SHOULD delay dropping the TCP connection from the client by a random interval to make active probing more difficult.

closeAfterDelay() can to violate spec by closing connection immediately.

Child Tickets

Change History (5)

comment:1 Changed 3 months ago by cypherpunks

Proposed fix:

-	// Consume and discard data on this connection until either the specified
-	// interval passes or a certain size has been reached.
-	discarded := 0
-	var buf [framing.MaximumSegmentLength]byte
-	for discarded < int(sf.closeDelayBytes) {
+	// Consume and discard data on this connection until the specified
+	// interval passes.
+	var buf [maxHandshakeLength]byte
+	for {
 		n, err := conn.Conn.Read(buf[:])
 		if err != nil {
 			return
 		}
-		discarded += n
 	}

This fix can also to stop some form of active probing attack discovered by #26083

comment:2 Changed 3 months ago by yawning

Resolution: wontfix
Status: newclosed

It does conform to the spec.

The interval and threshold is randomized on a per-server instance basis. Exactly how this is implemented (and even then there is a difference between SHOULD and MUST) is deliberately left up to the implementation, and the altered behavior is also fairly distinctive.

As a side note: I both recommended starting the development of something new, and the migration off obfs4 over a year ago.

comment:3 Changed 3 months ago by dcf

Yawning, I think this ticket was badly phrased wrt spec compliance. Since you don't want to maintain obfs4proxy anymore, could we at least leave the ticket open for discussion? As I read it, the patch is about an alternate (spec-compliant) behavior that may make active probing somewhat more expensive.

comment:4 Changed 3 months ago by yawning

As I read it, the patch is about an alternate (spec-compliant) behavior that may make active probing somewhat more expensive.

It doesn't.

comment:5 Changed 3 months ago by dcf

Component: Obfuscation/Censorship analysisObfuscation/Obfsproxy
Description: modified (diff)
Summary: obfs4proxy: closeAfterDelay() should to conform to obfs4 specobfs4: remove byte threshold for disconnection
Type: defectenhancement
Note: See TracTickets for help on using tickets.