Remove local LAN address ICE candidates

added component::circumvention/snowflake owner::arlolra priority::medium resolution::fixed reviewer::cohosh severity::normal status::closed type::enhancement labels

The WebRTC working draft touches on this issue: https://www.w3.org/TR/2016/WD-webrtc-20160128/#revealing-ip-addresses Even without WebRTC, the Web server providing a Web application will know the public IP address to which the application is delivered. Setting up communications exposes additional information about the browser’s network context to the web application, and may include the set of (possibly private) IP addresses available to the browser for WebRTC use. Some of this information has to be passed to the corresponding party to enable the establishment of a communication session.

Revealing IP addresses can leak location and means of connection; this can be sensitive. Depending on the network environment, it can also increase the fingerprinting surface and create persistent cross-origin state that cannot easily be cleared by the user.

A connection will always reveal the IP addresses proposed for communication to the corresponding party. The application can limit this exposure by choosing not to use certain addresses using the settings exposed by the RTCIceTransportPolicy dictionary, and by using relays (for instance TURN servers) rather than direct connections between participants. One will normally assume that the IP address of TURN servers is not sensitive information. These choices can for instance be made by the application based on whether the user has indicated consent to start a media connection with the other party.

Mitigating the exposure of IP addresses to the application itself requires limiting the IP addresses that can be used, which will impact the ability to communicate on the most direct path between endpoints. Browsers are encouraged to provide appropriate controls for deciding which IP addresses are made available to applications, based on the security posture desired by the user. The choice of which addresses to expose is controlled by local policy (see RTCWEB-IP-HANDLING for details).

The latter link is all about handling IP addresses with respect to privacy: https://datatracker.ietf.org/doc/draft-ietf-rtcweb-ip-handling/?include_text=1

Trac:
Status: new to assigned
Owner: N/A to arlolra

Here's an attempt at this, https://github.com/keroserene/snowflake/commit/71934f1db34bec354a94266c79f2e631be604a03

Trac:
Status: assigned to needs_review

Trac:
Cc: N/A to cohosh, phw

Trac:
Reviewer: N/A to cohosh

Thanks for working on this! Thanks for keeping the option to leave the local addresses in as well.

That's unfortunate that the "public" RTCIceTransportPolicy was removed from the specification. It would be nicer if we could prevent the candidate from being included in the SDP instead of grepping for it afterwards, but I don't see a way to do that with the OnICECandidate callback.

Some comments:

FIXME: Should this check ip.IsLoopback() and others? I'd like to include 0.0.0.0 and 127.0.0.1 addresses in this, especially after dcf found #33157 (moved).
Let's expand the tests and include one for each type of local address
It's worth implementing this for each of the proxies as well.
Is there a way for us to use the other built-in functions in the net package for determining whether or not the IP address is local?

Trac:
Status: needs_review to needs_revision

Ok, new commit at https://github.com/keroserene/snowflake/commit/dbd133b6e1196e4ec7550f0ebb52854cb00d1007

Some comments:

FIXME: Should this check ip.IsLoopback() and others? I'd like to include 0.0.0.0 and 127.0.0.1 addresses in this, especially after dcf found #33157 (moved).

The condition now looks like IsLocal(ip) || ip.IsUnspecified() || ip.IsLoopback(), which includes these addresses.

Let's expand the tests and include one for each type of local address

Done

It's worth implementing this for each of the proxies as well.

Can this be a follow up?

Is there a way for us to use the other built-in functions in the net package for determining whether or not the IP address is local?

Not that I'm aware of. The included function was referenced from, https://github.com/golang/go/issues/29146

Trac:
Status: needs_revision to needs_review

especially after dcf found #33157 (moved).

Hmm, after reading that, I noticed we have, https://github.com/keroserene/snowflake/blob/master/proxy-go/snowflake.go#L60-L70

I should probably consolidate that with some of the nastiness added here to grep the sdp.

These changes looks good to me. Let's merge this and follow up with a port to proxies as you mentioned. Thanks!

Trac:
Status: needs_review to merge_ready

Replying to arlolra:

especially after dcf found #33157 (moved).

Hmm, after reading that, I noticed we have, https://github.com/keroserene/snowflake/blob/master/proxy-go/snowflake.go#L60-L70

I should probably consolidate that with some of the nastiness added here to grep the sdp.

That's a good idea. We can discuss this on #33157 (moved).

Sorry to bring it up at this late stage, but the pion/sdp package is an alternative to string operations. There's a SessionDescription type with Marshal and Unmarshal methods, containing an array of Attributes that each have a ToICECandidate method.

Replying to cohosh:

FIXME: Should this check ip.IsLoopback() and others? I'd like to include 0.0.0.0 and 127.0.0.1 addresses in this, especially after dcf found #33157 (moved).

I feel that the 0.0.0.0 is unrelated to anything involving candidate addresses. 0.0.0.0 is appearing in the o= origin field and the c= connection information field (the latter is what remoteIPFromSDP looks at), not in an a=candidate attribute.

However, I imagine that if a LAN address appears as a a=candidate, that may also cause it to appear in o= or c=, and we may have to additionally deal with that when #33157 (moved) is figured out.

Sorry to bring it up at this late stage, but the pion/sdp package is an alternative to string operations.

The patch already uses that parse candidate lines, https://github.com/keroserene/snowflake/commit/dbd133b6e1196e4ec7550f0ebb52854cb00d1007#diff-0f3a063993ea3b440ad2ce0abb6ac195R101

However, I missed that Attributes are public on the SessionDescription, and can be manipulated directly.

Will fix, thanks.

However, I missed that Attributes are public on the SessionDescription, and can be manipulated directly.

I think I must have confused sdp.SessionDescription with webrtc.SessionDescription

Thanks for catching that, dcf

Will fix, thanks.

Here's a branch for this now, https://github.com/keroserene/snowflake/compare/trac19026

Trac:
Status: merge_ready to needs_review

Replying to arlolra:

Here's a branch for this now, https://github.com/keroserene/snowflake/compare/trac19026

https://github.com/keroserene/snowflake/compare/trac19026#diff-0f3a063993ea3b440ad2ce0abb6ac195R144

Could

	if !bc.keepLocalAddresses {
		offer = &webrtc.SessionDescription{
			Type: offer.Type,
			SDP:  stripLocalAddresses(offer.SDP),
		}
	}

be instead

	if !bc.keepLocalAddresses {
		offer.SDP = stripLocalAddresses(offer.SDP)
	}

?

https://github.com/keroserene/snowflake/compare/trac19026#diff-0f3a063993ea3b440ad2ce0abb6ac195R105

			if a.IsICECandidate() {

Is it necessary to test IsICECandidate, or could you skip it and just check the err result of ToICECandidate?

The attributes loop is structured like this, with attrs = append(attrs, a) in three places:

for a in attributes {
	if a.IsICECandidate() {
		ice, err = a.ToICECandidate()
		if err != nil {
			attrs = append(attrs, a)
			continue
		}
		if ice.Typ == "host" {
			ip = net.ParseIP(ice.Address)
			if ip == nil {
				attrs = append(attrs, a)
				continue
			}
			if IsLocal(ip) {
				/* no append in this case */
				continue
			}
		}
	}
	attrs = append(attrs, a)
}

Consider restructuring so that you only continue in the "skip" case, and all other cases fall through to attrs = append(attrs, a). Expressing the logic: if a candidate, and type=="host", and an IP address, and IP address is local, skip; otherwise keep.

for a in attributes {
	if a.IsICECandidate() {
		ice, err = a.ToICECandidate()
		if err == nil && ice.Typ == "host" {
			ip = net.ParseIP(ice.Address)
			if ip != nil && IsLocal(ip) {
				/* no append in this case */
				continue
			}
		}
	}
	attrs = append(attrs, a)
}

But also possibly with my note about ToICECandidate above:

for a in attributes {
	if ice, err = a.ToICECandidate(); err == nil {
		if ice.Typ == "host" {
			ip = net.ParseIP(ice.Address)
			if ip != nil && IsLocal(ip) {
				/* no append in this case */
				continue
			}
		}
	}
	attrs = append(attrs, a)
}

Replying to dcf:

Could {{{ if !bc.keepLocalAddresses { offer = &webrtc.SessionDescription{ Type: offer.Type, SDP: stripLocalAddresses(offer.SDP), } } }}} be instead {{{ if !bc.keepLocalAddresses { offer.SDP = stripLocalAddresses(offer.SDP) } }}} ?

It could, but since offer *webrtc.SessionDescription comes from a call to pc.LocalDescription(), I didn't want to invalidate the cached parsed description in that structure, https://github.com/pion/webrtc/blob/master/sessiondescription.go#L10-L13

https://github.com/keroserene/snowflake/compare/trac19026#diff-0f3a063993ea3b440ad2ce0abb6ac195R105

{{{ if a.IsICECandidate() { }}}

Is it necessary to test IsICECandidate, or could you skip it and just check the err result of ToICECandidate?

You could skip it, yes, but I felt the cheap string check was preferable attempting a parse, https://github.com/pion/sdp/blob/03441e3c706c7c3b719ee75194049a31cbb2eb7e/common_description.go#L112-L122

The attributes loop is structured like this, with attrs = append(attrs, a) in three places: {{{ for a in attributes { if a.IsICECandidate() { ice, err = a.ToICECandidate() if err != nil { attrs = append(attrs, a) continue } if ice.Typ == "host" { ip = net.ParseIP(ice.Address) if ip == nil { attrs = append(attrs, a) continue } if IsLocal(ip) { /* no append in this case */ continue } } } attrs = append(attrs, a) } }}}

Consider restructuring so that you only continue in the "skip" case, and all other cases fall through to attrs = append(attrs, a). Expressing the logic: if a candidate, and type=="host", and an IP address, and IP address is local, skip; otherwise keep. {{{ for a in attributes { if a.IsICECandidate() { ice, err = a.ToICECandidate() if err == nil && ice.Typ == "host" { ip = net.ParseIP(ice.Address) if ip != nil && IsLocal(ip) { /* no append in this case */ continue } } } attrs = append(attrs, a) } }}}

But also possibly with my note about ToICECandidate above: {{{ for a in attributes { if ice, err = a.ToICECandidate(); err == nil { if ice.Typ == "host" { ip = net.ParseIP(ice.Address) if ip != nil && IsLocal(ip) { /* no append in this case */ continue } } } attrs = append(attrs, a) } }}}

Yeah, that was ugly. I pushed commit for this suggestion, https://github.com/keroserene/snowflake/commit/edd53af92ac868cf3ba57988e14de887f088a47b

Replying to arlolra:

It could, but since offer *webrtc.SessionDescription comes from a call to pc.LocalDescription(), I didn't want to invalidate the cached parsed description in that structure, https://github.com/pion/webrtc/blob/master/sessiondescription.go#L10-L13

You could skip it, yes, but I felt the cheap string check was preferable attempting a parse, https://github.com/pion/sdp/blob/03441e3c706c7c3b719ee75194049a31cbb2eb7e/common_description.go#L112-L122

Okay, works for me.

Yeah, that was ugly. I pushed commit for this suggestion, https://github.com/keroserene/snowflake/commit/edd53af92ac868cf3ba57988e14de887f088a47b

Everything looks good from my point of view.

Ok, merged starting at, https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=0fae4ee8ea487c3b4384217e193e5b9a9088e7de

What's left to do is a follow up for the proxies, as suggested in comment:10

Trac:
Status: needs_review to assigned

What's left to do is a follow up for the proxies, as suggested in comment:10

Here's a branch for proxy-go, https://github.com/keroserene/snowflake/commits/lan

Is it worth the effort for the JS proxies?

Trac:
Status: assigned to needs_review

LGTM. I guess for browser-based proxies that depends on how much effort it is. I'm okay with making it lower priority, but if that's the case let's either leave this ticket open or make a new ticket to track it.

Trac:
Status: needs_review to merge_ready

Merged as, https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=670e4ba4380b3fa5cf82043559dcb8c2ca790a7d https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=1867f89562fb25bf9a3c2172a7b6f0a198c81adb

I guess for browser-based proxies that depends on how much effort it is. I'm okay with making it lower priority, but if that's the case let's either leave this ticket open or make a new ticket to track it.

Ok, it's probably not too great an effort, just not really a priority. I filed #33744 (moved) for that.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed