AICH: rate-limit OP_AICHREQUEST + O(1) LoadHashSet via offset cache by got3nks · Pull Request #186 · amule-org/amule

got3nks · 2026-06-17T09:39:30Z

Summary

Addresses both mitigations identified by @danim7 in #166 (analysis).

Two independent commits — they can be reviewed and bisected separately.

Commit 1 — rate-limit OP_AICHREQUEST

ClientTCPSocket.cpp:1548 was the only file-request-shaped opcode in the TCP dispatch that bypassed CheckForAggressive(). A hostile peer could send 16-byte OP_AICHREQUEST packets at it and each one chained through ProcessAICHRequest → CreatePartRecoveryData → LoadHashSet, forcing an O(N) walk of known2.met per packet. On a busy seeder (~120 MiB known2.met per TiB shared) the amplification is real.

Match the OP_STARTUPLOADREQ pattern at :539: tick the per-client request counter and bail if Ban() already fired this round.

The other ungated AICH opcodes don't have the same disk-amplification shape (OP_AICHANSWER is inbound response data; OP_AICHFILEHASHREQ only does an in-memory GetFileByID lookup) and stay as-is.

Commit 2 — O(1) LoadHashSet via offset cache

s_rootHashCache already existed for dedup-on-write (introduced in #579) as unordered_set<CAICHHash>. Extend it to unordered_map<CAICHHash, uint64> where the value is the byte offset of the entry's root-hash position in known2.met.

LoadRootHashCacheLocked records file.GetPosition() before reading each rootHash during the one-shot walk.
SaveHashSet records the offset of the new entry's root hash immediately after the append succeeds.
LoadHashSet looks up the cached offset under the existing mutex, seeks straight there, and validates — O(1) on a hit. On a cold cache it pays the one-shot LoadRootHashCacheLocked walk once and is O(1) for every subsequent call.

Two follow-on wins fall out:

When the cache is loaded and the requested root hash isn't present, LoadHashSet returns false immediately without opening known2.met — the cache is authoritative once warm.
The dedup-on-write fast path in SaveHashSet keeps its O(1) behaviour unchanged (set vs. map lookup cost is identical).

Memory cost: ~8 bytes per known2.met entry beyond the existing set. A 1 TiB library at ~5500 files averages ~44 KiB extra RAM. Negligible.

Defensive fallback: if the cached offset's hash doesn't match (stale cache from external known2.met mutation), LoadHashSet falls through to the existing linear-scan loop from the post-header position. False negatives are non-fatal — the caller sends an empty AICH answer and the peer re-requests.

Test plan

CI build matrix green (9/9: Ubuntu Debug+Release, macOS Debug+Release, mingw-w64 Debug+Release, App catalogs, Manpage catalogs, Translation checks).
Configure + build clean on macOS arm64 (Release and Debug, the latter to exercise the __DEBUG__-gated log paths).
End-to-end verification harness drives both code paths against an isolated patched amuled (no production touch). Setup boots a throwaway daemon under /tmp/amule-aich-test/ with one shared 500 KB test file and enables the ED2k Client / Remote Client Protocol / AICH-Transfer / SHAHashSet debug categories so the dispatch and ban paths are observable in the logfile.

Test 1 — offset cache returns valid AICH recovery data. Send a legitimate OP_AICHREQUEST referencing the test file's MD4 (computed in-script via hashlib.md4) and AICH master hash (parsed from known2_64.met offset 1, the first entry's 20-byte root). Receive OP_AICHANSWER with 108 bytes of recovery payload — proving LoadHashSet resolved the cached offset, seeked to the right entry, and emitted correct output.

Test 2 — flood triggers Ban() at exactly the expected request. Restart amuled (so the per-IP client-tracker state from test 1 doesn't pre-load m_LastFileRequest), then send 8 rapid OP_AICHREQUEST packets with 300 ms inter-send delay. Result mirrors CheckForAggressive's math exactly: requests 1-4 receive a 108-byte answer (score 0→3→6→9, all below the threshold of 10); request 5 silenced because the in-handler Ban() fires at score 12; requests 6-8 hit the new IsBanned() short-circuit before ProcessAICHRequest. The amuled debug log confirms with "Aggressive client banned (score: 12): amule-bench".
Harness output and log excerpt posted as a PR comment for visibility.

@danim7

The OP_AICHREQUEST handler was the only file-request-shaped op in the TCP dispatch that didn't call CheckForAggressive(). A hostile peer can hammer 16-byte requests at it and each one chains through ProcessAICHRequest -> CreatePartRecoveryData -> LoadHashSet, which historically walks known2.met linearly. On big seeders (~120 MiB of known2.met per TiB shared) the per-request cost is real. Match the OP_STARTUPLOADREQ pattern at ClientTCPSocket.cpp:539: tick the per-client request counter and bail if Ban() already fired this round. The other ungated AICH ops (OP_AICHANSWER inbound, OP_AICHFILEHASHREQ which only does an in-memory map lookup) don't have the same disk-amplification shape and stay as they were. Reported by @danim7 in issue amule-project#166. Refs: amule-project#166

Each incoming OP_AICHREQUEST on the seeder triggers ProcessAICHRequest -> CreatePartRecoveryData -> LoadHashSet, which historically walked known2.met from the start until it found a matching root hash -- O(N) per request, where N is the number of files in the seeder's library. On a busy seeder with a 1 TiB shared set (~120 MiB known2.met) every legitimate AICH response paid for a full sequential scan. The s_rootHashCache already existed for dedup-on-write (issue amule-project#579), storing every root hash in an unordered_set. Extend it from a set into an unordered_map<CAICHHash, uint64> where the value is the byte offset of the entry's root-hash position in known2.met: - LoadRootHashCacheLocked records file.GetPosition() before reading each rootHash during the one-shot walk. - SaveHashSet records the offset of the new entry's root hash immediately after the append succeeds. - LoadHashSet looks up the cached offset under the existing mutex, seeks straight there, and validates -- O(1) on a hit. On a cold cache it pays the one-shot LoadRootHashCacheLocked walk once and then is O(1) for every subsequent call. Two follow-on wins fall out of the same change: - When the cache is loaded and the requested root hash isn't present, LoadHashSet returns false immediately without opening known2.met -- the cache is authoritative once warm. - The dedup-on-write fast path in SaveHashSet keeps its O(1) behaviour unchanged (set vs. map lookup cost is identical). Memory cost: 8 extra bytes per known2.met entry (uint64 offset instead of unordered_set node payload). Negligible -- a 1 TiB library at ~5500 files/TiB averages ~44 KiB of extra RAM. Defensive fallback: if the cached offset's hash doesn't match (stale cache -- e.g. known2.met was externally mutated), LoadHashSet silently falls through to the existing linear-scan loop from the post-header position. False negatives are non-fatal: the caller sends an empty AICH answer and the peer re-requests. Refs: amule-project#166

got3nks · 2026-06-17T10:56:47Z

Ran the verification harness end-to-end against a debug-build patched amuled on macOS arm64. Both tests pass; sharing the output for the record.

Harness run

[setup] test root: /tmp/amule-aich-test
[setup] test file MD4 = cb3d05e74f4af73e897c02264dbbda3c
[setup] starting amuled with config-dir=/tmp/amule-aich-test/config
[setup] amuled accepting connections on port 14662
[setup] AICH master = 7689b0e4f62c6a571fdbbc50fdf4043f912bed41

[test 1] AICH still works after the offset-cache refactor
    -> OP_AICHREQUEST (md4=cb3d05e74f4af73e..., part=0)
    <- OP_AICHANSWER  payload=108 bytes  ✓
    PASS: AICH recovery path produces non-empty output

[setup] restarting amuled between tests for clean per-IP state

[test 2] OP_AICHREQUEST flood -- find the ban point
    req #1: ANSWER  [proto=0xc5/op=0x9c/108B]
    req #2: ANSWER  [proto=0xc5/op=0x9c/108B]
    req #3: ANSWER  [proto=0xc5/op=0x9c/108B]
    req #4: ANSWER  [proto=0xc5/op=0x9c/108B]
    req #5: silence
    req #6: silence
    req #7: silence
    req #8: silence
    answered 4/8 -- first silence at req #5
    logfile contains 'aggressive client banned' line  ✓
    PASS: rate-limit fires (ban point = req #5)

[result] ALL TESTS PASSED ✓

amuled debug log (excerpt — flood test)

12:53:01 OP_HELLO from 127.0.0.1
12:53:01 OP_AICHREQUEST from 127.0.0.1
12:53:01 AICH-Transfer: AICH Packet Request: Successfully created and send recoverydata for 'verify-pr186.bin' to Client amule-bench  ← req #1
12:53:02 OP_AICHREQUEST from 127.0.0.1
12:53:02 AICH-Transfer: Successfully created and send recoverydata                                                                       ← req #2
12:53:02 OP_AICHREQUEST from 127.0.0.1
12:53:02 AICH-Transfer: Successfully created and send recoverydata                                                                       ← req #3
12:53:02 OP_AICHREQUEST from 127.0.0.1
12:53:02 AICH-Transfer: Successfully created and send recoverydata                                                                       ← req #4
12:53:02 OP_AICHREQUEST from 127.0.0.1
12:53:02 ED2k Client: Aggressive client banned (score: 12): amule-bench --  -- eDonkey v26.62                                            ← req #5 BAN
12:53:02 ED2k Client: Client 'amule-bench' seems to be an aggressive client and is banned from the uploadqueue
12:53:05 OP_AICHREQUEST from 127.0.0.1     (no "Successfully created" follow-up)                                                         ← req #6 short-circuit
12:53:07 OP_AICHREQUEST from 127.0.0.1                                                                                                    ← req #7
12:53:09 OP_AICHREQUEST from 127.0.0.1                                                                                                    ← req #8

The behavior matches CheckForAggressive's math precisely (+3 per aggressive call after init, Ban() at score ≥ 10 → fires on call #5). For requests 6-8 the dispatch entry log line still fires (proving the packet reached the OP_AICHREQUEST case) but the "Successfully created" line never follows — proving the new IsBanned() check correctly short-circuits before ProcessAICHRequest does any disk work. The DoS amplification ceiling is 4 disk-walks per malicious connection.

danim7 · 2026-06-17T12:10:16Z

Thanks @got3nks for your prompt reply to implement these mitigations.

A few points worth mentioning:

In your commit, file SHAHashSet.cpp, line 849, you Seek to the offset position and you check the hash at line 857. You say that we fall through to the linear scan from the start as defensive recovery, but I got the impression we would actually scan from the offset position, not from the start. May you please check?
For the OP_AICHREQUEST: when it is our client the one asking for that info, how do we choose the peer to ask? Random peer? First we find? Although it is unlikely to require 4 AICH Hash sets in a row and get ourselves banned (even less since other interactions also increase m_Aggressiveness), if we have multiple peer candidates, we may ask the least-recently contacted one to avoid that.
In https://github.com/amule-org/amule/blob/master/src/ClientTCPSocket.cpp#L539 we call CheckForAggresive() and then we check if the client IsBanned(). In https://github.com/amule-org/amule/blob/master/src/ClientUDPSocket.cpp#L194 we only call CheckForAggresive()...is this intentional? or are we missing the IsBanned() call?

got3nks · 2026-06-17T12:27:59Z

Thanks @danim7 — all three points are sharp.

You're right about the fallback behavior. The merged code seeks to cachedOffset and the loop continues from there; on a mismatch we'd skip nHashCount * HASHSIZE bytes forward and keep reading, not restart from the file start. The comment is wrong. In the normal lifecycle the cached offset is always correct (populated either by the full walk in LoadRootHashCacheLocked or by SaveHashSet's captured write offset), so the bug is latent — it would only surface if known2.met were modified externally between cache load and request. Opening a small follow-up PR to add a true defensive rewind to 0 on first-read mismatch (and fix the comment).
Peer rotation on the send side. Audited CPartFile::RequestAICHRecovery — current behavior is uniform random selection across eligible peers (high-ID preferred, low-ID fallback), with IsAICHReqPending() preventing concurrent same-peer requests. Worth noting AICH itself is gated upstream: the master hash has to reach AICH_TRUSTED status, which requires ≥10 unique IPs sending the same hash with ≥92% agreement (SHAHashSet.cpp:45-46). So by the time AICH is callable, the source pool is already large by definition, and random selection scatters requests across it. The self-ban scenario would need that pool to collapse to 1 eligible peer AND multiple consecutive part-corruption events — degenerate enough that the current random policy seems to already handle it in practice. Not worth a fix unless the self-ban actually shows up in the wild.
UDP missing IsBanned() check. Verified — ClientUDPSocket.cpp:194 calls CheckForAggressive() but doesn't short-circuit on IsBanned(), unlike its TCP counterpart at ClientTCPSocket.cpp:539. Looks like an oversight from when the aggressive check was added there. Small follow-up PR.

Will open follow-ups for 1 and 3.

@danim7

… stale PR #186 added an offset cache so LoadHashSet seeks straight to the matching entry instead of linearly walking known2.met. The previous comment claimed that on a first-read mismatch we'd "fall through to the linear scan from the start as defensive recovery" — but the loop actually keeps reading from the cached offset onward, treating whatever bytes are at that position as a root hash + hashCount. If the cache is stale (known2.met modified externally between cache load and the request) the offset can land in the middle of a hash blob; we'd misread CurrentHash, misread nHashCount, and the loop would misalign and ultimately return false even though the entry still exists somewhere in the file. Reported by @danim7 in #186. Add a real defensive rewind: on the first iteration after seeking to the cached offset, if CurrentHash doesn't match our root hash, treat the cache as stale, jump back to just past the version header (byte 1), and continue with a true linear scan from the top. Cap the rewind to one attempt with cacheFallbackTriggered so we can't loop. Also handle the corner case where the cached offset is past EOF (file was truncated externally): skip the seek entirely and fall straight through to the linear scan. In the happy path (cache offset correct), behavior is unchanged: the first read matches and we return true after the existing match handler.

@danim7

ClientUDPSocket.cpp:194 calls CheckForAggressive() on incoming file-info packets but doesn't check IsBanned() after, unlike the TCP file-request path at ClientTCPSocket.cpp:539 (and the TCP OP_AICHREQUEST path added in #186). CheckForAggressive can call Ban() when m_Aggressiveness >= 10, but the UDP handler then falls through and keeps processing the request anyway -- AddAskedCount(), SetUDPPort(), SetLastUpRequest(), ProcessExtendedInfo(), etc. A freshly-banned client could keep the seeder doing work on each UDP file-info packet it sends. Small leak, same shape as the TCP gap that #186 closed. Mirror the TCP guard: break out of the dispatch case if IsBanned() returns true. Reported by @danim7 in #186 (comment) (point 3).

got3nks · 2026-06-17T13:27:24Z

Both follow-ups are now merged: #191 (defensive rewind in LoadHashSet when the cached offset is stale) and #192 (UDP IsBanned() short-circuit). Linking back here for traceability.

got3nks added 2 commits June 17, 2026 11:37

got3nks mentioned this pull request Jun 17, 2026

feat: Hash parts on upload to detect silent bit-rot #166

Open

got3nks merged commit ce5c787 into amule-org:master Jun 17, 2026
10 checks passed

got3nks deleted the fix/aich-rate-limit-offset-cache branch June 17, 2026 12:02

This was referenced Jun 17, 2026

fix(aich): true linear-scan fallback when LoadHashSet cache offset is stale #191

Merged

fix(udp): short-circuit UDP file-info handler when client is banned #192

Merged

Navigation Menu

Uh oh!

AICH: rate-limit OP_AICHREQUEST + O(1) LoadHashSet via offset cache - #186

AICH: rate-limit OP_AICHREQUEST + O(1) LoadHashSet via offset cache#186
got3nks merged 2 commits into
amule-org:masterfrom
got3nks:fix/aich-rate-limit-offset-cache

got3nks commented Jun 17, 2026 •

edited

Loading

Uh oh!

got3nks commented Jun 17, 2026

Uh oh!

Uh oh!

danim7 commented Jun 17, 2026

Uh oh!

got3nks commented Jun 17, 2026

Uh oh!

got3nks commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

got3nks commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Commit 1 — rate-limit OP_AICHREQUEST

Commit 2 — O(1) LoadHashSet via offset cache

Test plan

Uh oh!

got3nks commented Jun 17, 2026

Harness run

amuled debug log (excerpt — flood test)

Uh oh!

Uh oh!

danim7 commented Jun 17, 2026

Uh oh!

got3nks commented Jun 17, 2026

Uh oh!

got3nks commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

got3nks commented Jun 17, 2026 •

edited

Loading