Fix ADNL inbound peer limiter poisoning - #2483
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95b271271e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| td::actor::send_closure(actor_id(this), &AdnlPeerPairImpl::send_messages_from_queue); | ||
| } | ||
| } | ||
| td::actor::send_closure(local_actor_, &AdnlLocalId::add_inbound_peer, packet.remote_addr(), packet.from_short()); |
There was a problem hiding this comment.
When normal traffic is sent over an ADNL channel, the sender does not populate from_short (set_source is only called in the !via_channel path), and AdnlChannelImpl::decrypt explicitly permits packets without a source. This new call therefore usually records the default/empty node id instead of the authenticated channel peer, which consumes one slot in recent_inbound_peers; after any channel traffic from an IP, the limiter can admit only 59 real peer IDs and may reject the 60th legitimate peer behind the same IP. Use the peer pair's known peer id for channel packets, or require/populate a source before recording it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6789902. Channel packets now register the peer pair known peer_id_short_ instead of packet.from_short(), which may be unset for channel traffic. test-adnl passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678990284f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| td::actor::send_closure(peer_table_, &AdnlPeerTable::receive_decrypted_packet, short_id_, std::move(packet), |
There was a problem hiding this comment.
When the set is below UNIQUE_PEERS_PER_IP_LIMIT, this now forwards every decrypted packet without recording its source until after AdnlPeerPairImpl::receive_packet verifies the signature. However AdnlPeerTableImpl::receive_decrypted_packet creates PeerInfo/a peer pair for an unknown full from before that signature check, so one IP can stream packets with fresh full source keys and bad signatures; none are added to recent_inbound_peers, the unique-peer limiter never fills, and the peer table still allocates actors until idle GC. Please keep some pre-auth admission record for unknown full sources or move authentication ahead of peer creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 148d431. Unknown full-source packets are now signature-checked before PeerInfo/AdnlPeerPair allocation, so forged sources cannot create actors before authentication. test-adnl passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 148d431d4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| td::actor::send_closure(local_actor_, &AdnlLocalId::add_inbound_peer, packet.remote_addr(), packet.from_short()); |
There was a problem hiding this comment.
When the set is just below UNIQUE_PEERS_PER_IP_LIMIT and multiple authenticated non-channel peers from the same IP arrive in a burst, this send_closure only queues the accounting back to the local actor and receive_packet_checked delivers the packet immediately; all packets that passed the earlier receive_coro check before these callbacks run can be accepted, and later add_inbound_peer just drops the excess once the set is full. This regresses the old synchronous cap, so reserve/record the peer, or ask the local actor for admission, before delivering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, this is still outstanding. The limiter update is currently asynchronous, so a burst can pass the pre-check before the accounting callbacks run. I’ll add admission reservation before packet delivery.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0b0a680f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| void AdnlLocalId::add_inbound_peer(td::IPAddress addr, AdnlNodeIdShort peer, td::Promise<td::Unit> promise) { | ||
| auto &rate_limiter = inbound_rate_limiter_[remove_port(addr)]; |
There was a problem hiding this comment.
When add_inbound_peer is first reached from the channel path for an address that has never gone through receive_coro, this operator[] creates a new inbound_rate_limiter_ entry, but this path only arms cleanup_recent_inbound_peers_at_; cleanup_rate_limiter_at_ is armed only in receive_coro, and alarm() only erases empty/full map entries from that cleanup branch. After the 60-second peer set is cleared, the per-IP entry therefore remains indefinitely, so a peer with a valid channel can send packets from many source IPs and leak limiter entries. Arm the rate-limiter cleanup here too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 49b2a3d. add_inbound_peer now arms cleanup_rate_limiter_at_, including for channel traffic that bypasses receive_coro. Channel-created entries are therefore removed after their peer set is cleared and the limiter becomes eligible for cleanup. test-adnl passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49b2a3dd21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auto S = R.move_as_ok()->check_signature(packet.to_sign().as_slice(), packet.signature().as_slice()); | ||
| if (S.is_error()) { | ||
| VLOG(adnl, INFO) << this << ": dropping IN message [" << packet.from_short() << "->" << dst | ||
| << "]: bad signature: " << S; | ||
| return; |
There was a problem hiding this comment.
When an IP sends a burst of fresh full-source packets while recent_inbound_peers is still below 60, this new pre-auth verifies each signature but does not reserve the peer before the packet falls through to peers_.try_emplace and get_peer_pair; the cap is only enforced later from AdnlPeerPairImpl::receive_packet via add_inbound_peer (adnl-peer.cpp lines 278-286). Fresh evidence after the latest fix is that valid signed keys still allocate PeerInfo/peer-pair actors before admission, so an attacker that controls arbitrary keypairs can create many actors in one burst even though most packets are dropped after allocation. Please perform the unique-peer admission/reservation before creating the peer entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2216a32. When an inbound packet would require creating either a new PeerInfo or a peer-pair for the destination local ID, its signature is verified and the peer is admitted by AdnlLocalId before peers_.try_emplace or get_peer_pair is reached. Rejected burst peers therefore no longer allocate actors. test-adnl passes.
2 participants
Summary
The inbound limiter previously recorded
packet.from_short()before signature verification. An attacker could therefore fill the 60-peer set for an IP with forged IDs and block legitimate peers. This change moves the set update after authentication while retaining the existing admission check.Fixes #2373