EOF: limit validated container size to MAX_INITCODE_SIZE #930

pdobacz · 2024-06-17T12:02:16Z

codecov · 2024-06-17T12:14:01Z

Codecov Report

Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.

Project coverage is 94.27%. Comparing base (3cab4ca) to head (91f9a88).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
- Coverage   94.30%   94.27%   -0.03%     
==========================================
  Files         142      142              
  Lines       16113    16123      +10     
==========================================
+ Hits        15195    15200       +5     
- Misses        918      923       +5

Flag	Coverage Δ
eof_execution_spec_tests	`?`
ethereum_tests	`26.98% <27.27%> (+<0.01%)`	⬆️
ethereum_tests_silkpre	`18.85% <0.00%> (-0.01%)`	⬇️
execution_spec_tests	`17.97% <0.00%> (-0.02%)`	⬇️
unittests	`89.66% <54.54%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
lib/evmone/eof.hpp	`100.00% <ø> (ø)`
test/unittests/eof_validation_test.cpp	`99.33% <100.00%> (+<0.01%)`	⬆️
test/unittests/eof_validation.cpp	`90.83% <0.00%> (-1.54%)`	⬇️
lib/evmone/eof.cpp	`85.79% <50.00%> (-0.39%)`	⬇️

pdobacz · 2024-06-20T10:50:12Z

lib/evmone/eof.cpp

@@ -579,6 +582,10 @@ EOFValidationError validate_eof1(evmc_revision rev, bytes_view main_container) n
        bytes_view bytes;
        bool referenced_by_eofcreate = false;
    };
+
+    if (main_container.size() > MAX_INITCODE_SIZE)


An interesting case come up, concerning this piece: Should this check be done here (top-level validate_eof) or in validate_header.

An expectation would be to have the logic of validate_header protected against oversized containers. It has been since creation txs moved to the public API of eof.hpp, so, in principle, it should also contain the check as one of the first.

OTOH, this would be wasteful, as we'd be repeating the check for all subcontainers, which are always smaller than top-level. Also validate_header bare is only ever called in host.cpp during the process of validating a creation tx data (to be precise - to discover the split between container and calldata) - but there it is protected by the transaction-level MAX_INITCODE_SIZE check.

Let me know if you have any thoughts on this

Does it mean the validate_header doesn't work correctly on its own?

Does it mean the validate_header doesn't work correctly on its own?

Only guaranteed for containers which have its size under the limit. In other words - checking size is outside of validate_header's responsibility.

If this is now the validate_header() precondition then add an assert in the validate_header().

ok, I reworked the comment and assertion in validate_header. I think this is good now, thx!

pdobacz · 2024-06-20T16:37:37Z

Converted to draft b/c testing something on the CI front, ~~needs to remove the TEMP commits before merging!~~ EDIT: removed already

pdobacz · 2024-06-24T13:23:48Z

NOTE in a previous version I mistakenly added overflow checks for offsets (which accumulate section sizes). On second look, these weren't necessary, since a single check is done in validate_section_headers, which preceded the new checks. So it is just the MAX_INITCODE_SIZE requirement and check added, which prevents overflows and allows us to remove the overflow assertion (and not add new ones for other sections).

I have force pushed a version without these. Now codecov complains only about new lines with error message handling

chfast · 2024-06-25T10:10:51Z

lib/evmone/eof.cpp

@@ -771,8 +784,7 @@ std::variant<EOF1Header, EOFValidationError> validate_header(
        container_offsets.emplace_back(static_cast<uint16_t>(offset));
        offset += container_size;
    }
-    // NOTE: assertion always satisfied only as long as initcode limits apply (48K).
-    assert(offset <= std::numeric_limits<uint16_t>::max());


I think we should keep the assert.

This looks still removed?

I mean I would still keep it even if it's checked in validate_section_headers()

ah it moved up a bit and changed form, but the outcome is still the same (I think, please double check if in doubt) - the cast is safe.

assert(container.size() <= MAX_INITCODE_SIZE); checks that container size is checked outside validate_header() and validate_section_headers() check that declared containers sizes sum doesn't exceed container size, so the cast is safe, but I would still keep the assertion here (to prevent future errors in case validate_section_headers() is changed).

Also currently it's incosistent, you removed this assertion, but kept another one at line 776.

Also currently it's incosistent, you removed this assertion, but kept another one at line 776.

ouch, right, completely missed that.

OK on keeping the assertion, but I guess just one assertion at the end (the removed one) will be enough?

like 537d394 for example

I would keep the one in code_size loop, too. If something goes wrong, it's easier to debug when it's detected early.

okay, then let's keep all, one for each cast to uint16 (one was missing originally)

chfast · 2024-06-25T10:19:49Z

lib/evmone/eof.cpp

@@ -579,6 +582,10 @@ EOFValidationError validate_eof1(evmc_revision rev, bytes_view main_container) n
        bytes_view bytes;
        bool referenced_by_eofcreate = false;
    };
+
+    if (main_container.size() > MAX_INITCODE_SIZE)


Does it mean the validate_header doesn't work correctly on its own?

pdobacz · 2024-06-26T10:02:49Z

The 2 failed EEST cases are already failing for the base 1.0.4 release + evmone master, and are related to the RETURNCONTRACT/EOFCREATE rules introduced elsewhere. No objections to merge as is and fix forward @chfast (I'm already working on this)?

pdobacz force-pushed the max-initcode-size branch from fe1e32d to 28f5f83 Compare June 17, 2024 12:09

chfast added the EOF label Jun 20, 2024

pdobacz commented Jun 20, 2024

View reviewed changes

pdobacz force-pushed the max-initcode-size branch 2 times, most recently from f36adca to e22b44a Compare June 20, 2024 15:51

pdobacz marked this pull request as draft June 20, 2024 16:37

pdobacz force-pushed the max-initcode-size branch 2 times, most recently from 7a0ffdc to e99bc18 Compare June 24, 2024 11:47

pdobacz marked this pull request as ready for review June 24, 2024 12:09

pdobacz force-pushed the max-initcode-size branch from e99bc18 to 2ba1458 Compare June 24, 2024 12:54

pdobacz force-pushed the max-initcode-size branch from 2ba1458 to d55f69d Compare June 25, 2024 09:24

pdobacz requested review from chfast and gumb0 June 25, 2024 10:05

chfast reviewed Jun 25, 2024

View reviewed changes

pdobacz added 3 commits June 26, 2024 10:03

EOF: limit validated container size to MAX_INITCODE_SIZE

5eab31d

Point CI to ipsilon fork's release of EOF-EEST

497affb

Address feedback in validate_header

8251d34

pdobacz force-pushed the max-initcode-size branch from ec4b1f7 to 8251d34 Compare June 26, 2024 08:17

pdobacz self-assigned this Jun 26, 2024

chfast approved these changes Jun 26, 2024

View reviewed changes

Tidy assertions in validate_header

91f9a88

pdobacz force-pushed the max-initcode-size branch from 537d394 to 91f9a88 Compare June 26, 2024 13:51

gumb0 approved these changes Jun 26, 2024

View reviewed changes

pdobacz merged commit dbf8fb4 into master Jun 26, 2024
23 of 26 checks passed

pdobacz deleted the max-initcode-size branch June 26, 2024 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOF: limit validated container size to MAX_INITCODE_SIZE #930

EOF: limit validated container size to MAX_INITCODE_SIZE #930

pdobacz commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

pdobacz Jun 20, 2024

chfast Jun 25, 2024

pdobacz Jun 25, 2024

chfast Jun 25, 2024

pdobacz Jun 26, 2024

pdobacz commented Jun 20, 2024 •

edited

Loading

pdobacz commented Jun 24, 2024

chfast Jun 25, 2024

gumb0 Jun 26, 2024

gumb0 Jun 26, 2024

pdobacz Jun 26, 2024

gumb0 Jun 26, 2024

pdobacz Jun 26, 2024

pdobacz Jun 26, 2024

gumb0 Jun 26, 2024

pdobacz Jun 26, 2024

chfast Jun 25, 2024

pdobacz commented Jun 26, 2024

EOF: limit validated container size to MAX_INITCODE_SIZE #930

EOF: limit validated container size to MAX_INITCODE_SIZE #930

Conversation

pdobacz commented Jun 17, 2024 • edited Loading

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdobacz commented Jun 20, 2024 • edited Loading

pdobacz commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdobacz commented Jun 26, 2024

pdobacz commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

pdobacz commented Jun 20, 2024 •

edited

Loading