Reject RPC requests with trailing bytes

[ackintosh]

Issue Addressed

#9301

Proposed Changes

Previously, trailing bytes after an inbound RPC request payload were silently ignored and the request was processed as normal. This violates the consensus spec, which requires such input to be rejected with an InvalidRequest error
response.

This PR makes the inbound codec detect trailing bytes and respond to the peer with an InvalidRequest error instead of just closing the stream.

I've verified the fix using the reproduction script from #9301. With this PR, Lighthouse now rejects trailing bytes as expected:

$ go run . --config clients.yaml
[*] Trailing Bytes Spec Violation PoC
[*] Spec: phase0/p2p-interface.md
[*] "A reader MUST consider [any remaining bytes after n SSZ bytes] as invalid input"
[*] Expected: INVALID_REQUEST.
DEBUG peerInfo: ID=16Uiu2HAmAQpT8ZAoWdTN2d2Ju36x2cSuDH6zBrmmAHDGMtiVAyJW Addrs=[/ip4/127.0.0.1/tcp/60589]
  prysm        SPEC VIOLATION — accepts trailing bytes
DEBUG peerInfo: ID=16Uiu2HAmTkd6q26Et4zKD19kebN789Zh78bsa3MLzCNEq1aaRtdS Addrs=[/ip4/127.0.0.1/tcp/60591]
  lighthouse   COMPLIANT — rejects trailing bytes

Additional Info

As noted in the code comment, the trailing-bytes check is best-effort: it only catches trailing bytes that arrive in the same read as the request payload. Bytes arriving in a later read go undetected, because the request stream decodes a single frame and is not polled again.

Detecting those would require an extra read purely to probe for EOF, which adds a network round-trip for little benefit. When trailing bytes go undetected, the behavior simply falls back to the previous one (the request is processed normally), so nothing gets worse in that case.

[pawanjay176]

I'm not sure about this one. This is another case where being spec compliant just for the sake of it might not be the best idea and just additional code to maintain.

Since we ignore additional bytes, from an attacker's perspective, sending additional bytes is equivalent to sending the correct request, so this is not exploitable in any way afaiu. Correct me if this fixes any particular class of attacks.

[jxs]

Hi, Akihito, thanks for this!
Yeah I agree with Pawan though, I think we should probably make it explicit in the code as you did, but not respond to that otherwise it's as Pawan says and we are feeding invalid requests and just wasting bandwidth

[ackintosh]

I agree with both Pawan and João. I don’t think this change prevents any particular class of attacks.

The only reason for this change is to comply with the consensus spec, which explicitly says that implementations MUST reject requests with trailing bytes. If the spec did not use a MUST here, I would also consider this change unnecessary.

Perhaps the right fix is to propose a change to the consensus spec, if we believe rejecting these requests isn't necessary.