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

RESOLVED FIXED in Firefox 58

Status

()

defect
P2
normal
Rank:
29
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: deadbeef, Assigned: drno)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

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).
Assignee: nobody → drno
Status: UNCONFIRMED → NEW
Rank: 29
Ever confirmed: true
Priority: -- → P2
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?
Attachment #8919565 - Flags: review?(docfaraday) → review-
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.
Flags: needinfo?(docfaraday)
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.
Attachment #8919565 - Flags: review?(docfaraday) → review-
Comment on attachment 8919565 [details]
Bug 1408218: ignore EUI 64 and Teredo addresses if not needed.

https://reviewboard.mozilla.org/r/190400/#review196836
Attachment #8919565 - Flags: review?(docfaraday) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/103d6b6e79b8
ignore EUI 64 and Teredo addresses if not needed. r=bwc
https://hg.mozilla.org/mozilla-central/rev/103d6b6e79b8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(docfaraday)
Depends on: 1415249
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.
(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?
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
Blocks: 1473840
No longer blocks: 1473840
Depends on: 1473840
You need to log in before you can comment on or make changes to this bug.