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.

Sign up for GitHub

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

p2p: rejected inbound connections aren't closed, cause fd leak #2046

Closed
ebuchman opened this issue Jul 24, 2018 · 4 comments
Closed

p2p: rejected inbound connections aren't closed, cause fd leak #2046

ebuchman opened this issue Jul 24, 2018 · 4 comments
Assignees
@xla
Labels
C:p2p Component: P2P pkg T:bug Type Bug (Confirmed) T:security Type: Security (specify priority)
Projects
current iteration
Milestone
Wishlist

Comments

@ebuchman
Copy link
Contributor

Issue was discovered by a security auditor.

We don't close inbound connection objects when we reject them for max peers:

tendermint/p2p/switch.go

Lines 487 to 500 in 05a76fb

func (sw *Switch) listenerRoutine(l Listener) {
for {
inConn, ok := <-l.Connections()
if !ok {
break
}
// ignore connection if we already have enough
// leave room for MinNumOutboundPeers
maxPeers := sw.config.MaxNumPeers - DefaultMinNumOutboundPeers
if maxPeers <= sw.peers.Size() {
sw.Logger.Info("Ignoring inbound connection: already have enough peers", "address", inConn.RemoteAddr().String(), "numPeers", sw.peers.Size(), "max", maxPeers)
continue
}

This can be easily exploited to cause nodes to panic from use of too many file descriptors (and may partially explain some of the high fd usage we've been seeing on testnets).
@ebuchman ebuchman added T:bug Type Bug (Confirmed) C:p2p Component: P2P pkg T:security Type: Security (specify priority) labels Jul 24, 2018
@ebuchman ebuchman added this to Planned in current iteration via automation Jul 24, 2018
@xla
Copy link
Contributor

xla commented Jul 24, 2018

Related to #2006

@xla xla added this to the launch milestone Jul 24, 2018
ebuchman added a commit that referenced this issue Jul 25, 2018
@ebuchman
p2p: fix conn leak. part of #2046
94bbef5
ebuchman added a commit that referenced this issue Jul 25, 2018
@ebuchman
p2p: fix conn leak. part of #2046
359898d
@ebuchman ebuchman mentioned this issue Jul 25, 2018
Merged
4 tasks
@ebuchman
Copy link
Contributor Author

Issue is fixed in 359898d from #2056 and released in v0.22.6 but reopening as we should have better testing for this.

Perhaps will be addressed in #2006 and related

@xla xla mentioned this issue Jul 25, 2018
Closed
@xla
Copy link
Contributor

xla commented Jul 25, 2018

#2006 is now #2067

ValarDragon added a commit that referenced this issue Jul 25, 2018
@ValarDragon
p2p/secret_connection: Switch to using hkdf instead of xsalsa for sec…
0e26ca5 
…ret derivation

This also merges the three calls to hkdf into a single call.
Notably the hkdf call is now just called upon the computed shared
secret, since the shared secret is a function of the pubkeys.

It also makes nonces start at 0, as we are using chacha as a stream
cipher.

 tendermint - revert to v0.10.1

rpc: fix /blockchain OOM #2049

rpc: validate height in abci_query

changelog and version

p2p: fix conn leak. part of #2046

consensus: include evidence in proposed block parts. fixes #2050

makefile: Add `make check_dep` and remove `make ensure_deps` (#2055)

This adds a new makefile command, which is used in CI linting, `make check_dep`.
This ensures the toml is in sync with the lock, and that were not pinning to a
branch in any repository.

This also adapts `make get_vendor_deps` to check the lock, in addition to
populating the vendor directory. This removes the need for `make ensure_deps`.

This makes `make get_vendor_deps` consistent between tendermint and the sdk.
ValarDragon pushed a commit that referenced this issue Jul 25, 2018
@ebuchman @ValarDragon
p2p: fix conn leak. part of #2046
ec65193
@xla xla self-assigned this Jul 27, 2018
@xla xla moved this from Planned to Ongoing in current iteration Jul 27, 2018
@ebuchman
Copy link
Contributor Author

ebuchman commented Aug 5, 2018

Closing for #2067

@ebuchman ebuchman closed this as completed Aug 5, 2018
current iteration automation moved this from Ongoing to Done Aug 5, 2018
xla added a commit that referenced this issue Sep 18, 2018
@xla
p2p: Implement PeerTransport
7c920a2 
This is the implementation for the design described in ADR 12[0]. It's
the first step of a larger refactor of the p2p package as tracked in
interface bundling all concerns of low-level connection handling and
isolating the rest of peer lifecycle management from the specifics of
the low-level internet protocols. Even if the swappable implementation
will never be utilised, already the isolation of conn related code in
one place will help with the reasoning about execution path and
addressation of security sensitive issues surfaced through bounty
programs and audits.

We deliberately decided to not have Peer filtering and other management
in the Transport, its sole responsibility is the translation of
connections to Peers, handing those to the caller fully setup. It's the
responsibility of the caller to reject those and or keep track. Peer
filtering will take place in the Switch and can be inspected in a the
following commit.

This changeset additionally is an exercise in clean separation of logic
and other infrastructural concerns like logging and instrumentation. By
leveraging a clean and minimal interface. How this looks can be seen in
a follow-up change.

Design #2069[2]
Refs #2067[3]
Fixes #2047[4]
Fixes #2046[5]

changes:
* describe Transport interface
* implement new default Transport: MultiplexTransport
* test MultiplexTransport with new constraints
* implement ConnSet for concurrent management of net.Conn, synchronous
to PeerSet
* implement and expose duplicate IP filter
* implemnt TransportOption for optional parametirisation

[0] https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-012-peer-transport.md
[1] #2067
[2] #2069
[3] #2067
[4] #2047
[5] #2046
xla added a commit that referenced this issue Sep 18, 2018
@xla
p2p: Implement PeerTransport
be5d68e 
This is the implementation for the design described in ADR 12[0]. It's
the first step of a larger refactor of the p2p package as tracked in
interface bundling all concerns of low-level connection handling and
isolating the rest of peer lifecycle management from the specifics of
the low-level internet protocols. Even if the swappable implementation
will never be utilised, already the isolation of conn related code in
one place will help with the reasoning about execution path and
addressation of security sensitive issues surfaced through bounty
programs and audits.

We deliberately decided to not have Peer filtering and other management
in the Transport, its sole responsibility is the translation of
connections to Peers, handing those to the caller fully setup. It's the
responsibility of the caller to reject those and or keep track. Peer
filtering will take place in the Switch and can be inspected in a the
following commit.

This changeset additionally is an exercise in clean separation of logic
and other infrastructural concerns like logging and instrumentation. By
leveraging a clean and minimal interface. How this looks can be seen in
a follow-up change.

Design #2069[2]
Refs #2067[3]
Fixes #2047[4]
Fixes #2046[5]

changes:
* describe Transport interface
* implement new default Transport: MultiplexTransport
* test MultiplexTransport with new constraints
* implement ConnSet for concurrent management of net.Conn, synchronous
to PeerSet
* implement and expose duplicate IP filter
* implemnt TransportOption for optional parametirisation

[0] https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-012-peer-transport.md
[1] #2067
[2] #2069
[3] #2067
[4] #2047
[5] #2046
@bennofs bennofs mentioned this issue Jan 5, 2019
Closed
@abelliumnt abelliumnt mentioned this issue Mar 5, 2019
Merged
firelizzard18 pushed a commit to AccumulateNetwork/tendermint that referenced this issue Feb 1, 2024
@faddat @sergio-mena
misc(tools)!: remove tools package (tendermint#2046)
2bbb5c1 
- 1) protos are now built with the sdk's proto builder 
- 2) go tools methodology just bloats .mod and .sum making comet harder
to work with
- 3) tidy: mod and sum don't have maintenance tools in them anymore
- 4) stop using an abandoned Dockerfile


---------


tl;dr: by removing things from go.mod we can make comet easier to
maintain, even if that means less stylish golang.


---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@xla xla

Labels
C:p2p Component: P2P pkg T:bug Type Bug (Confirmed) T:security Type: Security (specify priority)
Projects
No open projects
current iteration
  
Done
Milestone
Wishlist
Development

No branches or pull requests
2 participants
@xla @ebuchman