Slight memory leak in AddTimeData() #6490

1. 
   hno commented at 9:36 am on July 29, 2015: none

   The list of known node addresses (setKnown) is never pruned, making it collect information about all nodes ever seen since startup.

   This allows for a slow DoS on IPv6 connected nodes.

2. hno referenced this in commit 1dac192776 on Jul 29, 2015
3. 
   hno commented at 12:44 pm on July 29, 2015: none

   The above is an attempt to fix this. Not yet verified.
4. 
   jonasschnelli commented at 12:55 pm on July 29, 2015: contributor

   It’s basically not a leak,… there is just no cap. Assuming a CNetAddr requires 20bytes (16bytes ip, some vector overhead), connecting to 50'000 nodes will fill up your memory +~1MB. I agree that a cap would be nice, but i don’t see how it would be possible to DoS. Reconnecting and sending again a version message would not increase the uses memory because the offsets are stored in a std::set with the ip address as set key.
5. 
   sipa commented at 1:03 pm on July 29, 2015: member

   A vector has around 32 bytes overhead on 64-bit systems.
6. 
   jonasschnelli commented at 1:09 pm on July 29, 2015: contributor

   @sipa: thanks for the info. So if you can control 50'000 ips (IPv6) you can increase the mem consumption of a attacked node about ~2.4MB by closing and reconnecting from a different ip to not exceed the -maxconnections. But i would guess there are better options to attack a bitcoin node if you can control a big amount of ips.

   But sure, a cap would be nice.

7. 
   casey commented at 4:06 pm on July 29, 2015: contributor

   IPv6 addresses are typically allocated, even to end users, in blocks of 2^64, so the attack isn’t as crazy as it sounds.
8. 
   jonasschnelli commented at 6:33 am on July 30, 2015: contributor

   Hmmm… just analyzed the code and i can’t see a place where AddTimeData() keeps a nOffsetSample or a CNetAddr in memory. Looks like the only thing hold im mem is the nTimeOffset (single int64_t, not increasing).

   But found a more effective attack: connect, misbehave, get added on the banlist (https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L3838). A banMap entry will reserve at least 302 bytes (CSubNet and CBanEntry).

   Maybe someone could point out a possible AddTimeData()? Meanwhile i try to implement a upper bound for the banlist.

9. 
   paveljanik commented at 10:14 am on August 7, 2015: contributor

   I think we should just return early from AddTimeData() when we already have enough time samples (because we do nothing in such cases anyway - see the comment there). Something like

   0if (setKnown.size() == 200)
   1    return;
   

   immediately after definition of static setKnown.

   The current debugging output of “Added time data, samples…” is misleading anyway because the newly added time data are not used to update the time offset.

10. 
    laanwj commented at 4:01 pm on September 4, 2015: member

    Fixed by #6545
11. laanwj closed this on Sep 4, 2015

    ---

12. MarcoFalke locked this on Sep 8, 2021

Contributors
hno jonasschnelli sipa casey paveljanik laanwj