Merge pull request #553 from rod-hynes/master

Dial port 0 mitigations

psiphon/UDPConn.go

@@ -50,6 +50,10 @@ func NewUDPConn(
 		return nil, nil, errors.Trace(err)
 	}
 
+	if port <= 0 || port >= 65536 {
+		return nil, nil, errors.Tracef("invalid destination port: %d", port)
+	}
+
 	ipAddrs, err := LookupIP(ctx, host, config)
 	if err != nil {
 		return nil, nil, errors.Trace(err)

psiphon/common/protocol/protocol.go

@@ -220,6 +220,14 @@ func TunnelProtocolSupportsPassthrough(protocol string) bool {
 		protocol == TUNNEL_PROTOCOL_UNFRONTED_MEEK_SESSION_TICKET
 }
 
+func TunnelProtocolSupportsUpstreamProxy(protocol string) bool {
+
+	// TODO: Marionette UDP formats are incompatible with
+	// UpstreamProxy, but not currently supported.
+
+	return !TunnelProtocolUsesQUIC(protocol)
+}
+
 func UseClientTunnelProtocol(
 	clientProtocol string,
 	serverProtocols TunnelProtocols) bool {

psiphon/common/protocol/serverEntry.go

@@ -483,9 +483,7 @@ func (serverEntry *ServerEntry) GetSupportedProtocols(
 
 	for _, protocol := range SupportedTunnelProtocols {
 
-		// TODO: Marionette UDP formats are incompatible with
-		// useUpstreamProxy, but not currently supported
-		if useUpstreamProxy && TunnelProtocolUsesQUIC(protocol) {
+		if useUpstreamProxy && !TunnelProtocolSupportsUpstreamProxy(protocol) {
 			continue
 		}
 

psiphon/common/quic/quic.go

@@ -202,6 +202,13 @@ func Dial(
 		return nil, errors.Tracef("unsupported version: %s", negotiateQUICVersion)
 	}
 
+	// Fail if the destination port is invalid. Network operations should fail
+	// quickly in this case, but IETF quic-go has been observed to timeout,
+	// instead of failing quickly, in the case of invalid destination port 0.
+	if remoteAddr.Port <= 0 || remoteAddr.Port >= 65536 {
+		return nil, errors.Tracef("invalid destination port: %d", remoteAddr.Port)
+	}
+
 	if isObfuscated(negotiateQUICVersion) {
 		var err error
 		packetConn, err = NewObfuscatedPacketConn(

psiphon/dialParameters.go

@@ -329,6 +329,17 @@ func MakeDialParameters(
 		dialParams.TunnelProtocol = selectedProtocol
 	}
 
+	if config.UseUpstreamProxy() &&
+		!protocol.TunnelProtocolSupportsUpstreamProxy(dialParams.TunnelProtocol) {
+
+		// When UpstreamProxy is configured, ServerEntry.GetSupportedProtocols, when
+		// called via selectProtocol, will filter out protocols such that will not
+		// select a protocol incompatible with UpstreamProxy. This additional check
+		// will catch cases where selectProtocol does not apply this filter.
+		return nil, errors.Tracef(
+			"protocol does not support upstream proxy: %s", dialParams.TunnelProtocol)
+	}
+
 	if (!isReplay || !replayBPF) &&
 		ClientBPFEnabled() &&
 		protocol.TunnelProtocolUsesTCP(dialParams.TunnelProtocol) {

psiphon/server/api.go

@@ -663,18 +663,30 @@ func statusAPIRequestHandler(
 			//
 			// IsValidServerEntryTag ensures that the local copy of psinet is not stale
 			// before returning a negative result, to mitigate accidental pruning.
+			//
+			// In addition, when the reported dial port number is 0, flag the server
+			// entry as invalid to trigger client pruning. This covers a class of
+			// invalid/semi-functional server entries, found in practice to be stored
+			// by clients, where some protocol port number has been omitted -- due to
+			// historical bugs in various server entry handling implementations. When
+			// missing from a server entry loaded by a client, the port number
+			// evaluates to 0, the zero value, which is not a valid port number even if
+			// were not missing.
 
-			var serverEntryTagStr string
+			serverEntryTag, ok := getOptionalStringRequestParam(failedTunnelStat, "server_entry_tag")
 
-			serverEntryTag, ok := failedTunnelStat["server_entry_tag"]
 			if ok {
-				serverEntryTagStr, ok = serverEntryTag.(string)
-			}
+				serverEntryValid := db.IsValidServerEntryTag(serverEntryTag)
+
+				if serverEntryValid {
+					dialPortNumber, err := getIntStringRequestParam(failedTunnelStat, "dial_port_number")
+					if err == nil && dialPortNumber == 0 {
+						serverEntryValid = false
+					}
+				}
 
-			if ok {
-				serverEntryValid := db.IsValidServerEntryTag(serverEntryTagStr)
 				if !serverEntryValid {
-					invalidServerEntryTags[serverEntryTagStr] = true
+					invalidServerEntryTags[serverEntryTag] = true
 				}
 
 				// Add a field to the failed_tunnel log indicating if the server entry is

psiphon/server/server_test.go

@@ -2010,6 +2010,7 @@ type pruneServerEntryTestCase struct {
 	PsinetValid       bool
 	ExpectPrune       bool
 	IsEmbedded        bool
+	DialPort0         bool
 	ServerEntryFields protocol.ServerEntryFields
 }
 
@@ -2031,6 +2032,7 @@ func initializePruneServerEntriesTest(
 	// - ExpectPrune: prune outcome based on flags above
 	// - IsEmbedded: pruned embedded server entries leave a tombstone and cannot
 	//   be reimported
+	// - DialPort0: set dial port to 0, a special prune case (see statusAPIRequestHandler)
 
 	pruneServerEntryTestCases := []*pruneServerEntryTestCase{
 		&pruneServerEntryTestCase{IPAddress: "192.0.2.1", ExplicitTag: true, LocalTimestamp: newTimeStamp, PsinetValid: true, ExpectPrune: false},
@@ -2043,15 +2045,23 @@ func initializePruneServerEntriesTest(
 		&pruneServerEntryTestCase{IPAddress: "192.0.2.8", ExplicitTag: false, LocalTimestamp: oldTimeStamp, PsinetValid: false, ExpectPrune: true, IsEmbedded: false},
 		&pruneServerEntryTestCase{IPAddress: "192.0.2.9", ExplicitTag: true, LocalTimestamp: oldTimeStamp, PsinetValid: false, ExpectPrune: true, IsEmbedded: true},
 		&pruneServerEntryTestCase{IPAddress: "192.0.2.10", ExplicitTag: false, LocalTimestamp: oldTimeStamp, PsinetValid: false, ExpectPrune: true, IsEmbedded: true},
+		&pruneServerEntryTestCase{IPAddress: "192.0.2.11", ExplicitTag: true, LocalTimestamp: oldTimeStamp, PsinetValid: true, ExpectPrune: true, IsEmbedded: false, DialPort0: true},
+		&pruneServerEntryTestCase{IPAddress: "192.0.2.12", ExplicitTag: false, LocalTimestamp: oldTimeStamp, PsinetValid: true, ExpectPrune: true, IsEmbedded: true, DialPort0: true},
+		&pruneServerEntryTestCase{IPAddress: "192.0.2.13", ExplicitTag: true, LocalTimestamp: oldTimeStamp, PsinetValid: true, ExpectPrune: true, IsEmbedded: true, DialPort0: true},
 	}
 
 	for _, testCase := range pruneServerEntryTestCases {
 
+		dialPort := 4000
+		if testCase.DialPort0 {
+			dialPort = 0
+		}
+
 		_, _, _, _, encodedServerEntry, err := GenerateConfig(
 			&GenerateConfigParams{
 				ServerIPAddress:     testCase.IPAddress,
 				WebServerPort:       8000,
-				TunnelProtocolPorts: map[string]int{runConfig.tunnelProtocol: 4000},
+				TunnelProtocolPorts: map[string]int{runConfig.tunnelProtocol: dialPort},
 			})
 		if err != nil {
 			t.Fatalf("GenerateConfig failed: %s", err)