Closed Bug 1183145 Opened 10 years ago Closed 5 years ago

We should not use deprecated IPV6 addresses while gathering candidates

Categories

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

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla79
Tracking Flags:
Tracking Status
firefox42 --- wontfix
firefox79 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: bwc, Assigned: bwc, NeedInfo)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1183145: Move platform-specific code from addrs.c into separate files.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1183145: Teach platform-specific code to filter out inappropriate IPv6 addresses, and mark temp addresses.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1183145: Rename a function to better reflect what it does, and fix a bug where handling of teredo and mac-based IPv6 was mixed up.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1183145: Add a flags field to nr_local_addr so IPv6 addresses can be marked as temporary, and filter non-temp IPv6 if temp IPv6 are available.
47 bytes, text/x-phabricator-request
Details | Review
It looks like we can use ioctl with SIOCGIFAFLAG_IN6 to get the flags for an address, and test for the IN6_IFF_DEPRECATED flag. This will help in certain pathological situations where deprecated temporary IPV6 addresses are not being cleaned up properly, resulting in the use of an excessive number of IPV6 addresses while gathering.
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
there are a lot of examples indeed out there about how to do that: http://lists.freebsd.org/pipermail/freebsd-net/2010-July/025820.html Now the question is more (for me) where is the code we should patch? It looks like most of the candidate gathering is delegated to lib nicer in media/mtransport/third_party/nICEr/src ? grep -R -l AF_INET6 * ice/ice_candidate.c net/transport_addr.c net/transport_addr_reg.c stun/addrs.c stun/ifaddrs-android.c
Flags: needinfo?(docfaraday)
The interface gathering code lives in media/mtransport/third_party/nICEr/src/stun/addrs.c, and this is also where all of the ioctl calls live right now. There is also media/mtransport/gonk_addrs.cpp that is only used on Firefox OS (most commonly referred to as B2G), but that does not seem to support IPV6 anyway. Also, don't forget about WIN32 here. I have not looked yet whether it has a concept of deprecated addresses.
Flags: needinfo?(docfaraday)
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3

I was just about to report this as well. This needs to be fixed. As people are using more video conferencing these days, leaking historical IPv6 addresses out to random people is a VERY bad thing to do.

I just looked a little bit closer. It's worse than this. It leaks ALL the IPv6 addresses, even the non-temporary ones. So on my MBP (MacOSX), it leaks the MAC address of my machine, allowing full tracking over the entire lifetime of my machine.

Priority: P3 → P2

Yeah, draft-ietf-rtcweb-transports (sec 3.3) says we should discard permanent IPv6 addresses if there are non-deprecated temporary ones present. Presumably, when operating in mode 2 we want to ensure that we prefer temp addresses also. That still allows the use of permanent addresses if there are no temporary addresses; is that the intent?

Flags: needinfo?(mt)
Flags: needinfo?(drno)
Assignee: nobody → docfaraday

While we're at it, we probably want to avoid using things like IFA_F_OPTIMISTIC, IFA_F_DADFAILED/IN6_IFF_DUPLICATED, IFA_F_TENTATIVE/IN6_IFF_TENTATIVE, IN6_IFF_DETACHED, and IN6_IFF_ANYCAST.

So the filtering out of DAD failures and whatnot seems fine, but it's worth poking at the principles behind mode X.

Mode 2 says that you provide local IPs, but only for the route you are using. So in that case, it's probably not terrible to use a permanent address, because that is analogous in many ways to a v4 address, I think can be equated to a permanent address. Obviously, we can (and should) prefer a temporary address, but we don't need to insist on it.

I would probably apply the same logic to mode 1. There is no value to us in using a permanent address when a temporary address exists. These interactions are inherently temporary.

Obviously, modes 3 and 4 should not provide any local address, temporary or permanent.

(The deprecated temporary is a curious thing. Following the spec seems right: filter them out at first, keep one if it becomes deprecated and you are doing an ICE restart. I would add: ...only if it was on the nominated path. So if you had used that path, you should ADD the new non-deprecated temporary address. However, if you have no non-deprecated temporary addresses on that interface, then you provide the permanent address, just as you would at the start.)

Flags: needinfo?(mt)

Retaining deprecated interfaces that are currently nominated would require a whole bunch of plumbing, and that's a MAY level thing, so I'm not sure it would be worth our time. The only time that makes much sense to me is if we need to choose between continuing to use a deprecated temp addy and a permanent one, but it is unclear when this would actually happen in practice. If temp addresses are being used, how often do we end up in a scenario when our last temp addy is deprecated, and no new temp addy exists?

Yeah, I'm speaking from the position of ideals. If a restart gets a whole new set of addresses, without a deprecated temporary address, then I think things will work. It's a slight regression, but as you say, there is no significant risk that you have to fall back to a permanent address anyway.

Ok, in order to get this information on linux, we need to use netlink. We have a perfectly good netlink-based implementation of getifaddrs here, that we only use on android:

https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/ifaddrs-android.c

I'm going to try renaming this from ifaddrs-android to ifaddrs-netlink, and try using it on linux too. If that works ok, then I will refactor it to handle flag checking as it goes. If that all works, I will then implement an ifaddrs-bsd variant, based on getifaddrs and ioctl with SIOCGIFAFLAG_IN6.

We might also be able to ask Necko to supply these addresses, instead of NrIceCtx. https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/media/mtransport/ipc/StunAddrsRequestParent.cpp#149

Worth looking into.

On the subject of Necko, is there anything we need to do to support RFC 6724? Is that all handled at the kernel level? Would there be any value in modifying NetlinkService to expose interface details like the IFA_F_* flags?

Flags: needinfo?(honzab.moz)

Could you try this binary (you said you were testing on OS X) and see if it only uses the privacy addresses?

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/AwuqTFY5T3Sp4uIDo0Q6yw/runs/1/artifacts/public/build/target.dmg

Flags: needinfo?(mozbugz)

Trying to find some others who have access to an IPv6 environment for testing. Try at least has IPv6 addresses, but I don't have any visibility into whether it uses privacy addresses or not.

Nevermind, that binary was not working correctly.

Flags: needinfo?(mozbugz)
Flags: needinfo?(drno)

Ok, go ahead and try this one: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/fn8qWmZHRYSTbgIFvWHXGw/runs/1/artifacts/public/build/target.dmg

Flags: needinfo?(mozbugz)
Flags: needinfo?(drno)

Results sent via slack.

Flags: needinfo?(dminor)

Most recent build seems to work on Linux and Windows (thanks Dan!).

Attachment #9147320 - Attachment description: Bug 1183145: (WIP) Refactor, and teach stun addrs code to remove non-privacy IPv6 addresses if privacy IPv6 addresses are available. → Bug 1183145: Add a flags field to nr_local_addr, mark temp IPv6 addresses, and filter non-temp IPv6 if temp IPv6 are available.
Attachment #9150132 - Attachment description: Bug 1183145: Move platform-specific code from addrs.c into separate files, and teach them IPV6 address filtering. → Bug 1183145: Move platform-specific code from addrs.c into separate files.
Attachment #9147320 - Attachment is obsolete: true

I am sorry for such a late reply. Honestly I don't know the answer off-hand, so I'll forward to Dragana and Michal, in case they know.

The question asked is in comment 14.

Flags: needinfo?(michal.novotny)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dd.mozilla)

I'm going to wait until after the merge to land this.

Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a759f9651479 Rename a function to better reflect what it does, and fix a bug where handling of teredo and mac-based IPv6 was mixed up. r=mjf https://hg.mozilla.org/integration/autoland/rev/a74c46669606 Add a flags field to nr_local_addr so IPv6 addresses can be marked as temporary, and filter non-temp IPv6 if temp IPv6 are available. r=mjf https://hg.mozilla.org/integration/autoland/rev/24a4472452b8 Move platform-specific code from addrs.c into separate files. r=mjf https://hg.mozilla.org/integration/autoland/rev/99cce372d20c Teach platform-specific code to filter out inappropriate IPv6 addresses, and mark temp addresses. r=mjf
Regressions: 1643169

(In reply to Byron Campen [:bwc] from comment #14)

Would there be any value in modifying NetlinkService to expose interface details like the IFA_F_* flags?

Where it would be used? NetlinkService is used solely for calculating network ID and determining link status. But if some of the information is useful anywhere else, we can expose it.

Flags: needinfo?(michal.novotny)

(In reply to Michal Novotny [:michal] from comment #55)

(In reply to Byron Campen [:bwc] from comment #14)

Would there be any value in modifying NetlinkService to expose interface details like the IFA_F_* flags?

Where it would be used? NetlinkService is used solely for calculating network ID and determining link status. But if some of the information is useful anywhere else, we can expose it.

If that is all it is used for, then moving that capability there would probably not be of much use. If, someday, there's some other code that needs more in-depth network interface information, we could think about moving it there.

Regressions: 1649678
Flags: needinfo?(dd.mozilla)

Declaring NI bankruptcy.

Flags: needinfo?(drno)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: