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.

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

EOF: limit validated container size to MAX_INITCODE_SIZE #930

Merged
merged 4 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ commands:

download_execution_spec_tests:
parameters:
repo:
type: string
default: ethereum/execution-spec-tests
release:
type: string
fixtures_suffix:
Expand All @@ -139,7 +142,7 @@ commands:
name: "Download execution-spec-tests: <<parameters.release>>"
working_directory: ~/spec-tests
command: |
curl -L https://github.com/ethereum/execution-spec-tests/releases/download/<<parameters.release>>/fixtures_<<parameters.fixtures_suffix>>.tar.gz | tar -xz
curl -L https://github.com/<<parameters.repo>>/releases/download/<<parameters.release>>/fixtures_<<parameters.fixtures_suffix>>.tar.gz | tar -xz
ls -l

build:
Expand Down Expand Up @@ -398,7 +401,8 @@ jobs:
steps:
- build
- download_execution_spec_tests:
release: eip7692@v1.0.3
repo: ipsilon/execution-spec-tests
release: eip7692@v1.0.4+ipsilon_bf37249b
fixtures_suffix: eip7692
- run:
name: "EOF pre-release execution spec tests (state_tests)"
Expand Down
19 changes: 18 additions & 1 deletion lib/evmone/eof.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
constexpr auto STACK_SIZE_LIMIT = 1024;
constexpr uint8_t NON_RETURNING_FUNCTION = 0x80;

constexpr size_t MAX_CODE_SIZE = 0x6000;
constexpr size_t MAX_INITCODE_SIZE = 2 * MAX_CODE_SIZE;

using EOFSectionHeaders = std::array<std::vector<uint16_t>, MAX_SECTION + 1>;

size_t eof_header_size(const EOFSectionHeaders& headers) noexcept
Expand Down Expand Up @@ -592,6 +595,10 @@
bytes_view bytes;
ContainerKind kind;
};

if (main_container.size() > MAX_INITCODE_SIZE)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

return EOFValidationError::container_size_above_limit;

Check warning on line 600 in lib/evmone/eof.cpp

View check run for this annotation

Codecov / codecov/patch

lib/evmone/eof.cpp#L600

Added line #L600 was not covered by tests

// Queue of containers left to process
std::queue<ContainerValidation> container_queue;

Expand Down Expand Up @@ -738,6 +745,12 @@
if (rev < EVMC_PRAGUE)
return EOFValidationError::eof_version_unknown;

// `offset` variable handled below is known to not be greater than the container size, as
// checked in `validate_section_headers`. Combined with the requirement for the container
// size to not exceed MAX_INITCODE_SIZE (checked before `validate-header` is called),
// this allows us to cast `offset` to narrower integers.
assert(container.size() <= MAX_INITCODE_SIZE);

const auto section_headers_or_error = validate_section_headers(container);
if (const auto* error = std::get_if<EOFValidationError>(&section_headers_or_error))
return *error;
Expand All @@ -757,6 +770,7 @@
std::vector<uint16_t> code_offsets;
const auto type_section_size = section_headers[TYPE_SECTION][0];
auto offset = header_size + type_section_size;

for (const auto code_size : code_sizes)
{
assert(offset <= std::numeric_limits<uint16_t>::max());
Expand All @@ -768,10 +782,11 @@
std::vector<uint16_t> container_offsets;
for (const auto container_size : container_sizes)
{
assert(offset <= std::numeric_limits<uint16_t>::max());
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());
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the assert.

Copy link
Member

Choose a reason for hiding this comment

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

This looks still removed?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

like 537d394 for example

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

const auto data_offset = static_cast<uint16_t>(offset);

Expand Down Expand Up @@ -971,6 +986,8 @@
return "ambiguous_container_kind";
case EOFValidationError::incompatible_container_kind:
return "incompatible_container_kind";
case EOFValidationError::container_size_above_limit:
return "container_size_above_limit";

Check warning on line 990 in lib/evmone/eof.cpp

View check run for this annotation

Codecov / codecov/patch

lib/evmone/eof.cpp#L989-L990

Added lines #L989 - L990 were not covered by tests
case EOFValidationError::impossible:
return "impossible";
}
Expand Down
1 change: 1 addition & 0 deletions lib/evmone/eof.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ enum class EOFValidationError
toplevel_container_truncated,
ambiguous_container_kind,
incompatible_container_kind,
container_size_above_limit,

impossible,
};
Expand Down
2 changes: 2 additions & 0 deletions test/unittests/eof_validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@
return "EOF_AmbiguousContainerKind";
case EOFValidationError::incompatible_container_kind:
return "EOF_IncompatibleContainerKind";
case EOFValidationError::container_size_above_limit:
return "EOF_ContainerSizeAboveLimit";

Check warning on line 97 in test/unittests/eof_validation.cpp

View check run for this annotation

Codecov / codecov/patch

test/unittests/eof_validation.cpp#L96-L97

Added lines #L96 - L97 were not covered by tests
case EOFValidationError::impossible:
return "impossible";
}
Expand Down
4 changes: 3 additions & 1 deletion test/unittests/eof_validation_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1211,9 +1211,11 @@ TEST_F(eof_validation, EOF1_subcontainer_containing_unreachable_code_sections)

TEST_F(eof_validation, max_nested_containers)
{
constexpr size_t MAX_CODE_SIZE = 0x6000;
constexpr size_t MAX_INITCODE_SIZE = 2 * MAX_CODE_SIZE;
bytecode code{};
bytecode nextcode = eof_bytecode(OP_INVALID);
while (nextcode.size() <= std::numeric_limits<uint16_t>::max())
while (nextcode.size() <= MAX_INITCODE_SIZE)
{
code = nextcode;
nextcode = eof_bytecode(OP_INVALID).container(nextcode);
Expand Down