Closed Bug 1219557 Opened 4 years ago Closed 4 years ago

Only pair addresses from 1918 ranges with others from the same range

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
Blocking Flags:

People

(Reporter: mt, Assigned: drno)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This might reduce the number of candidate pairs we have to worry about.
Does help with my desktop accumulating tons of temporary IPv6 addresses, but I like the idea.
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
It might break some strange 1918 inter office routings though, or make these fall back to server reflexive or even TURN connections. Worth adding a user preference to turn it off?
Meant to say "Does not help..." in comment #1.
Bug 1219557: don't pair candidates with addresses from different RFC1918 networks. r?mt
Attachment #8681429 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/23829/#review21299

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1016
(Diff revision 1)
> +      if(nr_transport_addr_is_rfc1918(&pcand->addr) !=
> +         nr_transport_addr_is_rfc1918(&lcand->addr))
> +        continue;

I wonder if it makes sense to merge in the ip_version here.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:436
(Diff revision 1)
> +          case AF_INET:
> +            if (((ntohl(addr->u.addr4.sin_addr.s_addr)>>24)&0xff)==0x0a)
> +              return(1);
> +            if ((((ntohl(addr->u.addr4.sin_addr.s_addr)>>24)&0xff)==0xac) &&
> +                (((ntohl(addr->u.addr4.sin_addr.s_addr)>>16)&0xff)>=0x10) &&
> +                (((ntohl(addr->u.addr4.sin_addr.s_addr)>>16)&0xff)<=0x1f))
> +              return(2);
> +            if ((((ntohl(addr->u.addr4.sin_addr.s_addr)>>24)&0xff)==0xc0) &&

Not a big fan of this if statement and the individual byte addressing. Why not instead use a  mask and a value.

They're right there in the RFC

 10.0.0.0        -   10.255.255.255  (10/8 prefix)
     172.16.0.0      -   172.31.255.255  (172.16/12 prefix)
     192.168.0.0     -   192.168.255.255 (192.168/16 prefix)

I would also tend to a table rather than an else.
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

What ekr said, also please detect IPv6 link local addresses as a separate class.

We should also detect RFC6598 addresses and group them in the same way.

Thinking about this more, this code should be a more generalized form of the canPairCandidates stuff that we already have to prevent pairing v6 with v4.  To that end, v6 addresses should get a different value to v4.
Attachment #8681429 - Flags: review?(martin.thomson)
Assignee: nobody → drno
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Bug 1219557: don't pair candidates from different reserved networks. r?mt
Attachment #8681429 - Attachment description: MozReview Request: Bug 1219557: don't pair candidates with addresses from different RFC1918 networks. r?mt → MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r?mt
Attachment #8681429 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/23829/#review21299

> I wonder if it makes sense to merge in the ip_version here.

Not 100% sure if what I got now is what you meant.

> Not a big fan of this if statement and the individual byte addressing. Why not instead use a  mask and a value.
> 
> They're right there in the RFC
> 
>  10.0.0.0        -   10.255.255.255  (10/8 prefix)
>      172.16.0.0      -   172.31.255.255  (172.16/12 prefix)
>      192.168.0.0     -   192.168.255.255 (192.168/16 prefix)
> 
> I would also tend to a table rather than an else.

I'm using masks and values now.
But I don't quite get what you mean by a table in this case. Could you give an example?
https://reviewboard.mozilla.org/r/23829/#review21299

> I'm using masks and values now.
> But I don't quite get what you mean by a table in this case. Could you give an example?

What I meant was something like;

typedef struct nr_private_addr_ {
  UINT4 addr;
  UINT4 mask;
} nr_private_addr;

nr_private_addr private_addrs[] = {
   XXX, YYY,
   ...
}

for (i=0; i<(sizeof(private_addrs)/sizeof(private_addr)); i++) {
  if ((ntohl(addr->u.addr4.sin_addr) & private_addrs[i].mask) == private_addrs[i].addr) {
    return i + 1;
  }
}

return 0;
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Bug 1219557: don't pair candidates from different reserved networks. r?mt
Attachment #8681429 - Flags: review?(martin.thomson)
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

https://reviewboard.mozilla.org/r/23829/#review21791

The general structure here is fine now.  But it needs tests.  I think that you will discover the endian conversion bugs if you had some tests.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:401
(Diff revision 3)
> +        if ((ntohl(addr->u.addr4.sin_addr.s_addr) & htonl(0xFFFF0000)) == htonl(0xA9FE0000))

I think that your endianness is a little off here.

Once you use ntohl on the addr, then you can just use the bare 0xffff0000L value.  Or, and this is probably better, since the addr is in network order, only convert the constants to network order.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:456
(Diff revision 3)
> +int nr_transport_addr_is_reserved(nr_transport_addr *addr)

s/reserved/private_use/

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:463
(Diff revision 3)
> +            if ((ip & ntohl(private_ipv4_addrs[i].mask)) == private_ipv4_addrs[i].addr)

Again, I think that the endianness code here is partial.
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/3-4/
Attachment #8681429 - Flags: review?(martin.thomson)
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

https://reviewboard.mozilla.org/r/23829/#review22589

Big issue here: pairing 192.168.1.1 (private) with 1.2.3.4 (public) still needs to work.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:993
(Diff revision 4)
> +int nr_ice_component_can_candidate_addr_pair(nr_transport_addr *left, nr_transport_addr *right)

I think that I'd prefer if this returned a boolean.  The different values here aren't going to be of any use.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:997
(Diff revision 4)
> +    if(nr_transport_addr_is_private_use(left) !=
> +       nr_transport_addr_is_private_use(right))
> +      return(2);

If the local address is from a private use range, don't we still want to pair it with a remote public IP?  This test needs to take the zero value into account.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:401
(Diff revision 4)
> +        if ((addr->u.addr4.sin_addr.s_addr & htonl(0xFFFF0000)) == htonl(0xA9FE0000))

nit: I'd reverse the htonl/ntohl arrangement, as I suggest below.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:456
(Diff revision 4)
> +int nr_transport_addr_is_private_use(nr_transport_addr *addr)

I think that this name is misleading.  The actual purpose is to get the private address use range.

int nr_transport_get_private_use_addr_range(\*addr)

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:461
(Diff revision 4)
> +          UINT4 ip = addr->u.addr4.sin_addr.s_addr;

Run ntohl() on this line and avoid the multiple rounds of htonl below.
Attachment #8681429 - Flags: review?(martin.thomson)
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/4-5/
Attachment #8681429 - Flags: review?(martin.thomson)
After so many issues being identified in code review I think this need some unit tests before shipping to prevent any serious accidents. Which unfortunately it not going to be trivial.
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

https://reviewboard.mozilla.org/r/23829/#review22679

But don't ship it without sorting out the tests...

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1001
(Diff revision 5)
> +    right_range = nr_transport_addr_get_private_addr_range(right);
> +    if(right_range && (nr_transport_addr_get_private_addr_range(left) !=
> +       right_range))

If right is the remote peer, please make sure that you note that in a comment.  This didn't used to matter, but now you have some asymmetry in the logic.  That asymmetry probably needs some explanation too.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1006
(Diff revision 5)
> +    return(0);



::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1036
(Diff revision 5)
> -      if (lcand->tcp_type && !pcand->tcp_type)
> +      if(nr_ice_component_can_candidate_addr_pair(&lcand->addr, &pcand->addr))

I just realized this: you have a can_candidate_addr_pair returning false(0) if the pair can be paired.  Maybe invert the return value.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1038
(Diff revision 5)
> -      if(pcand->addr.ip_version != lcand->addr.ip_version)
> +      if(nr_ice_component_can_candidate_tcptype_pair(lcand->tcp_type, pcand->tcp_type))

Same as for the other.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:407
(Diff revision 5)
> -      return ((*addrTop & htonl(0xFFC00000)) == htonl(0xFE800000));
> +          if ((*addrTop & htonl(0xFFC00000)) == htonl(0xFE800000))

Nit: missed one

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:463
(Diff revision 5)
> +            if ((ip & private_ipv4_addrs[i].mask) ==
> +                 private_ipv4_addrs[i].addr)

Nit: probably fits on the one line.
Attachment #8681429 - Flags: review?(martin.thomson) → review+
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/5-6/
Attachment #8681429 - Attachment description: MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r?mt → MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r=mt
https://reviewboard.mozilla.org/r/23827/#review22875

AFAICT, this test breaks in a few places if you try running the test with a real STUN/TURN server (which is a test-case that would be nice to have, since some of this behavior deals with when we pair local srflx/relay with a remote candidate).

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:407
(Diff revisions 5 - 6)
> -          if ((*addrTop & htonl(0xFFC00000)) == htonl(0xFE800000))
> +          if ((ntohl(*addrTop) & 0xFFC00000) == 0xFE800000)

Why the change? htonl of a constant is calculated at compile-time, and htonl'ing both sides again doesn't make the behavior any different.

::: media/mtransport/test/ice_unittest.cpp:127
(Diff revision 6)
> +  if (candidate.find(".") != std::string::npos) {

Probably want to look for ':' instead; IPV6 addys can have '.' in some notations.

::: media/mtransport/test/ice_unittest.cpp:2910
(Diff revision 6)
> +  for (size_t i=0; i < candidates.size(); i++) {

Let's use the for ( : ) syntax here.

::: media/mtransport/test/ice_unittest.cpp:2913
(Diff revision 6)
> +      std::size_t first = candidates[i].find(" ", 25);

Hmm, this seems fragile. The foundation id (the token right after the 'a=candidate:') is not necessarily a single character, nor is the component id. This is also kinda magic-constanty. I guess there's a few things that you could do here:

1) Try to reuse nr_ice_peer_candidate_from_attribute (would require a little refactoring to isolate the parse code)
2) Try to get the local candidates as NrIceCandidate
3) Write some simple token-skipping parse code.

::: media/mtransport/test/ice_unittest.cpp:2918
(Diff revision 6)
> +        std::cerr << "Failed to parse ip: " << ip.c_str() << std::endl;

I would expect this to fail the test.

::: media/mtransport/test/ice_unittest.cpp:2922
(Diff revision 6)
> +        host_net = nr_transport_addr_get_private_addr_range(&addr);

Hmm, we should probably either try to detect cases where there is more than one private address range being used and fail the test here, or try to determine the set of private address ranges in use.

Also, won't this return 0 for srflx/relay if we're testing against a real STUN/TURN server, and stomp the value for our host candidates?

::: media/mtransport/test/ice_unittest.cpp:2933
(Diff revision 6)
> +  for (size_t i=0; i < pairs.size(); i++) {

Let's use the for ( : ) syntax here.

::: media/mtransport/test/ice_unittest.cpp:2936
(Diff revision 6)
> +    ASSERT_TRUE(nr_transport_addr_get_private_addr_range(&addr) == host_net);

It seems like this won't always be true if we're testing against a real STUN/TURN server.
https://reviewboard.mozilla.org/r/23827/#review22875

> Why the change? htonl of a constant is calculated at compile-time, and htonl'ing both sides again doesn't make the behavior any different.

This was done on Martin's request. But I hear you. Changed it back for this case.

> Probably want to look for ':' instead; IPV6 addys can have '.' in some notations.

Problem with that is the : right after the initial candidate keyword. I'll probably have to use a tokenizer here as well.

> It seems like this won't always be true if we're testing against a real STUN/TURN server.

I'm disabling the STUN servers in the Gather() call on purpose. I don't see any value in running this against a public STUN server.
Because server reflexive candidates (assuming the STUN server hopefully reported a public IP back - not some bizarre enterprise multi layer NAT scenario) will still get paired with their public IP to any of the local IPs.
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/6-7/
Attachment #8681429 - Flags: review?(docfaraday)
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/7-8/
(In reply to Nils Ohlmeier [:drno] from comment #19)
> https://reviewboard.mozilla.org/r/23827/#review22875
> > It seems like this won't always be true if we're testing against a real STUN/TURN server.
> 
> I'm disabling the STUN servers in the Gather() call on purpose. I don't see
> any value in running this against a public STUN server.
> Because server reflexive candidates (assuming the STUN server hopefully
> reported a public IP back - not some bizarre enterprise multi layer NAT
> scenario) will still get paired with their public IP to any of the local IPs.

   Being able to run this test against a real STUN server could be useful since that would test the pairing logic on candidates that aren't on a reserved network (we don't want to pair a local srflx with a reserved network remote candidate).
Attachment #8681429 - Flags: review?(docfaraday)
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

https://reviewboard.mozilla.org/r/23829/#review23209

::: media/mtransport/test/ice_unittest.cpp:2938
(Diff revisions 6 - 7)
> +          if (host_net)
> +            FAIL() << "This test doesn't support multiple private interfaces";

If we aren't going to have support for this, we probably want a TODO for it with a bug #. I worry that an infra change could bust this test, and given the time pressure we would end up disabling it.

::: media/mtransport/test/ice_unittest.cpp:2979
(Diff revisions 6 - 7)
> +    ASSERT_TRUE(nr_transport_addr_get_private_addr_range(&addr) != 0);

What if the test is being run on a machine with a non-reserved interface?
Blocks: 1226838
Attachment #8681429 - Attachment description: MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r=mt → MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r=mt,r?bwc
Attachment #8681429 - Flags: review?(docfaraday)
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/7-8/
https://reviewboard.mozilla.org/r/23829/#review23209

> If we aren't going to have support for this, we probably want a TODO for it with a bug #. I worry that an infra change could bust this test, and given the time pressure we would end up disabling it.

Added TODO reference to bug 1226838

> What if the test is being run on a machine with a non-reserved interface?

I don't know any networks besides IETF meeting networks where you would get public IPs. Added it to the TODO list in bug 1226838.
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

https://reviewboard.mozilla.org/r/23829/#review23615

::: media/mtransport/test/ice_unittest.cpp:2954
(Diff revisions 7 - 8)
> -  int host_net = 0;
> -  for (auto c : candidates) {
> +    std::cerr << "This test needs exactly one private IPv4 host candidate to work" << std::endl;
> +    return;

I really think we need to make the test fail when this happens. Any change in CI that trips over this will never be noticed otherwise.

::: media/mtransport/test/ice_unittest.cpp:2982
(Diff revisions 7 - 8)
> +    std::cerr << "Don't run this test at IETF meetings!" << std::endl;
> +    std::cerr << "This test needs one private IPv4 host candidate to work" << std::endl;
> +    return;

Please cause the test to fail here.
Attachment #8681429 - Flags: review?(docfaraday) → review+
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/8-9/
Attachment #8681429 - Attachment description: MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r=mt,r?bwc → MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc
That try run looks like a Xmas tree... and apparently lots of our test machines don't have DNS resolvers set/available?!
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/9-10/
this fails to apply:

applying 381cb10f5726
patching file media/mtransport/third_party/nICEr/src/ice/ice_component.c
Hunk #2 FAILED at 1030
1 out of 2 hunks FAILED -- saving rejects to file media/mtransport/third_party/nICEr/src/ice/ice_component.c.rej
patch failed to apply
Flags: needinfo?(drno)
Keywords: checkin-needed
Comment on attachment 8681429 [details]
MozReview Request: Bug 1219557: don't pair candidates from different reserved networks. r+mt,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23829/diff/10-11/
Strangely my mercurial was able to merge the colliding pieces... anyway, fixed.
Flags: needinfo?(drno)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/357f32e43e16
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1259842
You need to log in before you can comment on or make changes to this bug.