Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5157 closed Bug (fixed)

check valid address before UTP_Create()

Reported by: reardon Owned by: jordan
Priority: Normal Milestone: 2.74
Component: libtransmission Version: 2.73
Severity: Normal Keywords:
Cc:

Description

While debugging other uTP code, I noticed that there is no addressing sanitizing, particularly for IPv6, when attempting to setup a uTP connection.

Seems like code in tr_netOpenPeerUTPSocket() should:

    assert( tr_address_is_valid( addr ) );

    if( !tr_address_is_valid_for_peers( addr, port ) )
        return -EINVAL;

prior to UTP_Create(), just like the sanity check in tr_netOpenPeerSocket()

This will avoid a ton of misfires due to bogus (mostly link-local) addresses we get from LTEP. Also, sanity check gives us a place to filter out Teredo and 6in4 peer connections (ie 2001:0::/32 and 2002::/16) should we want (see #4196)

Change History (9)

comment:1 Changed 8 years ago by cfpp2p

take a look at:

peer-io.c line 682

    assert( session );
    assert( tr_address_is_valid( addr ) );
    assert( torrentHash );

    if( utp )
        utp_socket = tr_netOpenPeerUTPSocket( session, addr, port, isSeed );

    if( !utp_socket ) {
        fd = tr_netOpenPeerSocket( session, addr, port, isSeed );
        dbgmsg( NULL, "tr_netOpenPeerSocket returned fd %d", fd );
    }

does that help sort any of this out for you?

comment:2 Changed 8 years ago by reardon

Huh? Are you talking about the assert()?? Those are not relevant to release code, but regardless those do not check for the condition I'm referring to.

What follows the assert() is the (somewhat odd, different than uT) logic which tries uTP before trying regular TCP connections. Won't insulate us from bad addresses received via LTEP/PEX.

comment:3 Changed 8 years ago by jordan

  • Component changed from Transmission to libtransmission
  • Owner set to jordan

comment:4 Changed 8 years ago by jordan

  • Milestone changed from None Set to 2.80
  • Status changed from new to assigned

I like this idea. Added in r13628, copied from tr_netOpenPeerSocket()

comment:5 Changed 8 years ago by jordan

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

comment:6 Changed 8 years ago by jordan

  • Milestone changed from 2.80 to 2.74

comment:7 Changed 8 years ago by reardon

I think it needs to return NULL instead of error, since the caller is not designed to handle error values. Alternatively, code elsewhere needs changes so that both netOpenPeer... routines share parallel logic, which they don't today.

comment:8 Changed 8 years ago by jordan

D'oh! That was a terrible commit, thanks.

comment:9 Changed 8 years ago by x190

Fixed in: r13629.

Last edited 8 years ago by x190 (previous) (diff)
Note: See TracTickets for help on using tickets.