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

Misuse of is_less_than_modulus #178

Closed
davidnevadoc opened this issue Oct 28, 2024 · 1 comment
Closed

Misuse of is_less_than_modulus #178

davidnevadoc opened this issue Oct 28, 2024 · 1 comment

Comments

@davidnevadoc
Copy link
Contributor

In the deserialization of prime field elements, we sometimes check that the resulting value lies in the correct range: [0, p-1].
This check is performed by the function is_less_than_modulus:

pub(crate) fn is_less_than_modulus(limbs: &[u64; Self::NUM_LIMBS]) -> bool {
let borrow = limbs.iter().enumerate().fold(0, |borrow, (i, limb)| {
crate::arithmetic::sbb(*limb, Self::MODULUS_LIMBS[i], borrow).1
});
(borrow as u8) & 1 == 1
}

It simply subtracts the modulus as a big integer and checks for an underflow. Note that the input must be in canonical form, not in Montgomery form.
The function is called correctly in some places, but incorrectly in others (mainly in SerdeObject implementation).


Here it is correctly used:

fn from_repr(repr: Self::Repr) -> subtle::CtOption<Self> {
let mut el = #field::default();
crate::serde::endian::Endian::LE.from_bytes(repr.as_ref(), &mut el.0);
subtle::CtOption::new(el * Self::R2, subtle::Choice::from(Self::is_less_than_modulus(&el.0) as u8))
}

(Note that it receives the canonical form, so the input is multiplied by R2, but not in the less_than_modulus call).

But here it is not:

fn from_raw_bytes(bytes: &[u8]) -> Option<Self> {
if bytes.len() != #size {
return None;
}
let elt = Self::from_raw_bytes_unchecked(bytes);
Self::is_less_than_modulus(&elt.0).then(|| elt)
}

and

fn read_raw<R: std::io::Read>(reader: &mut R) -> std::io::Result<Self> {
let mut inner = [0u64; #num_limbs];
for limb in inner.iter_mut() {
let mut buf = [0; 8];
reader.read_exact(&mut buf)?;
*limb = u64::from_le_bytes(buf);
}
let elt = Self(inner);
Self::is_less_than_modulus(&elt.0)
.then(|| elt)
.ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::InvalidData,
"input number is not less than field modulus",
)
})
}

@davidnevadoc
Copy link
Contributor Author

There is no bug, this function can and should be used in both Montgomery and non-Montomery form field elements.

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

1 participant