net: limit relative ban times to uint32 #36366

Bruce039 commented at 2:44 PM on September 28, 2026: none

BanMan::Ban computes the ban end as GetTime() + bantime (banman.cpp:140). setban and -bantime both accept any int64, so a relative bantime close to INT64_MAX:

setban 1.2.3.5 add 9223372036854775807

returns success, but the sum overflows. UBSan reports:

src/banman.cpp:140:73: runtime error: signed integer overflow: 1790412558 + 9223372036854775807 cannot be represented in type 'int64_t'

Without UBSan the result wraps negative, the ban is treated as already expired, and it is silently dropped. With -bantime=9223372036854775807 the same happens to every setban add that uses the default duration.

This limits relative ban times to uint32 (about 136 years):

setban rejects a relative bantime above UINT32_MAX with "Error: bantime is too large"
-bantime is parsed as uint32_t, so larger values saturate at UINT32_MAX

Absolute timestamps are unchanged.

The added cases in rpc_setban.py check that UINT32_MAX + 1 and INT64_MAX are rejected, that UINT32_MAX is accepted, and that with -bantime=INT64_MAX a default-duration ban is listed with a duration of UINT32_MAX. Both parts fail on master and pass with the change. p2p_disconnect_ban.py still passes.

AI tools were used to help find this issue and to prepare the patch and test.

DrahtBot commented at 2:44 PM on September 28, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36366
Ralph review: https://git.fish.foo/bitcoin/bitcoin/pulls/36366#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Stale ACK	maflcko

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

maflcko commented at 3:08 PM on September 28, 2026: member

I am not sure about starting/trying to support durations that C++ itself doesn't even support (https://en.cppreference.com/cpp/chrono/duration)

I think it would be fine to reject args that are too large. Though, argparsing doesn't really do any verification and I am sure you can find an int overflow for basically every single arg. Also, SaturatingAdd doesn't work for chrono types ...

Bruce039 force-pushed on Sep 28, 2026

Bruce039 renamed this:
~~banman: avoid overflow for large relative ban times~~
rpc: reject setban bantime that would overflow
on Sep 28, 2026

DrahtBot added the label RPC/REST/ZMQ on Sep 28, 2026

Bruce039 commented at 5:06 PM on September 28, 2026: none

Makes sense, switched to rejecting it: setban now fails with "Error: bantime is too large" when a relative bantime wouldn't fit once added to the current time, next to the existing check for absolute timestamps in the past. The SaturatingAdd in BanMan::Ban is gone and -bantime is left as is.

in src/rpc/net.cpp:865 in fcb3446ccf

 860 | @@ -860,6 +861,11 @@ static RPCMethod setban()
 861 |              throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Absolute timestamp is in the past");
 862 |          }
 863 |  
 864 | +        // The ban end time is computed as now + bantime, which must not overflow
 865 | +        if (!absolute && banTime > std::numeric_limits<int64_t>::max() - GetTime()) {

maflcko commented at 6:32 AM on September 29, 2026:

        if (!absolute && banTime > std::numeric_limits<uint32_t>::max()) {

This should be more than enough and also avoid the weird time dependency.

and -bantime is left as is.

Not sure. I'd say to either fix this fully or not at all. What about parsing it as u32?

Bruce039 commented at 7:31 AM on September 29, 2026:

Done, took the suggestion: setban now rejects a relative bantime above UINT32_MAX, and -bantime is parsed as uint32_t so larger values saturate there instead of overflowing later. The test covers both.

Bruce039 force-pushed on Sep 29, 2026

Bruce039 renamed this:
~~rpc: reject setban bantime that would overflow~~
net: limit relative ban times to uint32
on Sep 29, 2026

maflcko commented at 8:01 AM on September 29, 2026: member

lgtm ACK d49c1c9ab3e139158d76141b71943ff79903e87a

Seems fine to reject or clamp values larger than any meaningful use-case.

in src/init.cpp:1710 in d49c1c9ab3

1706 | @@ -1707,7 +1707,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1707 |  
1708 |      FastRandomContext rng;
1709 |      assert(!node.banman);
1710 | -    node.banman = std::make_unique<BanMan>(args.GetDataDirNet() / "banlist", &uiInterface, args.GetIntArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
1711 | +    node.banman = std::make_unique<BanMan>(args.GetDataDirNet() / "banlist", &uiInterface, args.GetArg<uint32_t>("-bantime", DEFAULT_MISBEHAVING_BANTIME));

willcl-ark commented at 9:26 AM on October 2, 2026:

In d49c1c9ab3e139158d76141b71943ff79903e87a

This covers startup args correctly but I think settings.json can also have a value > uint32 which will throw during conversion.

Not sure if that's common enough (or at all) to worry about though.

Bruce039 commented at 3:01 PM on October 2, 2026:

Good catch, thanks. A bantime above uint32 in settings.json made startup fail with "JSON integer out of range". -bantime is now read as int64 again and capped at UINT32_MAX, so it no longer matters where the value comes from. Added a settings.json case to rpc_setban.py, which fails on the previous push. (Changes prepared with AI tools.)

net: limit relative ban times to uint32

BanMan::Ban computes the ban end as GetTime() + bantime. Both setban and
-bantime accept any int64, so a relative bantime close to INT64_MAX
overflows there (UBSan: signed integer overflow in banman.cpp). In
practice the result wraps negative, the ban is treated as expired, and
it is silently dropped.

Limit relative ban times to uint32 (about 136 years):
- setban rejects a relative bantime above UINT32_MAX
- -bantime is capped at UINT32_MAX. It is still read as int64, so a
  larger value in settings.json does not make startup fail.

514ff32e2f

Bruce039 force-pushed on Oct 2, 2026

DrahtBot added the label CI failed on Oct 9, 2026

DrahtBot removed the label CI failed on Oct 9, 2026