Skip to content
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

Jump to bottom

Merge upstream anti DoS patches #1251

Open
bitcartel opened this issue Aug 14, 2016 · 23 comments
Open

Merge upstream anti DoS patches #1251

bitcartel opened this issue Aug 14, 2016 · 23 comments
Assignees
@bitcartel
Labels
C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. I-dos Problems and improvements with respect to Denial-of-Service. I-SECURITY Problems and improvements related to security. S-needs-clearer-scope Status: Needs clearer scope
Projects
Security and Stability

Comments

@bitcartel
Copy link
Contributor

At the time of writing, testnet is at block 4109 but the alpha node is handing out many peers who continuously advertise a block height of 1337 and are not synced. The result is that genuine nodes receiving these peers are unable to join the network.

    {
        "id" : 9,
        "addr" : "x.x.x.x:18233",
        "addrlocal" : "x.x.x.x:55684",
        "services" : "0000000000000001",
        "lastsend" : 1471152190,
        "lastrecv" : 1471152189,
        "bytessent" : 1719575,
        "bytesrecv" : 947,
        "conntime" : 1471152188,
        "timeoffset" : -1,
        "pingtime" : 0.00000000,
        "pingwait" : 31.86533400,
        "version" : 170002,
        "subver" : "/Satoshi:0.11.2/",
        "inbound" : false,
        "startingheight" : 1337,
        "banscore" : 0,
        "synced_headers" : -1,
        "synced_blocks" : -1,
        "inflight" : [
        ],
        "whitelisted" : false
    },

Upstream improvements should be identified and where possible merged. For example "Connection slot exhaustion DoS mitigation" bitcoin/bitcoin#6374
@bitcartel bitcartel added I-SECURITY Problems and improvements related to security. engineering meeting agenda labels Aug 14, 2016
@bitcartel bitcartel changed the title Merge upstream anti DOS patches Merge upstream anti DoS patches Aug 14, 2016
@daira daira added C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. and removed engineering meeting agenda labels Aug 15, 2016
@defuse
Copy link
Contributor

defuse commented Aug 16, 2016

This is related to #630. The way we bootstrap the network needs to be checked for DoS vulnerabilities.

@defuse defuse mentioned this issue Aug 16, 2016
Closed
@defuse
Copy link
Contributor

defuse commented Aug 16, 2016

I don't know the details, but I'm hoping the DoS against the testnet is only because we're not doing bootstrapping (#630) properly. When we launch, hopefully the bootstrapping system will prevent issues like this.

I went to the upstream GitHub repo, searched for "DoS" in Pull Requests, then clicked the "closed" tab, and of the merged ones before I started seeing stuff in the 0.10.0 milestone, and who were merged after 2015-11-10 (the date of the v0.11.2 tag), these seem relevant:

I'll go through that list again and remove irrelevant ones.

@defuse
Copy link
Contributor

defuse commented Aug 16, 2016 •
edited

Here are the ones we might want for 1.0. Backporting them will be a lot of work so let's only backport the ones that will help with actual attacks we're experiencing?

@defuse
Copy link
Contributor

defuse commented Aug 16, 2016

For this ticket I'll bring in 6374 and 6636, since they're the only ones relevant to the way our testnet is currently being DoSed.

@defuse defuse mentioned this issue Aug 16, 2016
Merged
@defuse defuse added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2016
@defuse
Copy link
Contributor

defuse commented Aug 16, 2016

Unless someone objects, let's not bother looking at the PRs in #1251 (comment) right now, and let's just let #1258 close this issue.

@daira
Copy link
Contributor

daira commented Aug 16, 2016 •
edited

I looked at them; some of them are possibly controversial and/or complicated, and definitely should not be in z9. But let's make sure that we keep track of them, since if I were an attacker, the next place I'd look for potential DoS attacks is things that have been fixed in Bitcoin.

zkbot pushed a commit that referenced this issue Aug 18, 2016
Auto merge of #1258 - ThisIsNotOfficialCodeItsJustForks:t1251-upstrea…
af9898e 
…m-anti-dos, r=daira

Pull in some DoS mitigations from upstream

Closes #1251.

**WARNING: I force pushed**
@defuse
Copy link
Contributor

defuse commented Aug 24, 2016

In our (@bitcartel and I) pairing we determined we should take a second look at 7079, 5843, 6193, 6192, 7106 before 1.0.

@defuse defuse reopened this Aug 24, 2016
@defuse defuse modified the milestones: Beta 1 - complete RPC, audit mitigations, other linux support, user bugfixes, z9 Release - RPC, Testing improvements Aug 24, 2016
@defuse
Copy link
Contributor

defuse commented Aug 24, 2016

Ping @nathan-at-least to double-check the prioritization of this.

@defuse defuse removed their assignment Aug 24, 2016
@ebfull ebfull modified the milestones: Beta 2 - audit mitigations and critical bug fixes, Beta 1 - complete RPC Sep 5, 2016
@daira daira added the I-dos Problems and improvements with respect to Denial-of-Service. label Sep 6, 2016
@bitcartel
Copy link
Contributor Author

bitcartel commented Sep 17, 2016 •
edited by str4d

5843 (we already have), 6192 and 6193 (related to REST interface which we do not recommend to be enabled) and:
7106 - PR #1411
7079 - PR #1407

@str4d
Copy link
Contributor

str4d commented Sep 29, 2016

#1407 and #1411 have been merged, which means we now have the following patches:

  • 5843
  • 6374 and 6636
  • 7079
  • 7106

This leaves the following from #1251 (comment):

@defuse
Copy link
Contributor

defuse commented Oct 20, 2016

See: #1574

@bitcartel bitcartel added the M-has-pr To-be-removed (GitHub has linked:pr filter) label Oct 20, 2016
@bitcartel
Copy link
Contributor Author

This ticket should be closed once #1574 lands, even though the nature of the ticket is open-ended.

@daira
Copy link
Contributor

daira commented Oct 22, 2016

#1574 is being bumped to rc3.

@daira daira modified the milestones: 1.0.0-rc3, 1.0.0-rc2 Oct 22, 2016
@nathan-at-least nathan-at-least modified the milestones: 1.0.1 stabilization, 1.0.0-rc3 Oct 24, 2016
@ebfull ebfull modified the milestones: 1.0.5, 1.0.6 Jan 19, 2017
@daira daira removed the M-has-pr To-be-removed (GitHub has linked:pr filter) label Feb 9, 2017
@daira daira modified the milestones: 1.0.7, 1.0.6 Feb 9, 2017
@arcalinea arcalinea modified the milestones: 1.0.8, 1.0.7 Mar 4, 2017
@daira daira added S-needs-clearer-scope Status: Needs clearer scope M-has-pr To-be-removed (GitHub has linked:pr filter) and removed M-has-pr To-be-removed (GitHub has linked:pr filter) labels Mar 20, 2017
@daira daira modified the milestones: 1.0.9, 1.0.8 Mar 26, 2017
@nathan-at-least nathan-at-least removed this from the 1.0.9 milestone May 10, 2017
@nathan-at-least nathan-at-least removed this from Work Queue in Continuous Improvement May 10, 2017
@nathan-at-least
Copy link
Contributor

Bumped from 1.0.9.

Before merging more from upstream, we need to ensure we understand and follow their own QA standards, and I suspect we will want stricter standards if feasible.

@daira daira removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bitcartel bitcartel

Labels
C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. I-dos Problems and improvements with respect to Denial-of-Service. I-SECURITY Problems and improvements related to security. S-needs-clearer-scope Status: Needs clearer scope
Projects
No open projects
Security and Stability
  
Work Queue
Milestone
No milestone
Development

No branches or pull requests
7 participants
@bitcartel @daira @ebfull @defuse @nathan-at-least @str4d @arcalinea