Duplicate peer socket address in torrents repository

[josecelano]

I've been doing manual testing. I was adding new torrents to the live demo:

https://index.torrust-demo.com

I was using several BitTorrent clients, and at some point, I had a duplicate peer in the tracker. Actually, It's not a duplicate peer because the peer ID is different, but the peer socket address is the same.

We already knew about this potential issue.

This is the tracker response for the torrent:

{
  "info_hash": "cc7daef5908af33c28c410573dff50942cf339b0",
  "seeders": 0,
  "completed": 0,
  "leechers": 2,
  "peers": [
    {
      "peer_id": {
        "id": "0x2d4b54323338342d0000416b3165494941576e36",
        "client": "KTorrent"
      },
      "peer_addr": "2.137.90.142:6881",
      "updated": 1713901886647,
      "updated_milliseconds_ago": 1713901886647,
      "uploaded": 0,
      "downloaded": 0,
      "left": 195636,
      "event": "None"
    },
    {
      "peer_id": {
        "id": "0x2d4b54323338342d00006c446e6d70426f71634d",
        "client": "KTorrent"
      },
      "peer_addr": "2.137.90.142:6881",
      "updated": 1713900682225,
      "updated_milliseconds_ago": 1713900682225,
      "uploaded": 0,
      "downloaded": 0,
      "left": 195636,
      "event": "None"
    }
  ]
}

I don't know why the peer ID changed. Maybe KTorrent uses a different peer ID when you restart it.

The question is, should we allow this? I think it makes no sense to return two peers because other clients will connect to the same socket address. I would use the socket address instead of the peer ID as a key for the PeerList. If the peer id changes the tracker would return only the one that has announced the latest.

cc @torrust/maintainers

[josecelano]

Hi @da2ce7 any feedback about this?

[mario-nt]

@josecelano I think we should not allow to have that, as you said, it makes no sense to have two different peers with the same IP.

I wonder if the same user is using for example two bittorrent clients at the same time, it would share more data than with just one? or if it is getting more requests due to appeared two times (two different peer id) (considering the bandwith is not acting as a bottleneck) I am not sure this make sense.

[josecelano]

Hi @mario-nt thank you for your feedback.

@josecelano I think we should not allow to have that, as you said, it makes no sense to have two different peers with the same IP.

I'm considering two options:

Option 1. Allow this in the repository and remove duplicates in the request response. That way, we know when a client is using more than one peer ID.
Option 2. Don't allow it. USe the socket address as ID in the PeerList.

I think I'm going to implement option 2 because option 1 can make the tracker slower and I think knowing that a peer is changing its ID is not relevant for the tracker service.

I wonder if the same user is using for example two bittorrent clients at the same time, it would share more data than with just one? or if it is getting more requests due to appeared two times (two different peer id) (considering the bandwith is not acting as a bottleneck) I am not sure this make sense.

I suppose the only way to do that is with a client proxy, where you expose a single socket address but split the traffic internally. But I don't think such a thing exists or is useful. I see that it's most likely that the client changed the ID for some reason.

[josecelano]

Option 3 is just to let the BitTorrent client handle it, but I wonder if this could allow an attack 🤔. You can announce using a different peer ID each time increasing the memory consumption.

[josecelano]

Option 4 is to check if the socket address already exists before inserting the new peer, but it's also too slow.

UPDATE

If there is another using the same socket address, we keep the old one, and we don't insert the new one. Just to differentiate this option from option 5.

[josecelano]

Hi @mario-nt @da2ce7 I was implementing the option 2 and I realized there was a problem. The stats behavior changes. In the current version, we count a download as completed when the peer has announced at least twice.

If a peer with a ID1 announces a complete event and then announces again using the same IP but a different peer ID. In the current version there are no completed peers yet because we consider them different peers. In the new version there is 1 complete peer because we consider it the same peer.

I haven't considered the opposite case, when a peer with one ID changes its IP. Maybe the peer has a dynamic IP, and after rebooting the system, the peer ID is the same, but the IP has changed. In that case, if the peer completes the download the stats are not going to be increased.

In conclusion, I think we should do nothing (option 3).

Regarding the attack, you cannot do it unless you enable this. Becuase we always ignore the IP in the request. We get the IP from the request. If that feature is implemented you have to ensure that does not happen when enabled (because it's a private network or because you use another mechanism to detect and stop that attack).

[josecelano]

Hi @mario-nt @da2ce7 I was implementing the option 2 and I realized there was a problem. The stats behavior changes. In the current version, we count a download as completed when the peer has announced at least twice.

If a peer with a ID1 announces a complete event and then announces again using the same IP but a different peer ID. In the current version there are no completed peers yet because we consider them different peers. In the new version there is 1 complete peer because we consider it the same peer.

I haven't considered the opposite case, when a peer with one ID changes its IP. Maybe the peer has a dynamic IP, and after rebooting the system, the peer ID is the same, but the IP has changed. In that case, if the peer completes the download the stats are not going to be increased.

In conclusion, I think we should do nothing (option 3).

Regarding the attack, you cannot do it unless you enable this. Becuase we always ignore the IP in the request. We get the IP from the request. If that feature is implemented you have to ensure that does not happen when enabled (because it's a private network or because you use another mechanism to detect and stop that attack).

I was wrong. The attack to generate many peers with different IDs and the same IP is possible in the current version. I guess we should implement option 4 or option 5.

option 5

If a new peer announces itself using a socket address already being used by another peer (a peer with a different ID), we remove the previous one from the list. With option 4 we don't insert the new one but I think this is the right way because the new peer controls the IP now.

[josecelano]

I think the problem is a more generic problem. If a new peer (a peer using a new ID) starts using the same IP address as a previous one, we should remove the previous one on all the peer lists (all the swarms). That would be the right solution because that means the IP is not longer controlled by the previous peer. The problem with this solution is it would be too slow. We have to iterate over all torrents and all peers in a torrent to remove the peers using the same IP if their ID don't match the one for the peer making the new request.

On the other hand, we supposedly remove inactive peers so the peers using the same IP but the old ID will be removed anyway when the tracker does the periodic cleaning (by default every 10 minutes. See inactive_peer_cleanup_interval config option). So there is a limit to the number of peers an attacker can add to the peer lists. The tracker will only keep as many peers as the number of requests can handle in 10 minutes. Therefore, I suppose this attack turns into a normal DDos attack. And we don't have any protection against those attacks. For example, if a client tries to announce thousands of torrents.

I'm closing the issue again, but fixing the memory consumption attack depends on fixing this issue.