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

TOB-FUEL-34: Integer overflow in load_byte #568

Closed
xgreenx opened this issue Aug 28, 2023 · 1 comment
Closed

TOB-FUEL-34: Integer overflow in load_byte #568

xgreenx opened this issue Aug 28, 2023 · 1 comment
Labels
audit-report Issue from the audit report

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 28, 2023

Description

The end of the range calculated in store_byte can overflow if a + c == Word::MAX. In the debug profile or if overflow checks are enabled in production then the code panics. Else function returns PanicReason::MemoryOverflow. Note that because the range ac..0 is empty, the ownership checks are passed. Though, the check ac < VM_MAX_RAM fails, because in order to trigger the overflow ac must equal Word::MAX. If VM_MAX_RAM would equal Word::MAX, then invalid data might be written to memory, bypassing the ownership checks.

image

The following figure shows the assembler code which triggered the overflow.

Figure 35.2: Proof of concept code for overflowing the range end.

op::aloc(RegId::ONE),
op::not(16, 59),
op::sb(16, 16, 0), // panic
op::ret(RegId::ONE)

Recommendations

Short term, avoid the overflow by using overflowing_add to add the 1 in order to create a range which can be validated by the ownership checks.
Long term, deploy the fuzzer described in the fuzzing appendix (see appendix E), which is able to discover this bug automatically. Furthermore, consider disallowing the clippy rule integer_arithmetic, which requires checking whether an overflow occurred.

@xgreenx xgreenx added the audit-report Issue from the audit report label Aug 28, 2023
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 28, 2023

Fixed with refactoring caused by #511

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Issue from the audit report
Projects
None yet
Development

No branches or pull requests

1 participant