fix: don't use WakuMessageSize in req/resp protocols #2601
Conversation
chaitanyaprem
requested review from
SionoiS,
jm-clius,
gabrielmer,
NagyZoltanPeter and
Ivansete-status
|
You can find the image built from this PR at Built from 035f90e |
waku/waku_lightpush/protocol.nim
Outdated
| ): T = | ||
| let wl = WakuLightPush( | ||
| rng: rng, | ||
| peerManager: peerManager, | ||
| pushHandler: pushHandler, | ||
| requestRateLimiter: newTokenBucket(rateLimitSetting), | ||
| maxRPCSize: calculateRPCSize(maxMessageSize), |
There was a problem hiding this comment.
I think adding state and dynamic calculation here is overkill. The idea of the max stream read size was simply to have some reasonable safety limit that will prevent unrealistically (read: adversarially) large requests (which presumably could be an attack vector). We could have just used a very, very big magic number, but opted to initialise our constants with some value based off the MaxWakuMsgSize with safety margins to show that there was some rhyme and reason behind our thinking. Because this max msg size value is now configurable, our generous high limit may indeed now not be generous enough for differently configured nodes - at least in the lightpush case. However, even here the node could likely not configure message size to anything larger than the underlying libp2p limit (of 1MB). If we want to change this to be future proof, I'd rather keep using the underlying libp2p constant (1 MB) for all nodes and keep the simplicity of pure constants or just ignore the max rpc size limit (set to -1 all around). The latter could presumably expose node to more attacks, though.
There was a problem hiding this comment.
yep, realised this ... will change it to -1.
There was a problem hiding this comment.
The latter could presumably expose node to more attacks, though
I am wondering what kind of attacks, because read would anyways be done by the libp2p layer ir-respective of length passed. It is only the length check for the message that is done based on what is passed to it.
We could keep it to 1MB, but then it is going to cause issues for protocols like Store where response can include 100*WakuMessageSize bytes which would definitely be more than 1MB even with default value.
If we really have to deal with attacks, then maybe there should be some sort of length negotiation as part of metadata protocol or some other protocol when nodes connect to each other. This i feel is overkill at this point and can be taken up if we notice any vulnerabilities.
There was a problem hiding this comment.
owever, even here the node could likely not configure message size to anything larger than the underlying libp2p limit (of 1MB).
Also, if this is a hard limit set by libp2p...then maybe we shouldn't let users configure anything more than 1MB-WakuHeaders as maxMsgSize.
There was a problem hiding this comment.
I am wondering what kind of attacks
Mmm. Indeed, not sure if possible impacts. It seems to me if someone were to pack the length prefix (or just send a very large amount of bytes), we'd have some potential to DoS the stream. This is a sanity check for the buffer length and if not reasonable the stream reader exits: https://github.com/vacp2p/nim-libp2p/blob/b30b2656d52ee304bd56f8f8bbf59ab82b658a36/libp2p/stream/lpstream.nim#L238-L239
There was a problem hiding this comment.
We could keep it to 1MB, but then it is going to cause issues for protocols like Store where response can include 100*WakuMessageSize bytes which would definitely be more than 1MB even with default value.
Which is why for store the max rpc size is MaxPageSize*MaxWakuMsgSize + safety_margin. Agree though. This is complicated by configurable sizes.
There was a problem hiding this comment.
This i feel is overkill at this point and can be taken up if we notice any vulnerabilities.
Agree.
Description
While debugging waku-org/go-waku#1076, i had noticed that for lightpush protocol, the read from stream is being done by using defaultMaxMsgSize which is 150Kib. This means even if a node sets this value to a higher one, lightpush send fails from a client if messageSize crosses 150Kb+overhead.
More discussion on this here : https://discord.com/channels/1110799176264056863/1230492037099294720
Changes
- [ ] LightPush serviceNode to use MaxWakuMessageSize to calculate RPCSize while reading a message. Now i am not so sure if this is the right approach, rather let it try to publish via relay which would fail?