Request collector stores arbitrary Monitoring Requests

[palango]

An attacker can send an arbitrary amount of MonitoringRequests to the request collector, which will - given matching chain id - store it to the shared db. This allows cheap denial of service attacks against the shared database, the request collector and the monitoring service.

[karlb]

We must accept MonitoringRequests before those channels are confirmed. This makes it hard to just discard MRs in case of a DOS attack without complicating the request collector a lot. I suggest a combination of

• rate limiting (will be useful for many other attacks, too)
• regularly checking the DB for MRs that don't have a matching channel and are older than a certain time.

[palango]

I agree with these two mitigations, @err508 what do you think?

Regarding rate limiting. Is it enough to rate limit in the matrix client when receiving messages, or do we have to put that inside matrix somehow? @ulope @err508 Maybe you can give input on this!

[ulope]

Well that's a bit of a circular problem.

Matrix has server side rate limiting support.
But since we don't want transfers to get throttled we have tuned those to rather high values.
Unfortunately this is a global per-server setting.

But how would an attacker generate arbitrary amounts of requests?
AFAIU only channels that are open on-chain should be allowed to have monitoring requests posted for.

Based on that it should be possible to implement some per-channel-peer rate limiting yourselves, shouldn't it?

[err508]

I'm not really familiar with the client code base, but before a transfer happens there seems to be done this check in the channel which relies on this check in the token_network view.
@karlb could you elaborate on cases where you think the request_collector would have to accept monitoring requests from properly configured clients, where the channel_opened event might not be visible yet?
If it is not to many cases, where valid monitoring requests arrive before the ChannelOpened event is visible, I'd just perform the same check before storing stuff to the MonitoringService db.

[err508]

Also regarding rate limiting or other checks performed in matrix: I think it's good practice to think of a module as an autonomous security perimeter.
MatrixServers might be deployed by some party and I think you don't want to be in the situation where you try to find their phone number in the middle of the night because some other module gets spammed.

[palango]

AFAIU only channels that are open on-chain should be allowed to have monitoring requests posted for.

We tried that in the beginning, but it might happen that a message is send before the services receive the event. Currently we use the same number of confirmation blocks as the client, but we still run into these cases, so for example #423 , which happened today in a scenario run)
So we want to allow receiving messages before the services receive the channel opening event.

Also regarding rate limiting or other checks performed in matrix: I think it's good practice to think of a module as an autonomous security perimeter.
MatrixServers might be deployed by some party and I think you don't want to be in the situation where you try to find their phone number in the middle of the night because some other module gets spammed.

That makes sense.
My question was more in the direction of, does it make sense to have throttling once the messages are deserialized, or should that be something that's implement in the client side matrix client. Because otherwise people could still send junk messages and overload the services, even though we would later filter out all these messages.

[err508]

We tried that in the beginning, but it might happen that a message is send before the services receive the event.

Correct me if I get this wrong, but this seems to be mostly a issue for tests, as in production a RequestMonitoring would be send only after a balance proof was received, which would only happen for a open, confirmed channel. If this is correct, another approach would be to make the chain-event related sanitization of the Requests configurable and switch them off or mock them for tests.

[palango]

Correct me if I get this wrong, but this seems to be mostly a issue for tests, as in production a RequestMonitoring would be send only after a balance proof was received, which would only happen for a open, confirmed channel.

The number of confirmed blocks between client and MS might be different. But even if they are the same and block propagation is a bit slower for the eth node of the MS, we can easily run into that scenario.

[err508]

So would using a strictly smaller number of blocks for confirmation in the MS work?

[karlb]

So would using a strictly smaller number of blocks for confirmation in the MS work?

This would increase the chance that the CannelOpened event arrives in time by a lot. Depending on how strict we want to be, that might be sufficient. But it would be nice if clients and services could choose their number of confirmed block independently.

Also the current approach allows the request collector to be very simple and not have any connection to the blockchain at all. We would lose that simplicity if we checked the MR's channel before saving them.

[err508]

If you don't won't to the request collector to depend on events, you can use the MS for that, e.g. provide an internal api to query for events the MS for seen events. Or simply "misuse" the db a little, store channel_identifiers to the db once the MS sees an event, s.t. the request collector can see that the channel actually exists.

But in general it's the services team's call to make if or how to implement a mitigation, just let me know if I can assist.