fix(rln): nullifierlog vulnerability #2855
Conversation
waku/waku_rln_relay/rln_relay.nim
Outdated
|
|
||
| var epochsToClear = rlnPeer.nullifierLog.keys().toSeq() | ||
| epochsToClear.sort(compareKeys) | ||
| epochsToClear = epochsToClear[0 ..< countToClear] |
There was a problem hiding this comment.
as it is, it should work fine now.
however I'm having second thought about cleaning the nullifierlog this way. it has some implicit knowledge that can be error prone.
perhaps a better way could be. which epoch are we at? x? well, remove everything in the table that is beyond x-epochGap.
There was a problem hiding this comment.
i like this idea, O(n) vs O(nlogn) not that it matters though because the nullifierLog will be small :)
waku/waku_rln_relay/rln_relay.nim
Outdated
| # dirty should never happen | ||
| return pubsub.ValidationResult.Reject | ||
| # insert the message to the log (never errors) | ||
| discard wakuRlnRelay.updateLog(msgProof.epoch, proofMetadataRes.get()) |
There was a problem hiding this comment.
the key here is that we shouldn't update the nullifierlog if the message is wrong (eg proof is invalid). otherwise anyone can "DoS" the nullifieerLog, forcing cleanups and making it useless.
waku/waku_rln_relay/rln_relay.nim
Outdated
| @@ -280,7 +280,7 @@ proc validateMessageAndUpdateLog*( | |||
| return MessageValidationResult.Invalid | |||
|
|
|||
| # insert the message to the log (never errors) | |||
| discard rlnPeer.updateLog(msgProof.epoch, proofMetadataRes.get()) | |||
| #discard rlnPeer.updateLog(msgProof.epoch, proofMetadataRes.get()) | |||
There was a problem hiding this comment.
this function is called even in invalid messages. see other comment.
There was a problem hiding this comment.
or maybe a better way use isValidMessage here and only updateLog if valid.
alrevuelta
changed the title
|
You can find the image built from this PR at Built from 8863572 |
* CHANGELOG.md add info for v0.30.0 * fix(rln-relay): clear nullifier log only if length is over max epoch gap (#2836) * chore: add TWN parameters for RLNv2 (#2843) * fix(rln): nullifierlog vulnerability (#2855) * chore(rln-relay): add chain-id flag to wakunode and restrict usage if mismatches rpc provider (#2858) --------- Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com> Co-authored-by: Alvaro Revuelta <alvrevuelta@gmail.com>
chore: Update master from release v0.30 (#2866) * CHANGELOG.md add info for v0.30.0 * fix(rln-relay): clear nullifier log only if length is over max epoch gap (#2836) * chore: add TWN parameters for RLNv2 (#2843) * fix(rln): nullifierlog vulnerability (#2855) * chore(rln-relay): add chain-id flag to wakunode and restrict usage if mismatches rpc provider (#2858) --------- Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com> Co-authored-by: Alvaro Revuelta <alvrevuelta@gmail.com>
nullifierLog. Now instead of counting elements in the table and removing when >epochGap, we ensure that they correspond to the current epoch +-epochGap. Anything than that we remove. This protects the node from messages arriving out-of-order.validateMessageAndUpdateLog. ThenullifierLogtable was updated with new epochs no matter if the message was rate-limited or not, and no matter if the message even contained a valid proof. This allowed an attacker to attack thenullifierLogtable, that in conjunction with the previous bug caused rate-limited messages to be accepted. The attacker at no cost could fill the table with unused epochs, bypassing the rate-limit protection.