banlist.dat: store banlist on disk

jonasschnelli commented at 3:50 pm on June 19, 2015: contributor

Currently, the list of banned nodes are only held in memory. A restart of bitcoind would reset the set of banned nodes.

This PR introduces a new filestore (banlist.dat) which stores the IPs/Subnets of the banned nodes more or less identical to the CAddrDB (peers.dat).

Also added within this PR is a feature that removes unused banned node entries because the banned_till time has expired.

includes tests for the banlist disk storage.

Unsure if we should introduce a new cmd arg -storebanlist to optionally allow storing of banned node data. It could be, that some users expecting a sweep of all banned nodes when they restart bitcoind.

jgarzik commented at 3:53 pm on June 19, 2015: contributor

Nice feature. RE -storebanlist, the system should default to storing. Users may disable.

Or don’t create an option at all – same as CAddrDB, user may delete file to clear the list.

Diapolo commented at 7:52 pm on June 19, 2015: none

I’d also vote for treating this the same as peers.dat and don’t add an option for it.

in src/net.cpp: in 9c4b16b843 outdated

2228+    CAutoFile filein(file, SER_DISK, CLIENT_VERSION);
2229+    if (filein.IsNull())
2230+        return error("%s: Failed to open file %s", __func__, pathBanlist.string());
2231+
2232+    // use file size to size memory buffer
2233+    int fileSize = boost::filesystem::file_size(pathBanlist);

Diapolo commented at 7:55 pm on June 19, 2015:

Not sure if this is best to use an int here… what is file_size returning?

jonasschnelli commented at 7:58 pm on June 19, 2015:

Do you expect users with more then 2GB of banned node data? I have taken this approach from CAddrDB 1:1. But i agree file sizes should be held in uint64_t or in size_t if they are not serialized/shared anywhere.

Diapolo commented at 8:00 pm on June 19, 2015:

I think it’s plenty of space, but we should be 100% sure we don’t overflow IMHO? Yeah there are more parts in the code that don’t do that.

laanwj commented at 11:20 am on June 26, 2015:

Right: to be precise, file sizes should be uint64_t, not size_t. Remember that size_t depends on the address width, it is for in-memory sizes.

jonasschnelli commented at 1:22 pm on June 26, 2015:

Agreed. Changed from int to uint64_t (also in CAddDB).

in src/net.cpp: in 9c4b16b843 outdated

2233+    int fileSize = boost::filesystem::file_size(pathBanlist);
2234+    int dataSize = fileSize - sizeof(uint256);
2235+    // Don't try to resize to a negative number if file is small
2236+    if (dataSize < 0)
2237+        dataSize = 0;
2238+    vector<unsigned char> vchData;

Diapolo commented at 7:56 pm on June 19, 2015:

Maybe you could use std:: so we can get rid of using namespace std; easier :)?

laanwj added the label Feature on Jun 22, 2015

dexX7 commented at 0:08 am on June 26, 2015: contributor

@jgarzik: RE -storebanlist, the system should default to storing. Users may disable.

This seems risky in my opinion, because not all IPs are static, and they don’t necessarily always belong to the same node.

sipa commented at 0:47 am on June 26, 2015: member

dexX7: that is why all bans are temporary.

jonasschnelli: I think defaulting to storing them is perfectly fine (or even not offering the option).

jonasschnelli commented at 7:15 am on June 26, 2015: contributor

@jgarziks way of resetting the banlist (by removing banlist.dat) is very straightforward and doesn’t need any explanation. I’d like to avoid another cmd arg option to not end up in having thousands of tiny options.

Node misbehaving bans are by default 24h. All other bans needs conscious actions from the users. The chance that your node accidentally get an IP out of a dynamic range, where the previous owner managed to add his bitcoin-core on a ban-list, is very rar. And then, the chance that all nodes did ban this IP or that you connect to a node which conscious added your new IP to the banlist is even rarer We have 3,706,452,992 assignable public IPv4 addresses.

laanwj commented at 11:21 am on June 26, 2015: member

Concept ACK, also agree that defaulting to store bans is perfectly fine (so is not offering an option).

As any kind of persistence commits to a format it is good to think forward a bit what should be included: would it make sense to add e.g. a “ban source” enum field, that specifies whether the ban is automatic or manual?

jonasschnelli force-pushed on Jun 26, 2015

jonasschnelli commented at 1:52 pm on June 26, 2015: contributor

I like the idea of storing the “ban source”. Because we now keep/dump a serialized std::map<CSubNet, int64_t> everything beyond that would be a bigger change. We should probably introduce a simple class for the banlist entry like class CBanEntry { int64_t bantime, enum banSource }. This might also be capable of allowing ban distinction between multiple bans on the same IP but with different ports (but would mean moving from std::map to a vector).

laanwj commented at 3:20 pm on June 26, 2015: member

We should probably introduce a simple class for the banlist entry like class CBanEntry { int64_t bantime, enum banSource }

Right. That would also allow for versioning in serialization.

This might also be capable of allowing ban distinction between multiple bans on the same IP but with different ports

This might be overdoing it; at least I don’t see much added value in per-port bans, at the least they’re easily circumvented (for incoming connections: just reconnect and you get a different source port, for outgoing connections simply rebind to a different port).

Diapolo commented at 3:27 pm on June 26, 2015: none

@laanwj Think about per port bans with incoming Tor/proxy connections, they’re all 127.0.0.1 and when I currently ban a single 127.0.0.1 with a specific port all further connection attempts are blocked until bantime is over. Not sure if this is to solve in any way.

laanwj commented at 3:33 pm on June 26, 2015: member

@Diapolo That simply won’t work. As I try to explain above, source ports aren’t unique nor identifying.

Diapolo commented at 3:55 pm on June 26, 2015: none

@laanwj Indeed, my fault… but should be warned when banning 127.0.0.1 or do you assume people know what they are banning?

jonasschnelli force-pushed on Jun 26, 2015

jonasschnelli commented at 7:58 pm on June 26, 2015: contributor

Followed and implemented @laanwj s proposal. Added CBanEntry as ban metadata container which uses enum BanReason (BanReasonNodeMisbehaving, BanReasonManuallyAdded). Also added nCreateTime within the new metadata class, to allow to backtrack when a ban was added/created. Using now everywhere the typedef std::map<CSubNet, CBanEntry> banmap_t.

Now the banlist.dat file is extendable over the possibility to detect different versions (CBanEntry->nVersion) of entries and allow possible migrations during deserialization.

jonasschnelli force-pushed on Jun 26, 2015

in src/rpcnet.cpp: in 0386ba4b62 outdated

552     {
553+        CBanEntry banEntry = (*it).second;
554         UniValue rec(UniValue::VOBJ);
555         rec.push_back(Pair("address", (*it).first.ToString()));
556-        rec.push_back(Pair("banned_untill", (*it).second));
557+        rec.push_back(Pair("banned_untill", banEntry.nBanUntil));

laanwj commented at 2:49 pm on June 29, 2015:

s/untill/until/ I suppose?

jonasschnelli commented at 6:04 pm on June 29, 2015:

s/untill/until/ I suppose?

Fixed typo.

laanwj commented at 2:50 pm on June 29, 2015: member

utACK

in src/net.cpp: in 0386ba4b62 outdated

490     return fResult;
491 }
492 
493-void CNode::Ban(const CNetAddr& addr, int64_t bantimeoffset, bool sinceUnixEpoch) {
494+void CNode::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
495     CSubNet subNet(addr.ToString()+(addr.IsIPv4() ? "/32" : "/128"));

laanwj commented at 2:53 pm on June 29, 2015:

Hhmm at some point we should add a CSubNet constructor or factory function that makes a subnet with one unique CNetAddr (netmask all ones), and use that, instead of going through a string. Not a big deal, though.

jonasschnelli commented at 6:40 pm on June 29, 2015:

Hhmm at some point we should add a CSubNet constructor or factory function that makes a subnet with one unique CNetAddr (netmask all ones), and use that, instead of going through a string. Not a big deal, though.

Right. Added (see latest commit). If we don’t add it now, it probably will never be added.

in src/net.cpp: in 0386ba4b62 outdated

503+    CBanEntry banEntry(GetTime());
504+    banEntry.banReason = banReason;
505+    banEntry.nBanUntil = GetTime()+GetArg("-bantime", 60*60*24);  // Default 24-hour ban
506     if (bantimeoffset > 0)
507-        banTime = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;
508+        banEntry.nBanUntil = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;

laanwj commented at 3:04 pm on June 29, 2015:

maybe

0if (bantimeoffset <= 0)
1{
2    bantimeoffset = GetArg("-bantime", 60*60*24); // Default 24-hour ban
3    sinceUnixEpoch = false;
4}
5banEntry.nBanUntil = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;

(slightly cleaner, not calling GetTime() twice)

jonasschnelli commented at 6:05 pm on June 29, 2015:

Yes. This makes more sense. Fixed and pushed.

jonasschnelli force-pushed on Jun 29, 2015

ajweiss commented at 6:33 pm on June 29, 2015: contributor

I have an unsubmitted patch that also adds “remembering” of ban scores associated by IP address across disconnects over a time window (24h I think). That is, it prevents malicious nodes from carrying out sawtooth attacks where they can reset their ban score by disconnecting and reconnecting before being banned. There are pros (prevents malicious behavior) and cons (tightens up the rules and increases the likelihood of NAT exit hosts getting banned due to lots of small accumulations of ban points over time.) Is there any interest in this? I can open a new pull if so…

jonasschnelli force-pushed on Jun 29, 2015

laanwj commented at 6:17 pm on July 2, 2015: member

@ajweiss Not sure about that. At least make sure that the associated structure is size-limited. Especially with IPv6 it will be extremely easy to fill memory that way. But this needs to be discussed somewhere else as it is not related to this pull.

banlist.dat: store banlist on disk f581d3d656

CAddrDB/CBanDB: change filesize variables from int to uint64_t dfa174c295

use CBanEntry as object container for banned nodes

- added a reason enum for a ban
- added creation time for a ban

Using CBanEntry as container will keep banlist.dat extenable.

409bccfbf5

in src/netbase.cpp: in 79f375fc05 outdated

1299+
1300+    //set all netmask bits to 1
1301+    memset(netmask, 255, sizeof(netmask));
1302+
1303+    std::vector<CNetAddr> vIP;
1304+    if (!LookupHost(addr.ToString().c_str(), vIP, 1, fAllowLookup))

laanwj commented at 6:22 pm on July 2, 2015:

Thanks. But this seems a bit circuitous. Let’s do just:

0CSubNet::CSubNet(const CNetAddr &addr, bool fAllowLookup):
1    valid(addr.IsValid())
2{
3    memset(netmask, 255, sizeof(netmask));
4    network = addr;
5}

Also the fAllowLookup parameter is unnecessary. It should never be needed to look up anything when going from an already binary address.

Edit: added initialization for valid()

jonasschnelli commented at 6:38 pm on July 2, 2015:

Right. Wasn’t aware of the LookupHost() in the CNetAddr::CNetAddr() constructor. Fixed and pushed.

jonasschnelli commented at 6:44 pm on July 2, 2015:

Right! The isValid must be set explicit. Fixed.

jonasschnelli force-pushed on Jul 2, 2015

Adding CSubNet constructor over a single CNetAddr 177a0e4914

jonasschnelli force-pushed on Jul 2, 2015

laanwj merged this on Jul 2, 2015

laanwj closed this on Jul 2, 2015

laanwj referenced this in commit 66e5465773 on Jul 2, 2015

zkbot referenced this in commit d0b67191e2 on Apr 5, 2018

zkbot referenced this in commit bf0f1cbee9 on Mar 6, 2020

zkbot referenced this in commit 2d9a9aaa83 on Nov 11, 2020

zkbot referenced this in commit f40121446d on Nov 12, 2020

zkbot referenced this in commit 049951dc45 on Feb 11, 2021

zkbot referenced this in commit b3a6729944 on Feb 16, 2021

zkbot referenced this in commit e85265fbd5 on Feb 17, 2021

zkbot referenced this in commit b4b07a1bbd on Feb 17, 2021

DrahtBot locked this on Sep 8, 2021

banlist.dat: store banlist on disk #6310