i2p: limit the size of incoming messages

vasild commented at 12:43 pm on March 10, 2021: member

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 commented at 12:54 pm on March 10, 2021: member

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

vasild commented at 12:59 pm on March 10, 2021: member

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”.

DrahtBot added the label Utils/log/libs on Mar 10, 2021

laanwj commented at 2:16 pm on March 10, 2021: member

Concept and code review ACK 32b63eb166470b05795f39ab01bc57d62abf79e7

laanwj added the label P2P on Mar 10, 2021

DrahtBot commented at 4:43 pm on March 10, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21387 (fuzz: test the I2P Session public interface by vasild)

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.

MarcoFalke added the label 1 ACK on Mar 10, 2021

practicalswift commented at 10:23 pm on March 10, 2021: 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”)?

MarcoFalke removed the label 1 ACK on Mar 11, 2021

vasild commented at 9:08 am on March 11, 2021: member

@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 https://github.com/bitcoin/bitcoin/pull/21387/commits/5e085082fae5b90507aca3277800bfb367705617 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 :)

in src/util/sock.cpp:197 in 32b63eb166 outdated

199+            if (data.size() >= max_data.value()) {
200+                throw std::runtime_error(
201+                    strprintf("Received too many bytes without a terminator (%u)", data.size()));
202+            }
203+            peek_bytes = std::min(sizeof(buf), max_data.value() - data.size());
204+        }

jonatack commented at 10:11 am on March 11, 2021:

The dereference operator operator*() does not check if this optional contains a value, which may be more efficient than value() (https://en.cppreference.com/w/cpp/utility/optional/value). As has_value() is already checked, can also do (maybe not worth it but as a note):

 0         if (max_data.has_value()) {
 1-            if (data.size() >= max_data.value()) {
 2+            const size_t bytes{data.size()};
 3+            if (bytes >= *max_data) {
 4                 throw std::runtime_error(
 5-                    strprintf("Received too many bytes without a terminator (%u)", data.size()));
 6+                    strprintf("Received too many bytes without a terminator (%u)", bytes));
 7             }
 8-            peek_bytes = std::min(sizeof(buf), max_data.value() - data.size());
 9+            peek_bytes = std::min(sizeof(buf), *max_data - bytes);
10         }

vasild commented at 10:54 am on March 11, 2021:

Yeah, there is more than one way to write this. I chose the current one because I think the performance of operator* is negligible in this case and I find .value() more readable. Also

0if (max_data.has_value()) {

can be written as

0if (max_data) {

but I don’t like those “hidden” casts and somehow prefer the former. The latter raises the question: “given that max_data represents a number, did the programmer intend to check for non-zero value instead of whether the optional has value or not?”

I also prefer explicit comparisons (avoid implicit cast to boolean):

0if (foo != 0) { // makes it obvious that foo is a number
1if (foo != nullptr) { // makes it obvious that foo is a pointer
2if (foo) { // use this only if foo is a boolean

jonatack commented at 10:57 am on March 11, 2021:

I generally agree. It was more of a note to expose options.

jonatack commented at 10:23 am on March 11, 2021: member

ACK 32b63eb166470b05795f39ab01bc57d62abf79e7

Do we need to make the new param max_data optional?

Test/fuzz coverage here or as a follow-up would be :+1:

in src/util/sock.h:148 in 32b63eb166 outdated

144      */
145     virtual std::string RecvUntilTerminator(uint8_t terminator,
146                                             std::chrono::milliseconds timeout,
147-                                            CThreadInterrupt& interrupt) const;
148+                                            CThreadInterrupt& interrupt,
149+                                            std::optional<size_t> max_data = std::nullopt) const;

jonatack commented at 10:56 am on March 11, 2021:

not sure what is more idiomatic

0                                            std::optional<size_t> max_data = {}) const;

JeremyRubin commented at 6:34 pm on March 11, 2021:

personally I think nullopt is more readable. E.g., even though we know that {} is nullopt here, size_t x{} would be x == 0 right? So then could max_data={{}} be some(0)?

jonatack commented at 6:54 pm on March 11, 2021:

yes, std::nullopt has the advantage of being more precise

MarcoFalke commented at 7:47 am on March 16, 2021:

Even if the optional is kept, wouldn’t it make more sense to not provide a default value here?

vasild commented at 10:12 am on March 16, 2021:

Dropped std::optional altogether.

vasild commented at 11:17 am on March 11, 2021: member

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 commented at 3:31 pm on March 11, 2021: member

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 force-pushed on Mar 12, 2021

vasild commented at 6:16 am on March 12, 2021: member

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 :disappointed:

b9373ec8f...28cfd6de4: fix the CI failure

fanquake deleted a comment on Mar 12, 2021

vasild force-pushed on Mar 13, 2021

vasild commented at 5:51 am on March 13, 2021: member

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 commented at 7:42 am on March 16, 2021: member

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.

i2p: limit the size of incoming messages

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.

80a5a8ea2b

test: add a test to ensure RecvUntilTerminator() limit works 7059e6d822

vasild force-pushed on Mar 16, 2021

vasild commented at 10:11 am on March 16, 2021: member

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 commented at 11:13 am on March 16, 2021: member

Thank you Re-ACK 7059e6d82275b44efc41675ee10760145b6c1073

laanwj merged this on Mar 16, 2021

laanwj closed this on Mar 16, 2021

vasild deleted the branch on Mar 16, 2021

vasild referenced this in commit 40316a37cb on Mar 16, 2021

vasild commented at 2:03 pm on March 16, 2021: member

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 commented at 2:59 pm on March 16, 2021: member

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

Posthumous ACK 7059e6d82275b44efc41675ee10760145b6c1073

sidhujag referenced this in commit 4ec4c4b0c5 on Mar 16, 2021

laanwj referenced this in commit f9e86d8966 on Mar 30, 2021

Fabcien referenced this in commit de38a2d80d on Feb 15, 2022

DrahtBot locked this on Aug 18, 2022

i2p: limit the size of incoming messages #21407

Conflicts