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: fix multi-container-section case #978

Merged
merged 2 commits into from
Aug 22, 2024
Merged

eof: fix multi-container-section case #978

merged 2 commits into from
Aug 22, 2024

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented Aug 21, 2024

Credit to @chfast to find the case via fuzzing, as well as suggest this version of the fix (alternative version in comment, not sure which is cleaner anymore)

lib/evmone/eof.cpp Outdated Show resolved Hide resolved
@pdobacz pdobacz requested a review from chfast August 21, 2024 13:40
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.29%. Comparing base (85ded61) to head (0855eb1).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #978   +/-   ##
=======================================
  Coverage   94.29%   94.29%           
=======================================
  Files         143      143           
  Lines       16145    16148    +3     
=======================================
+ Hits        15224    15227    +3     
  Misses        921      921           
Flag Coverage Δ
eof_execution_spec_tests 16.67% <66.66%> (+<0.01%) ⬆️
ethereum_tests 26.91% <66.66%> (+<0.01%) ⬆️
ethereum_tests_silkpre 18.67% <0.00%> (-0.01%) ⬇️
execution_spec_tests 17.71% <0.00%> (-0.01%) ⬇️
unittests 89.74% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
lib/evmone/eof.cpp 85.68% <100.00%> (+0.02%) ⬆️
test/unittests/eof_validation_test.cpp 99.36% <100.00%> (+<0.01%) ⬆️

@chfast
Copy link
Member

chfast commented Aug 21, 2024

Let me try one more thing here.

@chfast
Copy link
Member

chfast commented Aug 21, 2024

I pushed an alternative fix where we check id != expected twice but the helper get_section_missing_error() remains sane.

@chfast chfast added bug Something isn't working EOF labels Aug 21, 2024
@chfast chfast requested a review from gumb0 August 21, 2024 14:47
@pdobacz pdobacz merged commit ce2a7f0 into master Aug 22, 2024
25 checks passed
@pdobacz pdobacz deleted the multi-cont-section branch August 22, 2024 11:00
@chfast
Copy link
Member

chfast commented Aug 26, 2024

This issue was found indirectly by fuzzing: I was debugging the fuzzer mutation and noticed this weird EOF header. This indicates this validation issue may not be exploitable. Yet, it is a serious bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working EOF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants