Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1837823005:
GetDefaultLocalAddress should return the bestIP (Closed)
DescriptionGetDefaultLocalAddress should return the bestIP
on an IPv6 network that contains the actual default local address. This is for preventing potential IP leaking.
BUG=webrtc:5376
Committed: https://crrev.com/af83fe65d99f8a3a589dba8e96ea05da5d4bd7d0
Cr-Commit-Position: refs/heads/master@{#12417}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Address comments #Patch Set 3 : Rebase to the master #
Messages
Total messages: 23 (11 generated)
Description was changed from ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. BUG=5376 ========== to ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing IP leaking. BUG=webrtc:5376 ==========
Description was changed from ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing IP leaking. BUG=webrtc:5376 ========== to ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing potential IP leaking. BUG=webrtc:5376 ==========
Patchset #1 (id:1) has been deleted
honghaiz@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
https://codereview.webrtc.org/1837823005/diff/20001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1837823005/diff/20001/webrtc/base/network.cc#ne... webrtc/base/network.cc:366: Network* network = GetNetworkFromAddress(default_local_ipv6_address_); I think this could be a little more clear: *ipaddr = default_local_ipv6_address_; Network* default_ipv6_network = GetNetworkFromAddress(default_local_ipv6_address_); if (default_ipv6_network) { // The default ipv6 network's BestIP is different than default_local_ipv6_address_, use it instead. // This is to prevent potential IP address leakage. See WebRTC bug 5376. *ipaddr = network->GetBestIP(); } return true; https://codereview.webrtc.org/1837823005/diff/20001/webrtc/base/network.cc#ne... webrtc/base/network.cc:367: *ipaddr = (network != nullptr) ? network->GetBestIP() How do we know that GetBestIP() will return an ipv6 address? Is a Network guaranteed to only return IP addresses of either ipv4 or ipv6, but not both? Should we check that it's ipv6?
Thanks! PTAL. https://codereview.webrtc.org/1837823005/diff/20001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1837823005/diff/20001/webrtc/base/network.cc#ne... webrtc/base/network.cc:366: Network* network = GetNetworkFromAddress(default_local_ipv6_address_); On 2016/03/30 17:25:06, pthatcher1 wrote: > I think this could be a little more clear: > > *ipaddr = default_local_ipv6_address_; > Network* default_ipv6_network = > GetNetworkFromAddress(default_local_ipv6_address_); > if (default_ipv6_network) { > // The default ipv6 network's BestIP is different than > default_local_ipv6_address_, use it instead. > // This is to prevent potential IP address leakage. See WebRTC bug 5376. > *ipaddr = network->GetBestIP(); > } > return true; Revised. 1. we don't need to assign *ipaddr twice in some cases. 2. ipv6_network need not be prefixed withe default. https://codereview.webrtc.org/1837823005/diff/20001/webrtc/base/network.cc#ne... webrtc/base/network.cc:367: *ipaddr = (network != nullptr) ? network->GetBestIP() On 2016/03/30 17:25:06, pthatcher1 wrote: > How do we know that GetBestIP() will return an ipv6 address? Is a Network > guaranteed to only return IP addresses of either ipv4 or ipv6, but not both? > Should we check that it's ipv6? IPv6 address and IPv4 address will be contained in different networks. Network includes the interface name, the prefix and the length of the prefix.
pthatcher@google.com changed reviewers: + pthatcher@google.com
lgtm Although I'd like it if Justin also verified this is the behavior we want.
On 2016/03/30 18:42:49, pthatcher wrote: > lgtm > > Although I'd like it if Justin also verified this is the behavior we want. OK I will wait until Justin looked at this. Also please lgtm using your webrtc account for this one.
lgtm
On 2016/03/30 21:11:54, pthatcher1 wrote: > lgtm Justin, Could you look at this CL by Friday? It will be great if we can land this before the end of the week.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@google.com, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1837823005/#ps60001 (title: "Rebase to the master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837823005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1837823005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837823005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1837823005/60001
Message was sent while issue was closed.
Description was changed from ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing potential IP leaking. BUG=webrtc:5376 ========== to ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing potential IP leaking. BUG=webrtc:5376 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing potential IP leaking. BUG=webrtc:5376 ========== to ========== GetDefaultLocalAddress should return the bestIP on an IPv6 network that contains the actual default local address. This is for preventing potential IP leaking. BUG=webrtc:5376 Committed: https://crrev.com/af83fe65d99f8a3a589dba8e96ea05da5d4bd7d0 Cr-Commit-Position: refs/heads/master@{#12417} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/af83fe65d99f8a3a589dba8e96ea05da5d4bd7d0 Cr-Commit-Position: refs/heads/master@{#12417} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
