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

Fix silent value truncation in assembly import #15597

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cameel
Copy link
Member

@cameel cameel commented Nov 29, 2024

While doing #15596 I noticed that the validation against using PUSH with more than 32 bytes does not get triggered in assembly import. Turns out that this is because the import code blindly converts the value field to u256, truncating it if it's too long. This happens for several other assembly item types as well.

Validations for assembly import are generally a mess, but the PR does not attempt to clean that up. It only adds rough checks that should be enough to disallow cases affected by the truncation bug.

@cameel cameel requested review from nikola-matic and aarlt November 29, 2024 02:07
@cameel cameel self-assigned this Nov 29, 2024
@@ -227,16 +239,19 @@ AssemblyItem Assembly::createAssemblyItemFromJSON(Json const& _json, std::vector
else if (name == "PUSH [tag]")
{
requireValueDefinedForInstruction(name, value);
requireValueInRange(name, value, 0, std::numeric_limits<unsigned>::max());
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure unsigned is the right type here. We convert the value to this type in many places, including when generating the bytecode so it seems to me that higher values cannot work, but there's one bit of code that seems to contradict it:

AssemblyItem AssemblyItem::toSubAssemblyTag(size_t _subId) const
{
assertThrow(data() < (u256(1) << 64), util::Exception, "Tag already has subassembly set.");
assertThrow(m_type == PushTag || m_type == Tag, util::Exception, "");
auto tag = static_cast<size_t>(u256(data()) & 0xffffffffffffffffULL);
AssemblyItem r = *this;
r.m_type = PushTag;
r.setPushTagSubIdAndTag(_subId, tag);
return r;
}
std::pair<size_t, size_t> AssemblyItem::splitForeignPushTag() const
{
solAssert(m_type == PushTag || m_type == Tag || m_type == RelativeJump || m_type == ConditionalRelativeJump);
u256 combined = u256(data());
size_t subId = static_cast<size_t>((combined >> 64) - 1);
size_t tag = static_cast<size_t>(combined & 0xffffffffffffffffULL);
return std::make_pair(subId, tag);
}

toSubAssemblyTag() and splitForeignPushTag() apparently expect the value to be u256 with the tag stored in the lower 64 bits. Not sure how to square that with the rest of the code. Is that structure used only temporarily, never reaching the assembling stage?

result = {AssemblyItemType::PushTag, updateUsedTags(u256(value))};
}
else if (name == "PUSH [$]")
{
requireValueDefinedForInstruction(name, value);
requireValueInRange(name, "0x" + value, 0, std::numeric_limits<size_t>::max());
Copy link
Member Author

@cameel cameel Nov 29, 2024

Choose a reason for hiding this comment

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

We really shouldn't be using platform-dependent types here, like unsigned or size_t. This means that validation ranges are platform-dependent too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm only including tests for invalid values, because I didn't get very far testing valid ones. Things get very broken when you try to use very high values that are still in the allowed range. For example using large tag values results in random segfaults. I don't think anyone ever properly considered the corner cases and I suspect that things only work properly when we're far from the limits of the default integer type (regardless of which type various fields claim to need). We should clean up this mess some day.

@cameel cameel force-pushed the fix-value-truncation-in-evmasm-import branch from 2c3935e to 0564aa8 Compare November 29, 2024 02:28
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Dec 13, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Dec 13, 2024
@ethereum ethereum deleted a comment from github-actions bot Dec 16, 2024
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Dec 31, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant