This is a DoS vulnerability.
Details
Details
- Reviewers
deadalnix Fabien - Group Reviewers
Restricted Project - Commits
- rSTAGING2939a67e6916: Ban nodes providing an invalid checksum in a message header
rABC2939a67e6916: Ban nodes providing an invalid checksum in a message header
test_runner.py to ensure other tests continue to pass in addition to the newly added abc-invalid-message.py
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cve891
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4492 Build 7047: Bitcoin ABC Buildbot (legacy) Build 7046: arc lint + arc unit
Event Timeline
| test/functional/abc-invalid-message.py | ||
|---|---|---|
| 24 ↗ | (On Diff #6597) | This should use a well defined command and alter the checksum. Check the message with proper checksum is accepted and then check that the message with an altered checksum is rejected. If not, we just test that the node really do not like what we send to it, which can be for any reason, the most likely one having nothing to do with the checksum, but because we fucked up the message construction in some way. Even if not now, this WILL be what this test actually checks for in no time. |
| test/functional/test_framework/mininode.py | ||
| 1577 ↗ | (On Diff #6597) | A more elegant way to achieve this is to generate the message and then alter provide a way to alter it. I not, the logic in send_message gets more or less duplicated. |
| 1579 ↗ | (On Diff #6597) | This name is misleading. |
Comment Actions
Rewrote the test to include:
- valid message
- invalid message derived from an invalid one
Rewrote the changes I introduced in mininode.py to be more elegant
Comment Actions
Make sure the ban happen when issuing new connexions. What happen when the very first message (that should be version) is borked ?
| test/functional/abc-invalid-message.py | ||
|---|---|---|
| 35 | Use a xor or something, because this may not invalid the checksum | |
| 42 | You don't need two nodes. | |
| 64 | You should use a very simple message like ping, inv or sedheaders, and certainly avoid something that is very stateful. | |
| 74 | Remove | |
Comment Actions
You did not check what happens when the message through which the connection is established has a faulty checksum.
| test/functional/abc-invalid-message.py | ||
|---|---|---|
| 31 ↗ | (On Diff #6654) | It seems to be that these two line always add up to 12. |
| 35 ↗ | (On Diff #6654) | message_data[i] ^ message_data[i] is always 0 so I do not think this is a good test. Xoring with a constant is preferable. I also do not thing the masking helps as binary ops cannot increase the size of the number to something that is out of bounds, by definition. |
| test/functional/abc-invalid-message.py | ||
|---|---|---|
| 35 ↗ | (On Diff #6654) | xor resulting in 0 is my bad... |
| src/net_processing.cpp | ||
|---|---|---|
| 3359 ↗ | (On Diff #6658) | If 100 is the default ban score, then a bad checksum will immediately cause a node to get banned. |
| src/net_processing.cpp | ||
|---|---|---|
| 3359 ↗ | (On Diff #6658) | We can pick 20 (commonly selected value for other non-instant ban offenses). |
| src/net_processing.cpp | ||
|---|---|---|
| 3359 ↗ | (On Diff #6658) | How often does this happen? Asking for two friends. |
| src/net_processing.cpp | ||
|---|---|---|
| 3359 ↗ | (On Diff #6658) | Another thing to note is that lowering this value below 100 means nodes connecting using an invalid version message will never be banned for doing so (given the current implementation). |
| src/net_processing.cpp | ||
|---|---|---|
| 3407 ↗ | (On Diff #7101) | Ban the node. |