Opened 12 years ago

Closed 12 years ago

#2667 closed Enhancement (fixed)

Ignore martian addresses

Reported by: jch Owned by: charles
Priority: Normal Milestone: 1.80
Component: libtransmission Version: 1.76
Severity: Minor Keywords:
Cc:

Description

As discussed on IRC.

Attachments (3)

Change History (12)

Changed 12 years ago by jch

comment:1 Changed 12 years ago by jch

Should we apply the same test to PEX messages? To addresses returned from the tracker?

comment:2 Changed 12 years ago by jch

  • Severity changed from Normal to Minor
  • Type changed from Bug to Enhancement

comment:3 Changed 12 years ago by charles

  • Component changed from Transmission to libtransmission
  • Milestone changed from None Set to 1.80
  • Owner set to charles
  • Status changed from new to assigned
  • Version changed from 1.76+ to 1.76

comment:4 Changed 12 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

charles * r9748 libtransmission/ (net.c net.h peer-msgs.c): (trunk libT) #2667 "Ignore martian addresses in LTEP messages" -- implemented for 1.80

comment:5 Changed 12 years ago by charles

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Summary changed from Ignore martian addresses in LTEP messages to Ignore martian addresses

Reopened pending review by jch. <http://trac.transmissionbt.com/changeset/9778/>

09:39 < CIA-39> charles * r9778 libtransmission/ (net.c net.h peer-io.c peer-msgs.c): (trunk libT) #2667 "ignore martian addresses" -- modified this ticket. previously we ignored them from LTEP; now we ignore them regardless of the source. this commit implements this filtering change.

jch: I made isMartian() to a subfunction of tr_isValidPeerAddress() because the latter is already a gatekeeper against strange IP addresses regardless of who gave them to us (pex, ltep, tracker, dht, resume, etc). Accordingly, I've removed the isMartian() calls in peer-msgs.c, since tr_peerMgrAddPex() already filters with tr_isValidPeerAddress().

One aspect of this patch I'd particularly like your opinion on is this change in tr_netOpenPeerSocket():

-  if( isMulticastAddress( addr ) || isIPv6LinkLocalAddress( addr ) )
+  if( tr_isValidPeerAddress( addr, port ) )
    return -EINVAL;

tr_isValidPeerAddress() contains both of those two tests, but -- aside from the new martian code -- it also had a third one: isIPv4MappedOrCompatAddress(). I don't know if that was omitted from tr_netOpenPeerSocket() by accident or on purpose. Would a peer's address ever return true in isIPv4MappedOrCompatAddress()?

comment:6 Changed 12 years ago by jch

v4-mapped addresses should never happen -- we use IPV6_V6ONLY to ensure that (net.c line 405). Hence the test against such addresses is harmless.

v4-compatible is a different story. These addresses were used in a transition technology that is now obsolete. They are not used currently, and while it is unlikely they will be reused some day, nothing prevents it.

I would suggest removing the test against v4-compatible, and replace it with a test that only eliminates :: and ::1 (which is already done in isMartian). But I don't think it matters much, at least for the following 10 years.

--Juliusz

comment:7 Changed 12 years ago by charles

  • Resolution set to fixed
  • Status changed from reopened to closed

charles * r9780 libtransmission/net.c: (trunk libT) #2667 "ignore martian addresses" -- revise the tests to tr_isValidPeerAddr() based on feedback from jch @ http://trac.transmissionbt.com/ticket/2667#comment:6

comment:8 Changed 12 years ago by jch

  • Resolution fixed deleted
  • Status changed from closed to reopened

Bunch of simplifications:

  • multicast already covered in martian, flush separate function;
  • v4-mapped already covered outside martian, flush from martian.

Explanation of multicast:

  • the v6 test addr[0]==0xFF is completely equivalent to multicast;
  • the v4 test addr[0]&0xE0==0xE0 covers both multicast and a bunch of reserved addresses.

Feel free to retain the multicast test if you find it clearer, although I think it's confusing to have the same test in multiple places. --Juliusz

comment:9 Changed 12 years ago by charles

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks!

12:06 < CIA-39> charles * r9781 libtransmission/net.c: (trunk libT) #2667 "Ignore martian addresses" -- apply jch's 0001-Simplify-martian-address-detection.patch cleanup patch from http://trac.transmissionbt.com/ticket/2667#comment:8

Note: See TracTickets for help on using tickets.