Skip to content

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Mar 10, 2021

Put a limit on the amount of data Sock::RecvUntilTerminator() can read
if no terminator is received.

In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.

@jonatack
Copy link
Member

Is this from the OOM raised by the new fuzzer? Approach ACK; a test may be good.

@vasild
Copy link
Contributor Author

vasild commented Mar 10, 2021

I don't think so. To demonstrate the problem this PR is fixing a fuzz seed would have to supply all the data to cause OOM, whereas the output of the OOM you reported reads "will not generate inputs larger than 1048576 bytes".

@laanwj
Copy link
Member

laanwj commented Mar 10, 2021

Concept and code review ACK 32b63eb166470b05795f39ab01bc57d62abf79e7

@laanwj laanwj added the P2P label Mar 10, 2021
@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@practicalswift
Copy link
Contributor

Concept ACK

Happy this was caught so soon after I2P merge, and more importantly before release: we're doing something right :)

@vasild May I ask how you found this issue?

If it wasn't found using the the I2P fuzzer awaiting review, could you add a fuzzing harness which would have been able to catch this issue ("regression fuzzing")?

@maflcko maflcko removed the 1 ACK label Mar 11, 2021
@vasild
Copy link
Contributor Author

vasild commented Mar 11, 2021

@practicalswift,

how you found this issue?

Exactly the right question! :)

It was not found by #21387, but a problem in it put this in front of my eyes. While fuzzing with #21387 I noticed that there is one slow case which was unexpected because the fuzz test is very small and should be quick.

It turned out that once the fuzz data is exhausted FuzzedSock::Recv() will keep returning -1 and setting errno to EAGAIN forever. So RecvUntilTerminator() was (properly) re-calling Recv() zillion times before it gave up due to a timeout. This was fixed in the test by 5e08508 but it made me think what's the worse that can happen with a Recv() that never returns a terminator.

we're doing something right

That is adding tests that increase the coverage, I guess :)

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 32b63eb166470b05795f39ab01bc57d62abf79e7

Do we need to make the new param max_data optional?

Test/fuzz coverage here or as a follow-up would be 👍

@vasild
Copy link
Contributor Author

vasild commented Mar 11, 2021

Do we need to make the new param max_data optional?

I think it is good to have the possibility for the old behavior (unlimited), which fits well with std::optional - "if the limit is not set, then it is unlimited".

@vasild
Copy link
Contributor Author

vasild commented Mar 11, 2021

Appended a new commit with a unit test to ensure that the added functionality of Sock::RecvUntilTerminator() works as expected.

This is not a regression test that would fail if the bug being fixed by this PR resurfaces. I am working on that too, but it will require the changes from #21387 and will be in a separate PR of its own.

@vasild
Copy link
Contributor Author

vasild commented Mar 12, 2021

Appended a new commit with a unit test to ensure that the added functionality of Sock::RecvUntilTerminator() works as expected.

Or, in other words: invalidate two ACKs and break the CI at the same time 😞

b9373ec8f...28cfd6de4: fix the CI failure

@bitcoin bitcoin deleted a comment Mar 12, 2021
@vasild vasild force-pushed the limit_RecvUntilTerminator branch from 28cfd6d to faea165 Compare March 13, 2021 05:50
@vasild
Copy link
Contributor Author

vasild commented Mar 13, 2021

28cfd6de4...faea165c9: move the MAX_MSG_SIZE constant from i2p.cpp to i2p.h so that it can be used later by tests to send more than that without \n.

@MarcoFalke fa!

@laanwj
Copy link
Member

laanwj commented Mar 16, 2021

Do we need to make the new param max_data optional?

I think it is good to have the possibility for the old behavior (unlimited), which fits well with std::optional - "if the limit is not set, then it is unlimited".

I also thought about this; if there is no realistic case where to use "unlimited" (as it would always open some kind of DoS risk), it is more developer friendly to remove the possibility to set it.

vasild added 2 commits March 16, 2021 11:00
Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
if no terminator is received.

In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.
@vasild vasild force-pushed the limit_RecvUntilTerminator branch from faea165 to 7059e6d Compare March 16, 2021 10:08
@vasild
Copy link
Contributor Author

vasild commented Mar 16, 2021

Alright, keep it simple - no callers currently need "unlimited" behavior, so drop that functionality. It would be easy to re-add it if necessary in the future. Thanks, @jonatack, @laanwj, @MarcoFalke for the feedback!

faea165c9...7059e6d82: remove optional'ness of the max size argument

@laanwj
Copy link
Member

laanwj commented Mar 16, 2021

Thank you
Re-ACK 7059e6d

@laanwj laanwj merged commit 1b6c463 into bitcoin:master Mar 16, 2021
@vasild vasild deleted the limit_RecvUntilTerminator branch March 16, 2021 12:25
vasild added a commit to vasild/bitcoin that referenced this pull request Mar 16, 2021
Add a regression test for bitcoin#21407.

The test creates a socket that, upon read, returns some data, but never
the expected terminator `\n`, injects that socket into the I2P code and
expects `i2p::sam::Session::Connect()` to fail, printing a specific
error message to the log.
@vasild
Copy link
Contributor Author

vasild commented Mar 16, 2021

a test may be good

A test requires the changes made in #21387, added in that PR under the commit test: add I2P test for a runaway SAM proxy.

@jonatack
Copy link
Member

Changes since my previous review LGTM per git diff 32b63eb 7059e6d

Posthumous ACK 7059e6d

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 16, 2021
7059e6d test: add a test to ensure RecvUntilTerminator() limit works (Vasil Dimov)
80a5a8e i2p: limit the size of incoming messages (Vasil Dimov)

Pull request description:

  Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
  if no terminator is received.

  In the case of I2P this avoids a runaway (or malicious) I2P proxy
  sending us tons of data without a terminator before a timeout is
  triggered.

ACKs for top commit:
  laanwj:
    Re-ACK 7059e6d

Tree-SHA512: 21f3f3468c765c726cdc877eae847cdb4dbe225d94c5bd1849bd752c9761fac881c554f16ea7a685ad40312159d554e126c481e21c5fd83a6d947060b920373d
laanwj added a commit to bitcoin-core/gui that referenced this pull request Mar 30, 2021
40316a3 test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac77 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b586110 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d4 fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49 fuzz: implement unimplemented FuzzedSock methods (Vasil Dimov)

Pull request description:

  Change the networking code and the I2P code to be fully mockable and use `FuzzedSocket` to fuzz the I2P methods `Listen()`, `Accept()` and `Connect()`.

  Add a mocked `Sock` implementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in bitcoin/bitcoin#21407.

ACKs for top commit:
  practicalswift:
    Tested ACK 40316a3
  MarcoFalke:
    Concept ACK 40316a3
  jonatack:
    re-ACK 40316a3 reviewed `git range-diff 01bb3af 23c861d 40316a3` and the new unit test commit, debug built, ran unit tests, ran bitcoind with an I2P service and network operation with seven I2P peers (2 in, 5 out) is looking nominal
  laanwj:
    Code review ACK 40316a3

Tree-SHA512: 7fc4f129849e16e0c7e16662d9f4d35dfcc369bb31450ee369a2b97bdca95285533bee7787983e881e5a3d248f912afb42b4a2299d5860ace7129b0b19623cc8
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 15, 2022
Summary:
> net: add connect() and getsockopt() wrappers to Sock
>
> Extend the `Sock` class with wrappers to `connect()` and `getsockopt()`.
>
> This will make it possible to mock code which uses those.
bitcoin/bitcoin@b586110

> net: change ConnectSocketDirectly() to take a Sock argument
>
> Change `ConnectSocketDirectly()` to take a `Sock` argument instead of a
> bare `SOCKET`. With this, use the `Sock`'s (possibly mocked) methods
> `Connect()`, `Wait()` and `GetSockOpt()` instead of calling the OS
> functions directly.
bitcoin/bitcoin@82d360b

> i2p: use pointers to Sock to accommodate mocking
>
> Change the types of `i2p::Connection::sock` and
> `i2p::sam::Session::m_control_sock` from `Sock` to
> `std::unique_ptr<Sock>`.
>
> Using pointers would allow us to sneak `FuzzedSock` instead of `Sock`
> and have the methods of the former called.
>
> After this change a test only needs to replace `CreateSock()` with
> a function that returns `FuzzedSock`.
bitcoin/bitcoin@9947e44

> test: add I2P test for a runaway SAM proxy
>
> Add a regression test for bitcoin/bitcoin#21407.
>
> The test creates a socket that, upon read, returns some data, but never
> the expected terminator `\n`, injects that socket into the I2P code and
> expects `i2p::sam::Session::Connect()` to fail, printing a specific
> error message to the log.
bitcoin/bitcoin@40316a3

This is a partial backport of [[bitcoin/bitcoin#21387 | core#21387]], without the fuzzer changes.
Depends on D11037

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11042
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants