In-proxy broker: filter out invalid SDP candidates instead of failing

psiphon/common/inproxy/api.go

@@ -575,14 +575,16 @@ func (request *ClientOfferRequest) ValidateAndGetLogFields(
 	lookupGeoIP LookupGeoIP,
 	baseAPIParameterValidator common.APIParameterValidator,
 	formatter common.APIParameterLogFieldFormatter,
-	geoIPData common.GeoIPData) (common.LogFields, error) {
+	geoIPData common.GeoIPData) ([]byte, common.LogFields, error) {
 
 	if len(request.CommonCompartmentIDs) > maxCompartmentIDs {
-		return nil, errors.Tracef("invalid compartment IDs length: %d", len(request.CommonCompartmentIDs))
+		return nil, nil, errors.Tracef(
+			"invalid compartment IDs length: %d", len(request.CommonCompartmentIDs))
 	}
 
 	if len(request.PersonalCompartmentIDs) > maxCompartmentIDs {
-		return nil, errors.Tracef("invalid compartment IDs length: %d", len(request.PersonalCompartmentIDs))
+		return nil, nil, errors.Tracef(
+			"invalid compartment IDs length: %d", len(request.PersonalCompartmentIDs))
 	}
 
 	// The client offer SDP may contain no ICE candidates.
@@ -590,10 +592,10 @@ func (request *ClientOfferRequest) ValidateAndGetLogFields(
 
 	// Client offer SDP candidate addresses must match the country and ASN of
 	// the client. Don't facilitate connections to arbitrary destinations.
-	sdpMetrics, err := validateSDPAddresses(
+	filteredSDP, sdpMetrics, err := filterSDPAddresses(
 		[]byte(request.ClientOfferSDP.SDP), errorOnNoCandidates, lookupGeoIP, geoIPData)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, nil, errors.Trace(err)
 	}
 
 	// The client's self-reported ICECandidateTypes are used instead of the
@@ -602,23 +604,24 @@ func (request *ClientOfferRequest) ValidateAndGetLogFields(
 	// indistinguishable from host candidate types.
 
 	if !request.ICECandidateTypes.IsValid() {
-		return nil, errors.Tracef("invalid ICE candidate types: %v", request.ICECandidateTypes)
+		return nil, nil, errors.Tracef(
+			"invalid ICE candidate types: %v", request.ICECandidateTypes)
 	}
 
 	if request.Metrics == nil {
-		return nil, errors.TraceNew("missing metrics")
+		return nil, nil, errors.TraceNew("missing metrics")
 	}
 
 	logFields, err := request.Metrics.ValidateAndGetLogFields(
 		baseAPIParameterValidator, formatter, geoIPData)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, nil, errors.Trace(err)
 	}
 
 	if request.TrafficShapingParameters != nil {
 		err := request.TrafficShapingParameters.Validate()
 		if err != nil {
-			return nil, errors.Trace(err)
+			return nil, nil, errors.Trace(err)
 		}
 	}
 
@@ -634,8 +637,9 @@ func (request *ClientOfferRequest) ValidateAndGetLogFields(
 	logFields["has_personal_compartment_ids"] = hasPersonalCompartmentIDs
 	logFields["ice_candidate_types"] = request.ICECandidateTypes
 	logFields["has_IPv6"] = sdpMetrics.hasIPv6
+	logFields["filtered_ice_candidates"] = sdpMetrics.filteredICECandidates
 
-	return logFields, nil
+	return filteredSDP, logFields, nil
 }
 
 // Validate validates the that client has not specified excess traffic shaping
@@ -675,17 +679,17 @@ func (request *ProxyAnswerRequest) ValidateAndGetLogFields(
 	lookupGeoIP LookupGeoIP,
 	baseAPIParameterValidator common.APIParameterValidator,
 	formatter common.APIParameterLogFieldFormatter,
-	geoIPData common.GeoIPData) (common.LogFields, error) {
+	geoIPData common.GeoIPData) ([]byte, common.LogFields, error) {
 
 	// The proxy answer SDP must contain at least one ICE candidate.
 	errorOnNoCandidates := true
 
 	// Proxy answer SDP candidate addresses must match the country and ASN of
 	// the proxy. Don't facilitate connections to arbitrary destinations.
-	sdpMetrics, err := validateSDPAddresses(
+	filteredSDP, sdpMetrics, err := filterSDPAddresses(
 		[]byte(request.ProxyAnswerSDP.SDP), errorOnNoCandidates, lookupGeoIP, geoIPData)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, nil, errors.Trace(err)
 	}
 
 	// The proxy's self-reported ICECandidateTypes are used instead of the
@@ -694,11 +698,13 @@ func (request *ProxyAnswerRequest) ValidateAndGetLogFields(
 	// indistinguishable from host candidate types.
 
 	if !request.ICECandidateTypes.IsValid() {
-		return nil, errors.Tracef("invalid ICE candidate types: %v", request.ICECandidateTypes)
+		return nil, nil, errors.Tracef(
+			"invalid ICE candidate types: %v", request.ICECandidateTypes)
 	}
 
 	if request.SelectedProxyProtocolVersion != ProxyProtocolVersion1 {
-		return nil, errors.Tracef("invalid select proxy protocol version: %v", request.SelectedProxyProtocolVersion)
+		return nil, nil, errors.Tracef(
+			"invalid select proxy protocol version: %v", request.SelectedProxyProtocolVersion)
 	}
 
 	logFields := formatter(geoIPData, common.APIParameters{})
@@ -706,9 +712,10 @@ func (request *ProxyAnswerRequest) ValidateAndGetLogFields(
 	logFields["connection_id"] = request.ConnectionID
 	logFields["ice_candidate_types"] = request.ICECandidateTypes
 	logFields["has_IPv6"] = sdpMetrics.hasIPv6
+	logFields["filtered_ice_candidates"] = sdpMetrics.filteredICECandidates
 	logFields["answer_error"] = request.AnswerError
 
-	return logFields, nil
+	return filteredSDP, logFields, nil
 }
 
 // ValidateAndGetLogFields validates the ClientRelayedPacketRequest and returns

psiphon/common/inproxy/broker.go

@@ -759,7 +759,15 @@ func (b *Broker) handleClientOffer(
 		return nil, errors.Trace(err)
 	}
 
-	logFields, err = offerRequest.ValidateAndGetLogFields(
+	// The filtered SDP is the request SDP with any invalid (bogon, unexpected
+	// GeoIP) ICE candidates filtered out. In some cases, clients cannot
+	// avoid submitting invalid candidates (see comment in
+	// processSDPAddresses), so all invalid candidates are removed and the
+	// remaining SDP is used. Filtered candidate information is logged in
+	// logFields.
+
+	var filteredSDP []byte
+	filteredSDP, logFields, err = offerRequest.ValidateAndGetLogFields(
 		int(atomic.LoadInt64(&b.maxCompartmentIDs)),
 		b.config.LookupGeoIP,
 		b.config.APIParameterValidator,
@@ -769,6 +777,9 @@ func (b *Broker) handleClientOffer(
 		return nil, errors.Trace(err)
 	}
 
+	offerSDP := offerRequest.ClientOfferSDP
+	offerSDP.SDP = string(filteredSDP)
+
 	// AllowClient may be used to disallow clients from certain geolocations
 	// from offering. Clients are always allowed to match proxies with shared
 	// personal compartment IDs.
@@ -818,7 +829,7 @@ func (b *Broker) handleClientOffer(
 			PortMappingTypes:       offerRequest.Metrics.PortMappingTypes,
 		},
 		ClientProxyProtocolVersion:  offerRequest.Metrics.ProxyProtocolVersion,
-		ClientOfferSDP:              offerRequest.ClientOfferSDP,
+		ClientOfferSDP:              offerSDP,
 		ClientRootObfuscationSecret: offerRequest.ClientRootObfuscationSecret,
 		DoDTLSRandomization:         offerRequest.DoDTLSRandomization,
 		TrafficShapingParameters:    offerRequest.TrafficShapingParameters,
@@ -1000,7 +1011,15 @@ func (b *Broker) handleProxyAnswer(
 		return nil, errors.Trace(err)
 	}
 
-	logFields, err = answerRequest.ValidateAndGetLogFields(
+	// The filtered SDP is the request SDP with any invalid (bogon, unexpected
+	// GeoIP) ICE candidates filtered out. In some cases, proxies cannot
+	// avoid submitting invalid candidates (see comment in
+	// processSDPAddresses), so all invalid candidates are removed and the
+	// remaining SDP is used. Filtered candidate information is logged in
+	// logFields.
+
+	var filteredSDP []byte
+	filteredSDP, logFields, err = answerRequest.ValidateAndGetLogFields(
 		b.config.LookupGeoIP,
 		b.config.APIParameterValidator,
 		b.config.APIParameterLogFieldFormatter,
@@ -1009,6 +1028,9 @@ func (b *Broker) handleProxyAnswer(
 		return nil, errors.Trace(err)
 	}
 
+	answerSDP := answerRequest.ProxyAnswerSDP
+	answerSDP.SDP = string(filteredSDP)
+
 	if answerRequest.AnswerError != "" {
 
 		// The proxy failed to create an answer.
@@ -1029,7 +1051,7 @@ func (b *Broker) handleProxyAnswer(
 			ProxyID:                      initiatorID,
 			ConnectionID:                 answerRequest.ConnectionID,
 			SelectedProxyProtocolVersion: answerRequest.SelectedProxyProtocolVersion,
-			ProxyAnswerSDP:               answerRequest.ProxyAnswerSDP,
+			ProxyAnswerSDP:               answerSDP,
 		}
 
 		err = b.matcher.Answer(proxyAnswer)

psiphon/common/inproxy/inproxy_disabled.go

@@ -119,8 +119,9 @@ func (conn *webRTCConn) GetMetrics() common.LogFields {
 }
 
 type webRTCSDPMetrics struct {
-	iceCandidateTypes []ICECandidateType
-	hasIPv6           bool
+	iceCandidateTypes     []ICECandidateType
+	hasIPv6               bool
+	filteredICECandidates []string
 }
 
 func newWebRTCConnWithOffer(
@@ -139,12 +140,12 @@ func newWebRTCConnWithAnswer(
 	return nil, WebRTCSessionDescription{}, nil, errors.Trace(errNotEnabled)
 }
 
-func validateSDPAddresses(
+func filterSDPAddresses(
 	encodedSDP []byte,
 	errorOnNoCandidates bool,
 	lookupGeoIP LookupGeoIP,
-	expectedGeoIPData common.GeoIPData) (*webRTCSDPMetrics, error) {
-	return nil, errors.Trace(errNotEnabled)
+	expectedGeoIPData common.GeoIPData) ([]byte, *webRTCSDPMetrics, error) {
+	return nil, nil, errors.Trace(errNotEnabled)
 }
 
 func initPortMapper(coordinator WebRTCDialCoordinator) {

psiphon/common/inproxy/inproxy_test.go

@@ -102,6 +102,7 @@ func runTestInproxy() error {
 
 	// Enable test to run without requiring host firewall exceptions
 	SetAllowBogonWebRTCConnections(true)
+	defer SetAllowBogonWebRTCConnections(false)
 
 	// Init logging and profiling
 

psiphon/common/inproxy/sdp_test.go

@@ -0,0 +1,157 @@
+//go:build PSIPHON_ENABLE_INPROXY
+
+/*
+ * Copyright (c) 2024, Psiphon Inc.
+ * All rights reserved.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package inproxy
+
+import (
+	"context"
+	"net"
+	"strings"
+	"testing"
+
+	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
+	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/errors"
+)
+
+func TestProcessSDP(t *testing.T) {
+	err := runTestProcessSDP()
+	if err != nil {
+		t.Errorf(errors.Trace(err).Error())
+	}
+}
+
+func runTestProcessSDP() error {
+
+	config := &webRTCConfig{
+		Logger: newTestLogger(),
+		WebRTCDialCoordinator: &testWebRTCDialCoordinator{
+			disableSTUN:        true,
+			disablePortMapping: true,
+		},
+	}
+
+	// Create a valid, base SDP, including private network (bogon) candidates.
+
+	SetAllowBogonWebRTCConnections(true)
+	defer SetAllowBogonWebRTCConnections(false)
+
+	conn, webRTCSDP, metrics, err := newWebRTCConnWithOffer(
+		context.Background(), config)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	defer conn.Close()
+
+	SDP := []byte(webRTCSDP.SDP)
+
+	// Test disallow IPv6
+
+	if metrics.hasIPv6 {
+		preparedSDP, metrics, err := prepareSDPAddresses(
+			SDP, true, "", true)
+		if err != nil {
+			return errors.Trace(err)
+		}
+
+		found := false
+		for _, reason := range metrics.filteredICECandidates {
+			if strings.Contains(reason, "disabled IPv6") {
+				found = true
+				break
+			}
+		}
+		if !found {
+			return errors.TraceNew("unexpected filteredICECandidates")
+		}
+
+		if len(preparedSDP) >= len(SDP) {
+			return errors.TraceNew("unexpected SDP length")
+		}
+	}
+
+	// Test filter unexpected GeoIP
+
+	// This IP must not be a bogon; this address is not dialed.
+	testIP := "1.1.1.1"
+	expectedGeoIP := common.GeoIPData{Country: "AA", ASN: "1"}
+	lookupGeoIP := func(IP string) common.GeoIPData {
+		if IP == testIP {
+			return common.GeoIPData{Country: "BB", ASN: "2"}
+		}
+		return expectedGeoIP
+	}
+
+	// Add the testIP as a port mapping candidate.
+	preparedSDP, metrics, err := prepareSDPAddresses(
+		SDP, true, net.JoinHostPort(testIP, "80"), false)
+	if err != nil {
+		return errors.Trace(err)
+	}
+
+	filteredSDP, metrics, err := filterSDPAddresses(
+		preparedSDP, true, lookupGeoIP, expectedGeoIP)
+	if err != nil {
+		return errors.Trace(err)
+	}
+
+	found := false
+	for _, reason := range metrics.filteredICECandidates {
+		if strings.Contains(reason, "unexpected GeoIP") {
+			found = true
+			break
+		}
+	}
+	if !found {
+		return errors.TraceNew("unexpected filteredICECandidates")
+	}
+
+	if len(filteredSDP) >= len(preparedSDP) {
+		return errors.TraceNew("unexpected SDP length")
+	}
+
+	// Test filter bogons
+
+	SetAllowBogonWebRTCConnections(false)
+
+	// Allow no candidates (errorOnNoCandidates = false)
+	filteredSDP, metrics, err = filterSDPAddresses(
+		SDP, false, nil, common.GeoIPData{})
+	if err != nil {
+		return errors.Trace(err)
+	}
+
+	found = false
+	for _, reason := range metrics.filteredICECandidates {
+		if strings.Contains(reason, "bogon") {
+			found = true
+			break
+		}
+	}
+	if !found {
+		return errors.TraceNew("unexpected filteredICECandidates")
+	}
+
+	if len(filteredSDP) >= len(preparedSDP) {
+		return errors.TraceNew("unexpected SDP length")
+	}
+
+	return nil
+}

psiphon/common/inproxy/webrtc.go

@@ -146,12 +146,6 @@ type webRTCConfig struct {
 	ReliableTransport bool
 }
 
-// webRTCSDPMetrics are network capability metrics values for an SDP.
-type webRTCSDPMetrics struct {
-	iceCandidateTypes []ICECandidateType
-	hasIPv6           bool
-}
-
 // newWebRTCConnWithOffer initiates a new WebRTC connection. An offer SDP is
 // returned, to be sent to the peer. After the offer SDP is forwarded and an
 // answer SDP received in response, call SetRemoteSDP with the answer SDP and
@@ -1528,31 +1522,37 @@ func prepareSDPAddresses(
 		encodedSDP,
 		portMappingExternalAddr,
 		disableIPv6Candidates,
-		false, // bogons are expected, and stripped out
 		errorOnNoCandidates,
 		nil,
 		common.GeoIPData{})
 	return modifiedSDP, metrics, errors.Trace(err)
 }
 
-// validateSDPAddresses checks that the SDP does not contain an empty list of
+// filterSDPAddresses checks that the SDP does not contain an empty list of
 // candidates, bogon candidates, or candidates outside of the country and ASN
-// for the specified expectedGeoIPData.
-func validateSDPAddresses(
+// for the specified expectedGeoIPData. Invalid candidates are stripped and a
+// filtered SDP is returned.
+func filterSDPAddresses(
 	encodedSDP []byte,
 	errorOnNoCandidates bool,
 	lookupGeoIP LookupGeoIP,
-	expectedGeoIPData common.GeoIPData) (*webRTCSDPMetrics, error) {
+	expectedGeoIPData common.GeoIPData) ([]byte, *webRTCSDPMetrics, error) {
 
-	_, metrics, err := processSDPAddresses(
+	filteredSDP, metrics, err := processSDPAddresses(
 		encodedSDP,
 		"",
 		false,
-		true, // bogons should already by stripped out
 		errorOnNoCandidates,
 		lookupGeoIP,
 		expectedGeoIPData)
-	return metrics, errors.Trace(err)
+	return filteredSDP, metrics, errors.Trace(err)
+}
+
+// webRTCSDPMetrics are network capability metrics values for an SDP.
+type webRTCSDPMetrics struct {
+	iceCandidateTypes     []ICECandidateType
+	hasIPv6               bool
+	filteredICECandidates []string
 }
 
 // processSDPAddresses is based on snowflake/common/util.StripLocalAddresses
@@ -1592,12 +1592,10 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 ================================================================================
 
 */
-
 func processSDPAddresses(
 	encodedSDP []byte,
 	portMappingExternalAddr string,
 	disableIPv6Candidates bool,
-	errorOnBogon bool,
 	errorOnNoCandidates bool,
 	lookupGeoIP LookupGeoIP,
 	expectedGeoIPData common.GeoIPData) ([]byte, *webRTCSDPMetrics, error) {
@@ -1610,11 +1608,13 @@ func processSDPAddresses(
 
 	candidateTypes := map[ICECandidateType]bool{}
 	hasIPv6 := false
+	filteredCandidateReasons := make(map[string]int)
 
 	var portMappingICECandidates []sdp.Attribute
 	if portMappingExternalAddr != "" {
 
-		// Prepare ICE candidate attibute pair for the port mapping, modeled after the definition of host candidates.
+		// Prepare ICE candidate attibute pair for the port mapping, modeled
+		// after the definition of host candidates.
 
 		host, portStr, err := net.SplitHostPort(portMappingExternalAddr)
 		if err != nil {
@@ -1631,7 +1631,7 @@ func processSDPAddresses(
 
 			for _, component := range []webrtc.ICEComponent{webrtc.ICEComponentRTP, webrtc.ICEComponentRTCP} {
 
-				// The candidate ID is generated and the priorty and foundation
+				// The candidate ID is generated and the priority and foundation
 				// use the default for hosts.
 				//
 				// Limitation: NewCandidateHost initializes the networkType to
@@ -1692,25 +1692,29 @@ func processSDPAddresses(
 				candidateIsIPv6 := false
 				if candidateIP.To4() == nil {
 					if disableIPv6Candidates {
+						reason := "disabled IPv6"
+						filteredCandidateReasons[reason] += 1
 						continue
 					}
 					candidateIsIPv6 = true
-					hasIPv6 = true
 				}
 
 				// Strip non-routable bogons, including LAN addresses.
 				// Same-LAN client/proxy hops are not expected to be useful,
 				// and this also avoids unnecessary local network traffic.
 				//
-				// Well-behaved clients and proxies will strip these values;
-				// the broker enforces this and uses errorOnBogon.
+				// Well-behaved clients and proxies should strip these values;
+				// the broker enforces this with filtering.
 
 				if !GetAllowBogonWebRTCConnections() &&
 					common.IsBogon(candidateIP) {
 
-					if errorOnBogon {
-						return nil, nil, errors.TraceNew("unexpected bogon")
+					version := "IPv4"
+					if candidateIsIPv6 {
+						version = "IPv6"
 					}
+					reason := fmt.Sprintf("bogon %s", version)
+					filteredCandidateReasons[reason] += 1
 					continue
 				}
 
@@ -1721,6 +1725,16 @@ func processSDPAddresses(
 				// Legitimate candidates will not all have the exact same IP
 				// address, as there could be a mix of IPv4 and IPv6, as well
 				// as potentially different NAT paths.
+				//
+				// In some cases, legitimate clients and proxies may
+				// unintentionally submit candidates with mismatching GeoIP.
+				// This can occur, for example, when a STUN candidate is only
+				// a partial hole punch through double NAT, and when internal
+				// network addresses misuse non-private IP ranges (so are
+				// technically not bogons). Instead of outright rejecting
+				// SDPs containing unexpected GeoIP candidates, they are
+				// instead stripped out and the the resulting filtered SDP is
+				// used.
 
 				if lookupGeoIP != nil {
 					candidateGeoIPData := lookupGeoIP(candidate.Address())
@@ -1732,15 +1746,20 @@ func processSDPAddresses(
 						if candidateIsIPv6 {
 							version = "IPv6"
 						}
-						errStr := fmt.Sprintf(
-							"unexpected GeoIP for %s candidate: %s, %s",
+						reason := fmt.Sprintf(
+							"unexpected GeoIP for %s candidate: %s/%s",
 							version,
 							candidateGeoIPData.Country,
 							candidateGeoIPData.ASN)
-						return nil, nil, errors.TraceNew(errStr)
+						filteredCandidateReasons[reason] += 1
+						continue
 					}
 				}
 
+				if candidateIsIPv6 {
+					hasIPv6 = true
+				}
+
 				// These types are not reported:
 				// - CandidateTypeRelay: TURN servers are not used.
 				// - CandidateTypePeerReflexive: this candidate type only
@@ -1777,6 +1796,10 @@ func processSDPAddresses(
 	for candidateType := range candidateTypes {
 		metrics.iceCandidateTypes = append(metrics.iceCandidateTypes, candidateType)
 	}
+	for reason, count := range filteredCandidateReasons {
+		metrics.filteredICECandidates = append(metrics.filteredICECandidates,
+			fmt.Sprintf("%s: %d", reason, count))
+	}
 
 	return encodedSDP, metrics, nil
 }