-
Notifications
You must be signed in to change notification settings - Fork 38.8k
i2p: limit the size of incoming messages #21407
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
Conversation
|
Is this from the OOM raised by the new fuzzer? Approach ACK; a test may be good. |
|
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". |
|
Concept and code review ACK 32b63eb166470b05795f39ab01bc57d62abf79e7 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
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")? |
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
That is adding tests that increase the coverage, I guess :) |
There was a problem hiding this 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 👍
I think it is good to have the possibility for the old behavior (unlimited), which fits well with |
|
Appended a new commit with a unit test to ensure that the added functionality of 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. |
b9373ec to
28cfd6d
Compare
Or, in other words: invalidate two ACKs and break the CI at the same time 😞
|
28cfd6d to
faea165
Compare
|
@MarcoFalke |
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. |
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.
faea165 to
7059e6d
Compare
|
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!
|
|
Thank you |
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.
A test requires the changes made in #21387, added in that PR under the commit |
|
Changes since my previous review LGTM per Posthumous ACK 7059e6d |
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
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
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
Put a limit on the amount of data
Sock::RecvUntilTerminator()can readif 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.