reject a malformed mutable put instead of storing it as immutable

[dangowrt]

The DHT put handler treats a put as mutable only when seq, k and sig are
all present and valid, and otherwise stores the value as an immutable item at
sha1(v). A put that carries a public key or signature but is missing (or
mistypes) one of the other mutable fields — a wrong-length or absent sig, a
missing or mistyped seq — is therefore stored as an immutable item rather than
rejected: the signature is never checked, and the item is not at sha1(pk+salt)
where a reader looks for it.

This rejects a put that carries a k or sig but is not a well-formed mutable
put, with protocol error 203; genuine immutable puts (neither field present) are
unaffected.

Keying the rejection on k/sig rather than on seq as well is deliberate: a
put claiming to be mutable always carries a 32-byte key or 64-byte signature, so
the error reply is always much smaller than the request and can't be used to
amplify traffic (the check also runs after the write-token verification). A put
carrying only a seq and no key or signature is small, so rather than answer it
with an error it is left as a plain immutable put.

Found with a black-box BEP 44 conformance suite; verified by rebuilding
libtorrent and running test/test_dht — the new put_malformed_mutable test
(k+seq without sig, k alone, and k+sig without seq all rejected with 203; a
correctly signed put still stored) and the full DHT suite pass.

[arvidn]

this should have a unit test, at least covering all the cases where a PUT is treated as an invalid PUT rather than an immutable PUT.

Another important consideration is that the response (error message) should be significantly smaller than the request. So if the incoming message is very short, we should probably not respond at all. That would open up for magnification attacks, since the source IP address can be spoofed.

[dangowrt]

Thanks — addressed both.

Unit test. Added TORRENT_TEST(put_malformed_mutable) in test/test_dht.cpp: it gets a write token for the mutable target, then sends puts that carry the public key but aren't a complete mutable put — k+seq with no sig, and k alone — and asserts each is answered with error 203 malformed mutable put rather than stored. It ends with a correctly-signed put using the same key to confirm the valid mutable path still returns r. test_dht passes in full here. The wrong-length k/sig variants reach the same branch (with k present), so they land on the same predicate.

Amplification. Good catch — narrowed it so the reply can't be a magnifier:

• The predicate now keys on k/sig being present (msg_keys[3] || msg_keys[4]) rather than any of seq/k/sig. A put claiming to be mutable always carries a 32-byte key or 64-byte signature, so the request is always larger than the ~40-byte error reply — response smaller than request, not an amplifier. The one small case, a put carrying only a seq and no key/signature, is now left as a plain immutable put rather than rejected.
• The check also sits after verify_token, so a spoofed source is already answered with invalid token and never reaches this branch unless it completed a get and proved its address.

Force-pushed with the narrowed predicate and the test.

[Copilot]

🟡 Not ready to approve

The new malformed-mutable guard and test coverage don’t fully align with the PR description’s stated rejection criteria for malformed puts (notably around seq-only / missing-seq scenarios).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR tightens BEP 44 DHT put handling so that requests that appear to be mutable puts (by including mutable fields) but are not well-formed are rejected with protocol error 203, instead of being accepted and stored as immutable items.

Changes:

• Add a DHT-node-side guard to reject malformed “mutable” put requests early with error 203 and a specific message.
• Add a unit test ensuring malformed mutable puts are rejected while a valid signed mutable put is still accepted.

File summaries

File	Description
src/kademlia/node.cpp	Adds a validation gate rejecting malformed mutable puts instead of falling back to immutable storage.
test/test_dht.cpp	Adds a regression test for malformed mutable puts and validates correct mutable puts still work.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[arvidn]

I prefer the previous version of this. My comment about magnification was a bit hasty. This is already after we check the write_token, so you can't abuse this part in a magnification attack.

And, in either case, such protection should probably be implemented at a higher level anyway, looking at packet sizes.

[dangowrt]

Re the automated review:

• The predicate keys on k/sig (not seq) on purpose: a seq-only put is small, and answering it with an error is exactly the amplification vector raised in the review above, so it's left as a plain immutable put instead. I've updated the PR description to match the code — it was stale from before the narrowing.
• Added the missing "k+sig present, seq absent" case to put_malformed_mutable. It's rejected as a malformed mutable put before the signature is ever checked, so the test reuses the valid signature for it. test/test_dht passes in full.

[arvidn]

a seq-only put is small

Not necessarily, it can be > 1kiB. I prefer to protect against reflection/magnification attacks separately