Page MenuHomePhabricator

Ban nodes providing an invalid checksum in a message header
ClosedPublic

Authored by jasonbcox on Jan 11 2019, 00:43.

Details

Summary

This is a DoS vulnerability.

Test Plan

test_runner.py to ensure other tests continue to pass in addition to the newly added abc-invalid-message.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cve891
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4823
Build 7709: Bitcoin ABC Teamcity Staging
Build 7708: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jan 11 2019, 00:43
jasonbcox created this object with visibility "Custom Policy".
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 11 2019, 00:43
jasonbcox updated this revision to Diff 6597.Jan 11 2019, 01:00

Update because Phab wants me to

deadalnix requested changes to this revision.Jan 11 2019, 01:06
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Jan 11 2019, 01:06
jasonbcox updated this revision to Diff 6622.Jan 11 2019, 21:18

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

jasonbcox edited the test plan for this revision. (Show Details)Jan 11 2019, 21:19
deadalnix requested changes to this revision.Jan 11 2019, 22:52

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 ↗(On Diff #6622)

Use a xor or something, because this may not invalid the checksum

42 ↗(On Diff #6622)

You don't need two nodes.

64 ↗(On Diff #6622)

You should use a very simple message like ping, inv or sedheaders, and certainly avoid something that is very stateful.

74 ↗(On Diff #6622)

Remove

This revision now requires changes to proceed.Jan 11 2019, 22:52
jasonbcox updated this revision to Diff 6654.Jan 14 2019, 20:16
jasonbcox marked 4 inline comments as done.

Rebase + changes according to feedback

deadalnix requested changes to this revision.Jan 14 2019, 22:39

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.

This revision now requires changes to proceed.Jan 14 2019, 22:39
jasonbcox marked 2 inline comments as done.Jan 14 2019, 23:31
jasonbcox added inline comments.
test/functional/abc-invalid-message.py
35 ↗(On Diff #6654)

xor resulting in 0 is my bad...
the mask was necessary when xor used to be an inversion, as python was reevaluating the size... it was dumb.
Both of these things should be fixed now.

jasonbcox updated this revision to Diff 6658.Jan 14 2019, 23:33
jasonbcox marked an inline comment as done.

Fix zero'd checksum and simplified checksum index calculation

You did not check what happens when the message through which the connection is established has a faulty checksum.

Fabien added inline comments.Jan 15 2019, 08:17
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.
This does not seem correct to me, if there is a checksum then I expect it to fail from time to time.
I suggest to use a lower penalty here, so that it will allow for "natural" checksum errors while preventing a DoS.

Fabien requested changes to this revision.Jan 15 2019, 08:21
This revision now requires changes to proceed.Jan 15 2019, 08:21
jasonbcox added inline comments.Jan 15 2019, 18:58
src/net_processing.cpp
3359 ↗(On Diff #6658)

We can pick 20 (commonly selected value for other non-instant ban offenses).

deadalnix added inline comments.Feb 1 2019, 03:07
src/net_processing.cpp
3359 ↗(On Diff #6658)

How often does this happen? Asking for two friends.

jasonbcox updated this revision to Diff 7100.Feb 1 2019, 18:48

Rebasing due to lots of refactors to account for

jasonbcox planned changes to this revision.Feb 1 2019, 18:49

Still a WIP for the test changes Amaury pointed out

jasonbcox updated this revision to Diff 7101.Feb 1 2019, 20:11

Add test case for version message with invalid checksum (on connection to a peer)

jasonbcox added inline comments.Feb 1 2019, 20:13
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).

deadalnix requested changes to this revision.Feb 1 2019, 23:45
deadalnix added inline comments.
src/net_processing.cpp
3407

Ban the node.

This revision now requires changes to proceed.Feb 1 2019, 23:45
jasonbcox updated this revision to Diff 7119.Feb 2 2019, 01:26

Ban the node explicitly + fix a flakiness bug in the test

deadalnix accepted this revision.Feb 2 2019, 01:40
Fabien accepted this revision.Feb 3 2019, 14:10

OK for banning directly in order to handle the version checksum case

This revision is now accepted and ready to land.Feb 3 2019, 14:10
jasonbcox changed the visibility from "Custom Policy" to "Public (No Login Required)".Feb 8 2019, 03:27
This revision was automatically updated to reflect the committed changes.