Add an about:config pref with a whitelist for interfaces to use for ICE (in WebRTC)

RESOLVED FIXED in Firefox 41

Status

()

defect
P1
normal
Rank:
8
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ fixed, firefox42 fixed, firefox43 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

By adding a whitelist for interfaces used for ICE, a user/extension can lock down use of WebRTC (and addresses exposed by ICE) to a single interface, such as a VPN interface.  This minimizes the chance that your real external IP (or LAN) address is exposed to site content..  It's more restrictive than a limitation to default-route, and likely would totally block communication if the interface wasn't up, so it's not something most users should be messing with.  However for a subset of users it  may be an important piece of the puzzle.

In most cases, an extension would help you to set and maintain this (and maybe turn it off at times, or for certain configured sites).
backlog: --- → webRTC+
Rank: 8
Priority: -- → P1
Blocks: 1189167
Note that it simplified things a bunch to move the set_string() stuff to const char * from char *.  Tested on linux and works.  If you set this, and there's no match, you get no interfaces - on purpose.
Attachment #8642615 - Flags: review?(ekr)
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Note: as you'd expect, unlike the default_route changes, this only limits you to a single interface, but does full ICE on that (i.e. HOST candidates, local LAN IPs)
Just added gonk_addrs.c support.  ekr, if you're too busy, let's move this to Byron or Nils
Attachment #8643205 - Flags: review?(ekr)
Attachment #8642615 - Attachment is obsolete: true
Attachment #8642615 - Flags: review?(ekr)
Comment on attachment 8643205 [details] [diff] [review]
add a whitelist for network interfaces to use with ICE/webrtc

Review of attachment 8643205 [details] [diff] [review]:
-----------------------------------------------------------------

1. I know that it would be nice to constify the signature, but this is going to
break parts of nrappkit that we don't use in Firefox, so please don;t.

2. Rather than do this in the address gathering code, I would suggest doing this
in the ice code (Where I did my patch) with the result that you then don't need
separate code for gonk and non-gonk.

::: media/mtransport/nricectx.cpp
@@ +491,5 @@
>      }
> +    if (interface_whitelist.Length() > 0) {
> +      // Stupid cast.... but needed
> +      const nsCString &flat = PromiseFlatCString(static_cast<nsACString&>(interface_whitelist));
> +      NR_reg_set_string((char *)NR_STUN_REG_PREF_FORCE_INTERFACE_NAME, flat.get());

There seems to be some inconsistency about the naming here, since this says "whitelist" but the reg key looks like it's "force interface". What are the semantics?
Attachment #8643205 - Flags: review?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #4)
> Comment on attachment 8643205 [details] [diff] [review]
> add a whitelist for network interfaces to use with ICE/webrtc
> 
> Review of attachment 8643205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1. I know that it would be nice to constify the signature, but this is going
> to
> break parts of nrappkit that we don't use in Firefox, so please don;t.

Ok.  Minor pain, but can be done.

> 2. Rather than do this in the address gathering code, I would suggest doing
> this
> in the ice code (Where I did my patch) with the result that you then don't
> need
> separate code for gonk and non-gonk.

Ok, though I think it's pretty much a wash.

> ::: media/mtransport/nricectx.cpp
> @@ +491,5 @@
> >      }
> > +    if (interface_whitelist.Length() > 0) {
> > +      // Stupid cast.... but needed
> > +      const nsCString &flat = PromiseFlatCString(static_cast<nsACString&>(interface_whitelist));
> > +      NR_reg_set_string((char *)NR_STUN_REG_PREF_FORCE_INTERFACE_NAME, flat.get());
> 
> There seems to be some inconsistency about the naming here, since this says
> "whitelist" but the reg key looks like it's "force interface". What are the
> semantics?

It's a single interface that we must match (if it's set).  As mentioned, this forces it to fail if the interface isn't available (on purpose, for example if you want to restrict it to a VPN interface - if the VPN is down, it will simply fail instead of silently exposing you).

I can make the naming match.
Comment on attachment 8643330 [details] [diff] [review]
add a whitelist for network interfaces to use with ICE/webrtc

Review of attachment 8643330 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with changes below

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +646,5 @@
>        }
>  
> +      if (ctx->force_interface[0]) {
> +        // Limit us to only addresses on a single interface
> +        int force_addr_ct = 0;

C-style comments here please.

@@ +658,5 @@
> +            force_addr_ct++;
> +          }
> +        }
> +        addr_ct = force_addr_ct;
> +      }

I think I'd be happier if you just had a second array. We're going to want that if/when we land my patch and the logic is easier.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h
@@ +150,5 @@
>  
>    nr_ice_trickle_candidate_cb trickle_cb;
>    void *trickle_cb_arg;
> +
> +  char force_interface[MAXIFNAME];

Can we make this force_net_interface for clarity
Attachment #8643330 - Flags: review?(ekr) → review+
[Tracking Requested - why for this release]:
Plan is to uplift this into 41 to support an extension that can mitigate these issues ASAP (see mail to drivers)
unbitrotted against latest patch in bug 1189041
Attachment #8646988 - Flags: review?(ekr)
Attachment #8643330 - Attachment is obsolete: true
Comment on attachment 8646988 [details] [diff] [review]
add a whitelist for network interfaces to use with ICE/webrtc

Review of attachment 8646988 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with changes.

::: media/mtransport/nricectx.cpp
@@ +483,5 @@
>        NR_reg_set_char((char *)NR_STUN_REG_PREF_ALLOW_LINK_LOCAL_ADDRS, 1);
>      }
> +    if (force_net_interface.Length() > 0) {
> +      // Stupid cast.... but needed
> +      const nsCString &flat = PromiseFlatCString(static_cast<nsACString&>(force_net_interface));

attach & to nsCString pls.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +315,5 @@
>      return(_status);
>    }
>  #endif /* USE_TURN */
>  
> +#define MAXADDRS 100 /* Ridiculously high */

Thanks for fixing this.

@@ +399,5 @@
>  
>      if (r=nr_socket_factory_create_int(NULL, &default_socket_factory_vtbl, &ctx->socket_factory))
>        ABORT(r);
>  
> +    if (r=NR_reg_get_string((char *)NR_ICE_REG_PREF_FORCE_INTERFACE_NAME, ctx->force_net_interface, sizeof(ctx->force_net_interface))) {

We are doing if ((r= ...)) in new code.

@@ +402,5 @@
>  
> +    if (r=NR_reg_get_string((char *)NR_ICE_REG_PREF_FORCE_INTERFACE_NAME, ctx->force_net_interface, sizeof(ctx->force_net_interface))) {
> +      if (r == R_NOT_FOUND) {
> +        ctx->force_net_interface[0] = 0;
> +      } else {

Suggest we instead mark this as 0 in the ctor.

@@ +825,5 @@
>      needed=len/2;
>  
>      if(needed>sizeof(bytes)) ABORT(R_BAD_ARGS);
>  
> +    /* memset(bytes,0,needed); */

Let's just remove this code.
Attachment #8646988 - Flags: review?(ekr) → review+
https://hg.mozilla.org/mozilla-central/rev/9141e5b245f6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Tracked as this work is planned to ship in FF41.
Attachment #8643205 - Attachment is obsolete: true
Comment on attachment 8646988 [details] [diff] [review]
add a whitelist for network interfaces to use with ICE/webrtc

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Lack of ability to limit interfaces used for WebRTC (and thus to hide your non-VPN address when using a VPN).  Unlike bug 1189041, this allows use of (and exposure of) LAN IP addresses, and also will block all WebRTC use if the interface is not available/up (which can provide extra security if set to the VPN interface).

[Describe test coverage new/current, TreeHerder]: Manual testing.

[Risks and why]: Very low risk; has no impact when the pref isn't set.  Changes very isolated.  (Will need to be (easily) rebased for Beta since we're not asking for bug 1189041, which this landed on top of. -- Will upload rebased patch.)

[String/UUID change made/needed]: none
Attachment #8646988 - Flags: approval-mozilla-beta?
Attachment #8646988 - Flags: approval-mozilla-aurora?
Comment on attachment 8646988 [details] [diff] [review]
add a whitelist for network interfaces to use with ICE/webrtc

Approved for uplift to Aurora, this will give us some time to stabilize the patch and then we can uplift to Beta.
Attachment #8646988 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8650743 - Flags: approval-mozilla-beta?
Attachment #8646988 - Flags: approval-mozilla-beta?
Comment on attachment 8650743 [details] [diff] [review]
add a whitelist for network interfaces to use with ICE/webrtc

Since this change is isolated and does not impact default behavior (pref'd off), it should be safe to uplift to Beta. Dev testing on this one was good I've been told. Let's uplift to Beta.
Attachment #8650743 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.