1408218 - ICE shouldn't gather EUI 64 (MAC-based) IPv6 candidates

Status: RESOLVED FIXED

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

Description

[Taylor Brandstetter]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

Steps to reproduce:

Create a PeerConnection, set a local description.


Actual results:

EUI-64 candidates gathered.


Expected results:

Shouldn't have been gathered, according to https://tools.ietf.org/html/draft-ietf-rtcweb-transports-17#section-3.3. This is a significant fingerprinting surface; it potentially gives the application a persistent, globally-unique identifier for the user.

I haven't tested this myself, but this was brought up in a chromium bug (https://bugs.chromium.org/p/chromium/issues/detail?id=762506).

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

Review commit: https://reviewboard.mozilla.org/r/190400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190400/

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8919565 [details]
Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

https://reviewboard.mozilla.org/r/190400/#review195936

I think we probably want to loop through looking for mac-based vs "real" IPV6 before the filtering loop, not after.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:447
(Diff revision 1)
> +      case NR_IPV4:
> +        // IPv4 has no MAC based self assigned IP addresses
> +        return(0);
> +      case NR_IPV6:
> +        {
> +          // RFC 2373, Appendix A: lower 64bit == 0x020000FFFE000000

The '==' is misleading here.

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:395
(Diff revision 1)
>          else if (remove_link_local &&
>                   addrs[i].addr.ip_version == NR_IPV6 &&
>                   nr_transport_addr_is_link_local(&addrs[i].addr)) {
>              /* skip addrs[i], it's a link-local address */
>          }
> +        else if (addrs[i].addr.ip_version == NR_IPV6 &&

Do we really need the extra IPV6 check here?

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:397
(Diff revision 1)
> +            contains_mac_based_ipv6 += 1;
> +        }
>          else {
> +            if (addrs[i].addr.ip_version == NR_IPV6) {
> +                contains_regular_ipv6 += 1;

Maybe just use these as a boolean instead of incrementing?

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:415
(Diff revision 1)
> +    if (!contains_regular_ipv6 &&
> +        contains_mac_based_ipv6) {
> +        /* copy over mac based EUI-64 candidates as we don't have anything
> +         * better */
> +        for (i = 0; i < *count; ++i) {
> +            if (addrs[i].addr.ip_version == NR_IPV6 &&

Same question here.

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:417
(Diff revision 1)
> +        /* copy over mac based EUI-64 candidates as we don't have anything
> +         * better */
> +        for (i = 0; i < *count; ++i) {
> +            if (addrs[i].addr.ip_version == NR_IPV6 &&
> +                nr_transport_addr_is_mac_based(&addrs[i].addr)) {
> +                if ((r=nr_local_addr_copy(&tmp[n], &addrs[i])))

What about duplicates? Can the other stuff we filter out above crop up here too?

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8919565 [details]
Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190400/diff/1-2/

Comment 4

[Nils Ohlmeier [:drno]]

So I added a second loop over the addresses, which I had tried to avoid in my first patch (for performance reason).

We could also move that functionality into it's own function. Something which puts attribute on IP addresses based on the criteria functions. But then we would have to store that information somewhere in the local ip struct. And even with that information being present in the struct we would still need to loops, one to figure out what is available, and the second for trimming it down to what we want.

Let me know if you prefer that.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8919565 [details]
Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

https://reviewboard.mozilla.org/r/190400/#review196804

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:387
(Diff revision 2)
>  
> +    for (i = 0; i < *count; ++i) {
> +        if (nr_transport_addr_is_mac_based(&addrs[i].addr)) {
> +            contains_mac_based_ipv6 = 1;
> +        }
> +        if (nr_transport_addr_is_teredo(&addrs[i].addr)) {

else if? Otherwise I think we set contains_regular_ipv6 for mac-based addys.

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8919565 [details]
Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190400/diff/2-3/

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8919565 [details]
Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

https://reviewboard.mozilla.org/r/190400/#review196836

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/103d6b6e79b8
ignore EUI 64 and Teredo addresses if not needed. r=bwc

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/103d6b6e79b8

Comment 10

[alex]

I'm experiencing an unexpected behavior since this patch (I guess). Firefox is not gathering my IPv6 candidates even though they're neither MAC-based nor Teredo ones.

I tested with Firefox-57 and IPv6 candidates are gathered, starting from Firefox-58 they're not.

Comment 11

[Byron Campen [:bwc]]

(In reply to alex from comment #10)
> I'm experiencing an unexpected behavior since this patch (I guess). Firefox
> is not gathering my IPv6 candidates even though they're neither MAC-based
> nor Teredo ones.
> 
> I tested with Firefox-57 and IPv6 candidates are gathered, starting from
> Firefox-58 they're not.

What is the problematic IPV6 address here?

Comment 12

[alex]

I tested with these, but I guess the issue is with all IPv6 addresses starting with "2001:":

2001:b07:a14:5888:78af:9c75:7838:cd15
2001:67c:370:230:540f:4bd8:4f70:37d
2001:67c:370:230:46e0:a398:c84d:df1

I also filed a new bug with more information on how to reproduce the issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1473840