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

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add a whitelist for network interfaces to use with ICE/webrtc

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.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add a whitelist for network interfaces to use with ICE/webrtc

Just added gonk_addrs.c support.  ekr, if you're too busy, let's move this to Byron or Nils

Comment 4

[Eric Rescorla (: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?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(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 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add a whitelist for network interfaces to use with ICE/webrtc

Comment 7

[Eric Rescorla (:ekr)]

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

Comment 8

[Maire Reavy [:mreavy]]

[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)

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add a whitelist for network interfaces to use with ICE/webrtc

unbitrotted against latest patch in bug 1189041

Comment 10

[Eric Rescorla (:ekr)]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9141e5b245f6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9141e5b245f6

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as this work is planned to ship in FF41.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8618de5f122f

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add a whitelist for network interfaces to use with ICE/webrtc

Beta patch, tested

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/955e74958483