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

f64 field: BaseElement should not be convertible from u64 or u128 without error #230

Closed
plafer opened this issue Nov 28, 2023 · 1 comment

Comments

@plafer
Copy link
Contributor

plafer commented Nov 28, 2023

We currently convert From<u64> and From<128> for BaseElement (here). I believe these should be TryFrom<u64> and TryFrom<u128>.

If the value is greater than 2^64 - 2^32 + 1, we silently perform a modulus. This can potentially be a vector of attack. For example, in the Miden VM, BaseElement is sometimes used to represent the length of a list. When initializing the VM, we take the list length as a u64, and then into BaseElement. Basically, if the list is very long (>= 2^64 - 2^32 + 1), there will be a silent wrap, and we will end up processing just a few elements in the VM. Now I don't know of any hardware that can store any list of that kind of size, but I can see this wrapping used in conjunction with other vulnerabilities to create a hack.

This is a direct consequence of the fact that if I store 2^64 - 2^32 + 100 in a u64, I see this as being unrepresentable in a BaseElement, as opposed to thinking of it as equivalent to 100.

In case some users need the current behavior, I would create a function, say from_u64_with_wrapping(), and have the canonical Rust conversion (i.e. From/TryFrom) be "safer".

Would love to hear your thoughts on this. Do you also see it as enabling vulnerabilities of some kind?

@irakliyk
Copy link
Collaborator

irakliyk commented Feb 5, 2024

Closed by #243.

@irakliyk irakliyk closed this as completed Feb 5, 2024
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

No branches or pull requests

2 participants