-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cryptonote_protocol: some stricter checks on object requests #6554
cryptonote_protocol: some stricter checks on object requests #6554
Conversation
src/cryptonote_core/blockchain.cpp
Outdated
const uint64_t height_i = m_db->get_block_height(arg.blocks[i]); | ||
if (height_i != height + 1) | ||
{ | ||
error_message = "NOTIFY_REQUEST_GET_OBJECTS requests non contiguous blocks (height " + std::to_string(height_i) + " following height " + std::to_string(height) + ")"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we shouldn't forbid non contiguous get blocks requests.
It is just the way Monero reference implementation (monreod
) currently works and I doubt we should enforce this on the protocol level.
Could you clarify the problem/weakness here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It prevents the (admittedly fairly coarse) check against pre-v4 and post v4 max blocks from being useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't help to mitigates a DoS anyhow. Am i missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If blocks aren't contiguous, you can't easily clamp to 20 (instead of 100) for post rct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and if you're going to say "count bytes", it's not a trivial patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTIFY_REQUEST_GET_OBJECTS
request contains a list of block hashes to receive. It puts no restriction on the heights.
So it allows me to request block #0
and block #100
in a single request.
You are going to forbid this and force p2p client to always request contiguous blocks, i.e. block #0
, block #1
, block #2
.
What is the point of having a list of block hashes in the request then? Just a the first block hash
and blocks count
would be enough.
Summing this up. The patch introduces some problems to legit P2P clients and changes almost nothing from the DoS perspective, an attacker will just select 100 contiguous blocks span and spam your node with the requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proper way would be just to change the server code. We should just limit blocks count we send back in the reply. I.e. no matter how many blocks a client requested, we always serve only first N
blocks from the request, N
- the limit we want to put there (20/100/500/etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monerod does not do that (unless you find a case where it does).
The point of having a list of block hashes is that it is a lot more work to avoid doing that. WhatI don't like the argument of saying no improvement because a better improvement can be made regardless of the amount of work involved (in general, it can apply in some cases). There wlil be a new call where you give a starting hash and a number, but I object to having to do everything at once or nothing.
The patch does not introduce problems to legit P2P clients, unless you find a case where it does.
The patch does mitigate DoS potential somewhat (I do not claim it removes the DoS) because it does not allow more than 20 blocks for the borromean part of the chain where blocks are large, and no more than 100 (which is the max monerod requests unless you change it manually) otherwise, rather than 500.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is all about P2P protocol, not about referene monero node implementation (monerod).
There might be other monero node implementations. And this change will break the P2P protocol for them. Even if you still want to do this, you should bump the P2P protocol version.
But i already mentioned the correct way this should be handled, see #6554 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really do not care. If anyone made custom code, they can just change the number of blocks they have.
You can't just send fewer blocks, the client will complain and drop. This will require something else, which I will later.
f66561d
to
6efbc29
Compare
@@ -51,7 +51,8 @@ PUSH_WARNINGS | |||
DISABLE_VS_WARNINGS(4355) | |||
|
|||
#define LOCALHOST_INT 2130706433 | |||
#define CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT 500 | |||
#define CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet 500
blocks limit works pretty well. Any good reason to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To mitigate DoS potential, as above. 100 objects are smaller than 500 (ceteris paribus).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first two commits looks okay to me.
- cryptonote_protocol: reject requests/notifications before handshake fed0ced
- cryptonote_protocol: stricter limit to number of objects requested fb24d22
Regarding the third commit: i'd better nuke it. Or drop switching between pre-v4 limit and post-v4 limit, i.e. use single limit (no more than 100 blocks per request).
Nuke because it's actually wrong, or because you don't like it because there's a better way to do it later if we spend more time on it ? |
(I don't see "it will break third party code that happens to request more blocks than monerod" as wrong, it's what we want to prevent in the first place) |
6efbc29
to
fb24d22
Compare
I've removed the third one then, since rbrunner is also against. |
I got actual blockchain data: max block size pre v4: 89417 Assuming dupes can be requested (I think they can, I did not double check): So the sequential restrictions gains a bit, while the limit to 20 gains a lot. |
(up to height 2e6, I do not expect the numbers to differ substantially for the tail end) |
The limit to 20 blocks post v4 is important for mitigating the DOS. It doesn't break anything for existing monerod clients, and it should be simple for any other implementations to reduce the number of blocks they request. The blocks being sequential is not as important, but again it does not break any known non-spammy implementations. |
Reported by xnbya
Reported by xnbya
fb24d22
to
bbab044
Compare
No description provided.