waku-org / nwaku

Waku node and protocol.
Other
199 stars 51 forks source link

chore: Address more attack vectors in rate limiting non-relay protocols. #2589

Closed NagyZoltanPeter closed 3 months ago

NagyZoltanPeter commented 5 months ago

This issue is addressing some findings from @alrevuelta about some DOS attack vectors left open in the initial PR of rate limiting non-relay protocols.

Some comments:

  1. We are missing some DoS attack vectors. For example this allows an attacker to DoS the node, since it will naively read from the connection and return. We should also count read bytes, not just requests made. And not only on valid requests but also when RPC decode is not successful. This can be used for inspiration. I would suggest something similar, where if a given peer has sent more than x bytes/unit of time, readLp is not called + we disconnect from that peer without reading from the connection. In short: This code allows an attacker to flood with random bytes the peer without any limits.
let buf = await conn.readLp(MaxRpcSize.int)

let decodeRes = HistoryRPC.decode(buf)
if decodeRes.isErr():
  error "failed to decode rpc", peerId = $conn.peerId
  waku_store_errors.inc(labelValues = [decodeRpcFailure])
  # TODO: Return (BAD_REQUEST, cause: "decode rpc failed")
  return
  1. Unless I'm missing something, the rate limit is applied globally (eg to all peers), which means that a given peer can trigger this rate limit and won't allow other nodes to get any service from the node. imho the rate limit should be per peerId (or per IP address) so that we allow x amount of usage (bytes or requests) per peer instead of globally. Without this, we are still exposed to DoS attacks.
NagyZoltanPeter commented 3 months ago

These tasks are done by the linked PRs